Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dev-tool] Fix eslint errors and re-enable linter in dev-tool #18853

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/tools/dev-tool/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"integration-test:browser": "echo skipped",
"integration-test:node": "echo skipped",
"lint:fix": "eslint --no-eslintrc -c ../../.eslintrc.internal.json package.json src test --ext .ts --fix --fix-type [problem,suggestion]",
"lint": "eslint --no-eslintrc -c ../../../sdk/.eslintrc.internal.json package.json src test --ext .ts -f html -o template-lintReport.html || exit 0",
"lint": "eslint --no-eslintrc -c ../../../sdk/.eslintrc.internal.json package.json src test --ext .ts",
"pack": "npm pack 2>&1",
"prebuild": "npm run clean",
"unit-test": "npm run unit-test:node",
Expand Down
3 changes: 2 additions & 1 deletion common/tools/dev-tool/register.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ const { name: hostPackageName } = main.require("./package.json");

// We need to use whatever version of TypeScript the calling package uses to inspect syntax nodes, because
// that is what the ts-node invocation will use, and we need to agree with it on syntax brands.
const ts = hostPackageName === "@azure/dev-tool"
const ts =
hostPackageName === "@azure/dev-tool"
? require(path.join(cwd, "node_modules", "typescript"))
: main.require("typescript");

Expand Down
2 changes: 1 addition & 1 deletion common/tools/dev-tool/src/commands/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const baseCommandInfo = makeCommandInfo("dev-tool", "Azure SDK for JS dev
/**
* Default dev-tool subcommand
*/
export const baseCommand = async (...args: string[]) => {
export const baseCommand = async (...args: string[]): Promise<void> => {
const status = await subCommand(baseCommandInfo, baseCommands)(...args);

if (!status) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,30 +87,30 @@ function buildRunSamplesScript(
const scriptContent = `#!/bin/sh

function install_dependencies_helper() {
local samples_path=\$1;
local samples_path=$1;
cd \${samples_path};
${compileCMD(`npm install ${artifactURL}`, printToScreen)}
${compileCMD(`npm install`, printToScreen)}
}

function install_packages() {
echo "Using node \$(node -v) to install dependencies";
echo "Using node $(node -v) to install dependencies";
install_dependencies_helper ${samplesDir}/javascript
install_dependencies_helper ${samplesDir}/typescript;
cp ${envFilePath} ${samplesDir}/javascript/;
}

function run_samples() {
samples_path=\$1;
echo "Using node \$(node -v) to run samples in \${samples_path}";
samples_path=$1;
echo "Using node $(node -v) to run samples in \${samples_path}";
cd "\${samples_path}";
for SAMPLE in *.js; do
node \${SAMPLE};
done
}

function build_typescript() {
echo "Using node \$(node -v) to build the typescript samples";
echo "Using node $(node -v) to build the typescript samples";
cd ${samplesDir}/typescript
${compileCMD(`npm run build`, printToScreen)}
cp ${envFilePath} ${samplesDir}/typescript/dist/
Expand Down Expand Up @@ -273,7 +273,7 @@ export default leafCommand(commandInfo, async (options) => {
options["node-versions"]
?.split(",")
.concat(options["node-version"])
.filter((ver) => ver !== "" && parseInt(ver) !== NaN)
.filter((ver) => ver !== "" && !isNaN(parseInt(ver)))
)
];
const dockerContextDirectory: string =
Expand Down
2 changes: 1 addition & 1 deletion common/tools/dev-tool/src/commands/samples/prep.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ async function enableLocalRun(
// that can escape both linux and windows path separators
const depth = relativeDir.length - relativeDir.split(path.sep).join("").length;

let relativePath = new Array(depth).fill("..").join("/");
const relativePath = new Array(depth).fill("..").join("/");

outputContent = fileContents.replace(
importRegex,
Expand Down
8 changes: 6 additions & 2 deletions common/tools/dev-tool/src/commands/samples/publish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ function isDependency(moduleSpecifier: string): boolean {

// This seems like a reasonable test for "is a relative path" as long as
// absolute path imports are forbidden.
const isRelativePath = /^\.\.?[\/\\]/.test(moduleSpecifier);
const isRelativePath = /^\.\.?[/\\]/.test(moduleSpecifier);
return !isRelativePath;
}

Expand Down Expand Up @@ -372,10 +372,14 @@ async function makeSampleGenerationInfo(
// Resolve snippets to actual text
customSnippets: Object.entries(sampleConfiguration.customSnippets ?? {}).reduce(
(accum, [name, file]) => {
if (!file) {
return accum;
}

let contents;

try {
contents = fs.readFileSync(file!);
contents = fs.readFileSync(file);
} catch (ex) {
fail(`Failed to read custom snippet file '${file}'`, ex);
}
Expand Down
4 changes: 2 additions & 2 deletions common/tools/dev-tool/src/commands/samples/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ export const commandInfo = makeCommandInfo(
async function runSingle(name: string, accumulatedErrors: Array<[string, string]>) {
log("Running", name);
try {
if (/.*[\\\/]samples(-dev)?[\\\/].*/.exec(name)) {
if (/.*[\\/]samples(-dev)?[\\/].*/.exec(name)) {
// This is an un-prepared sample, so just require it and it will run.
await import(name);
} else if (!/.*[\\\/]dist-samples[\\\/].*/.exec(name)) {
} else if (!/.*[\\/]dist-samples[\\/].*/.exec(name)) {
// This is not an unprepared or a prepared sample
log.error("Skipped. This file is not in any samples folder.");
} else {
Expand Down
4 changes: 3 additions & 1 deletion common/tools/dev-tool/src/commands/samples/tsToJs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { EOL } from "os";
import * as prettier from "prettier";
import ts from "typescript";

import defaultPrettierOptions from "../../../../eslint-plugin-azure-sdk/prettier.json";

import { leafCommand, makeCommandInfo } from "../../framework/command";

import { createPrinter } from "../../util/printer";
Expand All @@ -20,7 +22,7 @@ export const commandInfo = makeCommandInfo(
);

const prettierOptions: prettier.Options = {
...(require("../../../../eslint-plugin-azure-sdk/prettier.json") as prettier.Options),
...(defaultPrettierOptions as prettier.Options),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this will work. I believe we need the dynamic behavior of require. /cc @witemple-msft

parser: "typescript"
};

Expand Down
2 changes: 1 addition & 1 deletion common/tools/dev-tool/src/commands/test-proxy/start.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export const commandInfo = makeCommandInfo(
{}
);

export default leafCommand(commandInfo, async (_options) => {
export default leafCommand(commandInfo, async () => {
await startProxyTool();
return true;
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export const commandInfo = makeCommandInfo(
{}
);

export default leafCommand(commandInfo, async (_options) => {
export default leafCommand(commandInfo, async () => {
const result = await checkWithTimeout(isProxyToolActive, 1000, 120000);
return result;
});
7 changes: 5 additions & 2 deletions common/tools/dev-tool/src/config/rollup.base.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function makeOnWarnForTesting(): (warning: RollupWarning, warn: WarningHandler)

// #endregion

export function makeBrowserTestConfig() {
export function makeBrowserTestConfig(): RollupOptions {
const config: RollupOptions = {
input: {
include: ["dist-esm/test/**/*.spec.js"],
Expand Down Expand Up @@ -151,7 +151,10 @@ const defaultConfigurationOptions: ConfigurationOptions = {
disableBrowserBundle: false
};

export function makeConfig(pkg: PackageJson, options?: Partial<ConfigurationOptions>) {
export function makeConfig(
pkg: PackageJson,
options?: Partial<ConfigurationOptions>
): RollupOptions[] {
options = {
...defaultConfigurationOptions,
...(options ?? {})
Expand Down
2 changes: 1 addition & 1 deletion common/tools/dev-tool/src/framework/CommandModule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,4 @@ export interface CommandModule<Options extends CommandOptions> {
/**
* A map from command name to an async function that loads its module
*/
export type CommandLoader = { [k: string]: () => Promise<CommandModule<{}>> };
export type CommandLoader = { [k: string]: () => Promise<CommandModule<CommandOptions>> };
2 changes: 1 addition & 1 deletion common/tools/dev-tool/src/templates/sampleReadme.md.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ function exampleNodeInvocation(info: SampleReadmeConfiguration) {
/**
* Creates a README for a sample package from a SampleReadmeConfiguration.
*/
export default (info: SampleReadmeConfiguration) => {
export default (info: SampleReadmeConfiguration): string => {
let stepCount = 1;
const step = (content: string) => `${stepCount++}. ${content}`;

Expand Down
4 changes: 2 additions & 2 deletions common/tools/dev-tool/src/util/checkWithTimeout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ const log = createPrinter("check-with-timeout");
*/
export async function checkWithTimeout(
predicate: () => boolean | Promise<boolean>,
delayBetweenRetriesInMilliseconds: number = 1000,
maxWaitTimeInMilliseconds: number = 10000
delayBetweenRetriesInMilliseconds = 1000,
maxWaitTimeInMilliseconds = 10000
): Promise<boolean> {
const maxTime = Date.now() + maxWaitTimeInMilliseconds;
while (Date.now() < maxTime) {
Expand Down
2 changes: 1 addition & 1 deletion common/tools/dev-tool/src/util/findMatchingFiles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export async function* findMatchingFiles(
dir: string,
matches: (name: string, entry: fs.Stats) => boolean,
findOptions?: Partial<FindOptions>
) {
): AsyncGenerator<string> {
const q: FileInfo[] = [];

const options: FindOptions = { ...defaultFindOptions, ...findOptions };
Expand Down
2 changes: 1 addition & 1 deletion common/tools/dev-tool/src/util/sampleConfiguration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ const removeJsTsExtensions = (name: string): string => name.replace(/\.[jt]s$/,
* @param info - FileInfo of a sample file to be considered
* @param skips - a list of strings that identify files to be skipped
*/
export function shouldSkip(info: FileInfo, skips: string[]) {
export function shouldSkip(info: FileInfo, skips: string[]): boolean {
// Add a slash to the skip if necessary
const addFirstSlash = (skip: string): string => (skip.startsWith("/") ? skip : "/" + skip);

Expand Down
30 changes: 18 additions & 12 deletions common/tools/dev-tool/src/util/testProxyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,15 @@ import fs from "fs-extra";
import { createPrinter } from "./printer";

const log = createPrinter("test-proxy");
export async function startProxyTool() {
log.info(
`Attempting to start test proxy at http://localhost:5000 & https://localhost:5001.\n`
);
export async function startProxyTool(): Promise<void> {
log.info(`Attempting to start test proxy at http://localhost:5000 & https://localhost:5001.\n`);

const subprocess = spawn(await getDockerRunCommand(), [], {
shell: true
shell: true,
});

const outFileName = "test-proxy-output.log";
const out = fs.createWriteStream(`./${outFileName}`, { flags: 'a' });
const out = fs.createWriteStream(`./${outFileName}`, { flags: "a" });
subprocess.stdout.pipe(out);
subprocess.stderr.pipe(out);

Expand Down Expand Up @@ -47,7 +45,7 @@ async function getDockerRunCommand() {
return `docker run -v ${repoRoot}:${testProxyRecordingsLocation} -p 5001:5001 -p 5000:5000 ${allowLocalhostAccess} ${imageToLoad}`;
}

export async function isProxyToolActive() {
export async function isProxyToolActive(): Promise<boolean> {
try {
await makeRequest("http://localhost:5000/info/available", {});
log.info(`Proxy tool seems to be active at http://localhost:5000\n`);
Expand All @@ -70,18 +68,26 @@ async function getImageTag() {
//
// $SELECTED_IMAGE_TAG = "1147815";
// (Bot regularly updates the tag in the file above.)

let contentInPWSHScript: string;
try {
const contentInPWSHScript = await fs.readFile(
contentInPWSHScript = await fs.readFile(
`${path.join(await getRootLocation(), "eng/common/testproxy/docker-start-proxy.ps1")}`,
"utf-8"
);
const tag = contentInPWSHScript.match(/\$SELECTED_IMAGE_TAG \= \"(.*)\"/)![1];
log.info(`Image tag obtained from the powershell script => ${tag}\n`);
return tag;
} catch (_) {
log.warn(`Unable to read the powershell script, trying "latest" tag instead\n`);
return "latest";
}

const tag = contentInPWSHScript.match(/\$SELECTED_IMAGE_TAG = "(.*)"/)?.[1];
if (!tag) {
log.warn(
`Unable to get the image tag from the powershell script, trying "latest" tag instead\n`
`Unable to locate the image tag in the powershell script, trying "latest" tag instead\n`
);
return "latest";
}

log.info(`Image tag obtained from the powershell script => ${tag}\n`);
return tag;
}
4 changes: 3 additions & 1 deletion common/tools/dev-tool/src/util/testUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ async function shouldRunProxyTool(): Promise<boolean> {
}
}

export async function runTestsWithProxyTool(testCommandObj: concurrently.CommandObj) {
export async function runTestsWithProxyTool(
testCommandObj: concurrently.CommandObj
): Promise<boolean> {
if (
await shouldRunProxyTool() // Boolean to figure out if we need to run just the mocha command or the test-proxy too
) {
Expand Down
2 changes: 2 additions & 0 deletions common/tools/dev-tool/test/framework.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@ describe("Command Framework", () => {
before(() => {
// Silence the logger
updateBackend({
/* eslint-disable @typescript-eslint/no-empty-function */
error: () => {},
warn: () => {},
info: () => {},
log: () => {}
/* eslint-enable @typescript-eslint/no-empty-function */
Comment on lines +30 to +35
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT:

Suggested change
/* eslint-disable @typescript-eslint/no-empty-function */
error: () => {},
warn: () => {},
info: () => {},
log: () => {}
/* eslint-enable @typescript-eslint/no-empty-function */
error: () => { /** empty body */ },
warn: () => { /** empty body */ },
info: () => { /** empty body */ },
log: () => { /** empty body */ }

});
});

Expand Down
3 changes: 2 additions & 1 deletion common/tools/dev-tool/test/samples/skips.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license.

import { assert } from "chai";
import { Stats } from "fs";

import { FileInfo } from "../../src/util/findMatchingFiles";
import { shouldSkip } from "../../src/util/sampleConfiguration";
Expand All @@ -17,7 +18,7 @@ export async function toFileInfo(fullPath: string): Promise<FileInfo> {
fullPath,
dir: path.dirname(fullPath),
name: path.basename(fullPath),
stat: {} as any
stat: {} as Stats
};
}

Expand Down