-
Notifications
You must be signed in to change notification settings - Fork 254
👷(CLDSRV-860) Monitor async await migration #6088
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
base: development/9.3
Are you sure you want to change the base?
Changes from all commits
9dff1c5
afc3df1
5dcc3b4
fbdc6fd
715261b
71cdf6d
3bf3610
d6efbc3
a6705dd
2421cbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /** | ||
| * @name Callback-style function (async migration) | ||
| * @description These functions use callback parameters. They should be refactored to use async/await. | ||
| * @kind problem | ||
| * @problem.severity recommendation | ||
| * @id js/callback-style-function | ||
| * @tags maintainability | ||
| * async-migration | ||
| */ | ||
|
|
||
| import javascript | ||
|
|
||
| from Function f, Parameter p | ||
| where | ||
| p = f.getParameter(f.getNumParameter() - 1) and | ||
| p.getName().regexpMatch("(?i)^(cb|callback|next|done)$") and | ||
| not f.isAsync() and | ||
| // Exclude test files and node_modules | ||
| not f.getFile().getAbsolutePath().matches("%/tests/%") and | ||
| not f.getFile().getAbsolutePath().matches("%/node_modules/%") | ||
| select f, "This function uses a callback parameter ('" + p.getName() + "'). Refactor to async/await." |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| - description: Scality Cloudserver Async Migration Suite | ||
| - queries: . |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| /** | ||
| * @name Promise .then() usage (async migration) | ||
| * @description These calls use .then() instead of async/await. They should be refactored to use async/await. | ||
| * @kind problem | ||
| * @problem.severity recommendation | ||
| * @id js/promise-then-usage | ||
| * @tags maintainability | ||
| * async-migration | ||
| */ | ||
|
|
||
| import javascript | ||
|
|
||
| from MethodCallExpr m | ||
| where | ||
| m.getMethodName() = "then" and | ||
| // Exclude test files and node_modules | ||
| not m.getFile().getAbsolutePath().matches("%/tests/%") and | ||
| not m.getFile().getAbsolutePath().matches("%/node_modules/%") | ||
| select m, "This call uses .then(). Refactor to async/await." |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| name: scality/cloudserver-async-migration | ||
| version: 0.0.1 | ||
| dependencies: | ||
| codeql/javascript-all: "*" |
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| /** | ||
| * Check that all new/modified functions in the current git diff use async/await. | ||
| * Fails with exit code 1 if any additions introduce callback-style functions. | ||
| * | ||
| * Usage: node scripts/check-diff-async.mjs | ||
| * In CI: runs against the current PR diff (files changed vs base branch) | ||
| */ | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { Project, SyntaxKind } from 'ts-morph'; | ||
|
|
||
| const CALLBACK_PARAM_PATTERN = /^(cb|callback|next|done)$/i; | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| function getChangedJsFiles() { | ||
| const base = process.env.GITHUB_BASE_REF | ||
| ? `origin/${process.env.GITHUB_BASE_REF}` | ||
| : 'HEAD'; | ||
| const output = execFileSync('git', [ | ||
| 'diff', | ||
| '--name-only', | ||
| '--diff-filter=ACMR', | ||
| base, | ||
| '--', | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| '**/*.js', | ||
| ], { encoding: 'utf8' }).trim(); | ||
|
|
||
| return output ? output.split('\n').filter(f => f.endsWith('.js')) : []; | ||
| } | ||
|
|
||
| /** | ||
| * Get added line numbers for a file in the current diff. | ||
| */ | ||
| function getAddedLineNumbers(filePath) { | ||
| const base = process.env.GITHUB_BASE_REF | ||
| ? `origin/${process.env.GITHUB_BASE_REF}` | ||
| : 'HEAD'; | ||
| const diff = execFileSync('git', ['diff', base, '--', filePath], { encoding: 'utf8' }); | ||
| const addedLines = new Set(); | ||
| let currentLine = 0; | ||
|
|
||
| for (const line of diff.split('\n')) { | ||
| const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/); | ||
|
|
||
| if (hunkMatch) { | ||
| currentLine = parseInt(hunkMatch[1], 10) - 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (line.startsWith('+') && !line.startsWith('+++')) { | ||
| currentLine++; | ||
| addedLines.add(currentLine); | ||
| } else if (!line.startsWith('-')) { | ||
| currentLine++; | ||
| } | ||
| } | ||
|
|
||
| return addedLines; | ||
| } | ||
|
|
||
| const changedFiles = getChangedJsFiles(); | ||
| if (changedFiles.length === 0) { | ||
| console.log('No changed JS files to check.'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| console.log(`Checking ${changedFiles.length} changed JS file(s) for async/await compliance...\n`); | ||
|
|
||
| const project = new Project({ | ||
| compilerOptions: { allowJs: true, noEmit: true }, | ||
| skipAddingFilesFromTsConfig: true, | ||
| }); | ||
|
|
||
| const filesToCheck = changedFiles.filter(f => | ||
| !f.startsWith('tests/') && | ||
| !f.startsWith('node_modules/') && | ||
| ( | ||
| f.startsWith('lib/') || | ||
| f.startsWith('bin/') || | ||
| !f.includes('/') | ||
| ) | ||
| ); | ||
| if (filesToCheck.length === 0) { | ||
| console.log('No source JS files in diff (tests and node_modules excluded).'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| project.addSourceFilesAtPaths(filesToCheck); | ||
|
|
||
| const violations = []; | ||
|
|
||
| for (const sourceFile of project.getSourceFiles()) { | ||
| const filePath = sourceFile.getFilePath().replace(process.cwd() + '/', ''); | ||
| const addedLines = getAddedLineNumbers(filePath); | ||
|
|
||
| if (addedLines.size === 0) continue; | ||
|
|
||
| const functions = [ | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.FunctionDeclaration), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.FunctionExpression), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.ArrowFunction), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.MethodDeclaration), | ||
| ]; | ||
|
|
||
| for (const fn of functions) { | ||
| if (fn.isAsync()) continue; | ||
|
|
||
| const startLine = fn.getStartLineNumber(); | ||
| if (!addedLines.has(startLine)) continue; | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const params = fn.getParameters(); | ||
| const lastParam = params[params.length - 1]; | ||
| if (lastParam && CALLBACK_PARAM_PATTERN.test(lastParam.getName())) { | ||
| violations.push({ | ||
| file: filePath, | ||
| line: startLine, | ||
| type: 'callback', | ||
| detail: `function has callback parameter '${lastParam.getName()}'`, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (violations.length === 0) { | ||
| console.log('✓ All new code in the diff uses async/await.'); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| console.error(`✗ Found ${violations.length} async/await violation(s) in the diff:\n`); | ||
| for (const v of violations) { | ||
| console.error(` ${v.file}:${v.line} [${v.type}] ${v.detail}`); | ||
| } | ||
| console.error('\nNew code must use async/await instead of callbacks.'); | ||
| console.error('See the async/await migration guide in CONTRIBUTING.md for help.'); | ||
| process.exit(1); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| /** | ||
| * Count async vs callback-style functions across the codebase using ts-morph. | ||
| * Used in CI to track async/await migration progress over time. | ||
| * | ||
| * Usage: node scripts/count-async-functions.mjs | ||
| */ | ||
| import { readFileSync, appendFileSync, writeFileSync } from 'node:fs'; | ||
| import { Project, SyntaxKind } from 'ts-morph'; | ||
|
|
||
| function getSourcePathsFromPackageJson() { | ||
| const packageJsonPath = new URL('../../package.json', import.meta.url); | ||
| const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8')); | ||
| const paths = packageJson.countAsyncSourcePaths; | ||
|
|
||
| if (Array.isArray(paths) && paths.length > 0 && paths.every(p => typeof p === 'string')) { | ||
| return paths; | ||
| } | ||
|
|
||
| throw new Error('package.json must define a non-empty string array "countAsyncSourcePaths"'); | ||
| } | ||
|
|
||
| const project = new Project({ | ||
| compilerOptions: { | ||
| allowJs: true, | ||
| noEmit: true, | ||
| }, | ||
| skipAddingFilesFromTsConfig: true, | ||
| }); | ||
|
|
||
| project.addSourceFilesAtPaths(getSourcePathsFromPackageJson()); | ||
|
|
||
| let asyncFunctions = 0; | ||
| let totalFunctions = 0; | ||
| let callbackFunctions = 0; | ||
| let thenChains = 0; | ||
|
|
||
| const CALLBACK_PARAM_PATTERN = /^(cb|callback|next|done)$/i; | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| for (const sourceFile of project.getSourceFiles()) { | ||
| const functions = [ | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.FunctionDeclaration), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.FunctionExpression), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.ArrowFunction), | ||
| ...sourceFile.getDescendantsOfKind(SyntaxKind.MethodDeclaration), | ||
| ]; | ||
|
|
||
| for (const fn of functions) { | ||
| totalFunctions++; | ||
|
|
||
| if (fn.isAsync()) { | ||
| asyncFunctions++; | ||
| continue; | ||
| } | ||
|
|
||
| const params = fn.getParameters(); | ||
| const lastParam = params[params.length - 1]; | ||
| if (lastParam && CALLBACK_PARAM_PATTERN.test(lastParam.getName())) { | ||
| callbackFunctions++; | ||
| } | ||
| } | ||
|
|
||
| const propertyAccesses = sourceFile.getDescendantsOfKind(SyntaxKind.PropertyAccessExpression); | ||
| for (const access of propertyAccesses) { | ||
| if (access.getName() === 'then') { | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| thenChains++; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const asyncFunctionPercent = totalFunctions > 0 | ||
| ? ((asyncFunctions / totalFunctions) * 100).toFixed(1) | ||
| : '0.0'; | ||
|
|
||
| const migrationPercent = (asyncFunctions + callbackFunctions) > 0 | ||
| ? ((asyncFunctions / (asyncFunctions + callbackFunctions)) * 100).toFixed(1) | ||
| : '0.0'; | ||
|
|
||
| console.log('=== Async/Await Migration Progress ==='); | ||
| console.log(`Total functions: ${totalFunctions}`); | ||
| console.log(`Async functions: ${asyncFunctions} (${asyncFunctionPercent}%)`); | ||
| console.log(`Callback functions: ${callbackFunctions}`); | ||
| console.log(`Remaining .then(): ${thenChains}`); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we also count "dual-style" functions (async + cb)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's useful right now ? |
||
| console.log(''); | ||
| console.log(`Migration (trend): ${asyncFunctions}/${asyncFunctions + callbackFunctions} (${migrationPercent}%)`); | ||
|
|
||
| if (process.env.GITHUB_STEP_SUMMARY) { | ||
| appendFileSync(process.env.GITHUB_STEP_SUMMARY, [ | ||
francoisferrand marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| '## Async/Await Migration Progress', | ||
| '', | ||
| `| Metric | Count |`, | ||
| `|--------|-------|`, | ||
| `| Total functions | ${totalFunctions} |`, | ||
| `| Async functions | ${asyncFunctions} (${asyncFunctionPercent}%) |`, | ||
| `| Callback-style functions | ${callbackFunctions} |`, | ||
| `| Remaining \`.then()\` chains | ${thenChains} |`, | ||
| `| Migration trend (async / (async + callback)) | ${asyncFunctions}/${asyncFunctions + callbackFunctions} (${migrationPercent}%) |`, | ||
| '', | ||
| ].join('\n')); | ||
|
|
||
| // Output benchmark JSON for visualization | ||
| const benchmarkData = [ | ||
| { | ||
| name: 'Async Migration Progress', | ||
| unit: '%', | ||
| value: parseFloat(migrationPercent), | ||
| }, | ||
| { | ||
| name: 'Async Functions Percentage', | ||
| unit: '%', | ||
| value: parseFloat(asyncFunctionPercent), | ||
| }, | ||
| { | ||
| name: 'Total callback functions', | ||
| unit: 'count', | ||
| value: callbackFunctions, | ||
| } | ||
| ]; | ||
| writeFileSync('async-migration-benchmark.json', JSON.stringify(benchmarkData, null, 2)); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,6 +80,8 @@ jobs: | |
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '22' | ||
|
|
@@ -95,14 +97,32 @@ jobs: | |
| cache: pip | ||
| - name: Install python deps | ||
| run: pip install flake8 | ||
| - name: Lint Javascript | ||
| run: yarn run --silent lint -- --max-warnings 0 | ||
| - name: Lint Javascript (strict, excluding async migration rules) | ||
| run: | | ||
| yarn run --silent lint -- --max-warnings 0 --rule "promise/prefer-await-to-then: off" --rule "n/callback-return: off" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should these rule exceptions not be added in the package.json instead? or do we expect developers to run lint with this rule, and be smart about ignoring them?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good question. I would answer with smart dev to make sure we don't miss anything in the lint... But I can define a new script if you prefer and think it's a bad idea.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. being smart about ignoring 1 warning is a thing, but may be difficult if there are many... which I guess would be the case here? Maybe could be the other way, i.e. default to off in eslint config, and we can anyway enable them explicitely with
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - name: Lint Markdown | ||
| run: yarn run --silent lint_md | ||
| - name: Lint python | ||
| run: flake8 $(git ls-files "*.py") | ||
| - name: Lint Yaml | ||
| run: yamllint -c yamllint.yml $(git ls-files "*.yml") | ||
| - name: Check async/await compliance in diff | ||
| run: yarn run check-diff-async | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| async-migration-report: | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| runs-on: ubuntu-24.04 | ||
DarkIsDude marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if: startsWith(github.ref, 'refs/heads/development/') | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '22' | ||
| cache: yarn | ||
| - name: install dependencies | ||
| run: yarn install --frozen-lockfile --network-concurrency 1 | ||
| - name: Count async/await migration progress | ||
| run: yarn run count-async | ||
|
|
||
| unit-tests: | ||
| runs-on: ubuntu-24.04 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| import mocha from "eslint-plugin-mocha"; | ||
| import promise from "eslint-plugin-promise"; | ||
| import n from "eslint-plugin-n"; | ||
| import path from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import js from "@eslint/js"; | ||
|
|
@@ -15,6 +17,8 @@ const compat = new FlatCompat({ | |
| export default [...compat.extends('@scality/scality'), { | ||
| plugins: { | ||
| mocha, | ||
| promise, | ||
| n, | ||
| }, | ||
|
|
||
| languageOptions: { | ||
|
|
@@ -67,5 +71,7 @@ export default [...compat.extends('@scality/scality'), { | |
| "quote-props": "off", | ||
| "mocha/no-exclusive-tests": "error", | ||
| "no-redeclare": ["error", { "builtinGlobals": false }], | ||
| "promise/prefer-await-to-then": "warn", | ||
| "n/callback-return": "warn", | ||
|
Comment on lines
+74
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
i.e. should we really add these now?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how "lots" ? if it makes the tool unusable, nobody will use it....and there won't be a visual either :/ |
||
| }, | ||
| }]; | ||
Uh oh!
There was an error while loading. Please reload this page.