diff --git a/src/steps/upgrade/twopointoh/backend/phpunit.ts b/src/steps/upgrade/twopointoh/backend/phpunit.ts index ce99214..ff030a8 100644 --- a/src/steps/upgrade/twopointoh/backend/phpunit.ts +++ b/src/steps/upgrade/twopointoh/backend/phpunit.ts @@ -39,7 +39,7 @@ export default class PhpUnit extends BaseUpgradeStep { 'https://github.com/sebastianbergmann/phpunit/blob/11.3.0/DEPRECATIONS.md', ]; - const dbLink = 'http://localhost:3000/extend/testing#model-factories'; + const dbLink = 'https://docs.flarum.org/2.x/extend/testing#model-factories'; return `Flarum 2.0 uses PHPUnit 11. The tool has applied the most significant changes, but you might still run into other deprecations. Please refer to the following links for more information: diff --git a/src/steps/upgrade/twopointoh/base.ts b/src/steps/upgrade/twopointoh/base.ts index 464b129..ee16e2d 100644 --- a/src/steps/upgrade/twopointoh/base.ts +++ b/src/steps/upgrade/twopointoh/base.ts @@ -15,6 +15,7 @@ import s from 'string'; import simpleGit from 'simple-git'; import { cloneNode } from '@babel/types'; import { PhpProvider } from '../../../providers/php-provider'; +import { StepFailures } from './step-failures'; export type ReplacementResult = { imports?: ImportChange[]; @@ -127,6 +128,8 @@ export abstract class BaseUpgradeStep implements Step { dots = (dots + 1) % 4; }; + const failures = new StepFailures(); + const targets = this.targets(); for (const target of targets) { @@ -145,9 +148,16 @@ export abstract class BaseUpgradeStep implements Step { } const code = fsEditor.read(file); - const advanced = this.advancedContent(file, code); - this.before(file, code, advanced); + // Same as the transform pass below: a file this step can't read + // shouldn't stop it from reporting on the others. + try { + const advanced = this.advancedContent(file, code); + + this.before(file, code, advanced); + } catch (error) { + failures.record(s(file.replace(paths.package(), '')).stripLeft('/').stripLeft('\\').toString(), error); + } progress(); } @@ -163,12 +173,22 @@ export abstract class BaseUpgradeStep implements Step { } const code = fsEditor.read(file); - const advanced = this.advancedContent(file, code); - const relativeTarget = s(file.replace(paths.package(), '')).stripLeft('/').stripLeft('\\').toString(); - // eslint-disable-next-line no-await-in-loop - const result = await this.applyReplacements(relativeTarget, code, advanced); + let result; + + // One file the step can't handle shouldn't decide the fate of the + // rest: note it and carry on, so the author is told about every + // problem this step found rather than the first one. + try { + const advanced = this.advancedContent(file, code); + + // eslint-disable-next-line no-await-in-loop + result = await this.applyReplacements(relativeTarget, code, advanced); + } catch (error) { + failures.record(relativeTarget, error); + continue; + } if (result.newPath && result.newPath !== file) { fsEditor.move(file, result.newPath!); @@ -213,7 +233,30 @@ export abstract class BaseUpgradeStep implements Step { // } } - const changesMade = await simpleGit(paths.requestedDir() ?? paths.cwd()) + const dir = paths.requestedDir() ?? paths.cwd(); + + if (failures.any()) { + // Leave the step all-or-nothing. Its partial writes are already on disk, + // and the run refuses to start with a dirty tree, so keeping them would + // block the re-run this step is asking the author to do. + await simpleGit(dir).checkout(['--', '.']); + + this.command.log('\u001B[A => ' + chalk.bgRed.bold(' FAILED ')); + this.command.log(''); + this.command.log(failures.report()); + this.command.log(''); + + const files = failures.count() === 1 ? 'file' : 'files'; + + this.command.error( + `${failures.count()} ${files} could not be upgraded by this step, so it made no changes.\n` + + ' Fix the problems above and run the command again — steps that already\n' + + ' completed are skipped, so it will resume from here.', + { code: 'FL_ERR' } + ); + } + + const changesMade = await simpleGit(dir) .diffSummary() .then((summary) => summary.files.length > 0); @@ -265,8 +308,7 @@ export abstract class BaseUpgradeStep implements Step { try { advanced = parseCode(code); } catch (error) { - this.command.warn(`Failed to parse code for ${file}, code: \n${code}`); - throw error; + throw new Error(`The transformed code could not be parsed back: ${(error as Error).message}`); } result.imports?.forEach((imp) => { @@ -332,13 +374,17 @@ export abstract class BaseUpgradeStep implements Step { try { return JSON.parse(code); - } catch { - this.command.error(`Failed to parse JSON file ${file}`); + } catch (error) { + throw new Error(`Not valid JSON: ${(error as Error).message}`); } } if (['js', 'ts', 'jsx', 'tsx'].includes(lang || '')) { - return parseCode(code); + try { + return parseCode(code); + } catch (error) { + throw new Error(`Could not parse: ${(error as Error).message}`); + } } return null; @@ -365,8 +411,7 @@ export abstract class BaseUpgradeStep implements Step { try { return generateCode(updated as t.File, false); } catch (error) { - this.command.warn(`Failed to generate code for ${file}, code: \n${code}`); - throw error; + throw new Error(`Could not generate code from the transformed syntax tree: ${(error as Error).message}`); } } diff --git a/src/steps/upgrade/twopointoh/step-failures.ts b/src/steps/upgrade/twopointoh/step-failures.ts new file mode 100644 index 0000000..93ec5ed --- /dev/null +++ b/src/steps/upgrade/twopointoh/step-failures.ts @@ -0,0 +1,54 @@ +import chalk from 'chalk'; + +type Failure = { + file: string; + reason: string; +}; + +/** + * The files a single upgrade step could not transform. + * + * A step runs over every matching file in an extension, and any one of them + * can defeat it — a parser that doesn't recognise some syntax, a transformer + * meeting a shape it wasn't written for. Collecting those instead of throwing + * at the first means the author is told about all of them at once, and fixes + * them in one pass rather than rediscovering them one re-run at a time. + */ +export class StepFailures { + private failures: Failure[] = []; + + record(file: string, error: unknown): void { + this.failures.push({ file, reason: StepFailures.reasonFor(error) }); + } + + any(): boolean { + return this.failures.length > 0; + } + + count(): number { + return this.failures.length; + } + + /** + * The failures, formatted for the author: which file, and why. + */ + report(): string { + return this.failures.map(({ file, reason }) => ` ${chalk.bold(file)}\n ${reason}`).join('\n\n'); + } + + /** + * The message alone. A stack trace is about our internals, not about the + * code being upgraded, so it isn't what the author needs to see. + */ + private static reasonFor(error: unknown): string { + if (error instanceof Error) { + return error.message; + } + + if (typeof error === 'string') { + return error; + } + + return JSON.stringify(error); + } +} diff --git a/src/utils/ast.ts b/src/utils/ast.ts index 98cf618..12271d4 100644 --- a/src/utils/ast.ts +++ b/src/utils/ast.ts @@ -8,14 +8,9 @@ import prettierConfig from '@flarum/prettier-config/prettierrc.json'; import * as recast from 'recast'; export function parseCode(code: string): t.File { - try { - return recast.parse(code, { - parser: require('recast/parsers/babel-ts'), - }); - } catch (error) { - console.log(code); - throw error; - } + return recast.parse(code, { + parser: require('recast/parsers/babel-ts'), + }); } export async function generateCode(ast: t.File, extenders = false): Promise { diff --git a/test/steps/upgrade/step-failures.test.ts b/test/steps/upgrade/step-failures.test.ts new file mode 100644 index 0000000..fcd626c --- /dev/null +++ b/test/steps/upgrade/step-failures.test.ts @@ -0,0 +1,68 @@ +import { StepFailures } from '../../../src/steps/upgrade/twopointoh/step-failures'; + +describe('StepFailures', () => { + it('has nothing to report when no file failed', () => { + const failures = new StepFailures(); + + expect(failures.any()).toBe(false); + expect(failures.count()).toBe(0); + }); + + it('records the file and the reason it failed', () => { + const failures = new StepFailures(); + + failures.record('src/Api/ListThingController.php', new Error('Unexpected token at line 42')); + + expect(failures.any()).toBe(true); + expect(failures.count()).toBe(1); + + const report = failures.report(); + + expect(report).toContain('src/Api/ListThingController.php'); + expect(report).toContain('Unexpected token at line 42'); + }); + + it('keeps going after the first failure so one run surfaces them all', () => { + // The point of collecting rather than throwing on the first: an author + // fixes everything the step found in one pass, instead of rediscovering + // problems one re-run at a time. + const failures = new StepFailures(); + + failures.record('a.php', new Error('first problem')); + failures.record('b.php', new Error('second problem')); + failures.record('c.php', new Error('third problem')); + + expect(failures.count()).toBe(3); + + const report = failures.report(); + + expect(report).toContain('a.php'); + expect(report).toContain('b.php'); + expect(report).toContain('c.php'); + expect(report).toContain('third problem'); + }); + + it('reports a non-Error throw without losing what it was', () => { + const failures = new StepFailures(); + + // Transformers can reject with a string, or with a PHP subsystem payload. + failures.record('odd.php', 'a plain string failure'); + + expect(failures.report()).toContain('a plain string failure'); + }); + + it('reports the message rather than the whole stack', () => { + // Stacks belong behind a debug flag; the author needs the file and the + // reason, not our internals. + const error = new Error('the useful part'); + error.stack = 'Error: the useful part\n at Object. (/cli/src/internal.ts:1:1)'; + + const failures = new StepFailures(); + failures.record('x.php', error); + + const report = failures.report(); + + expect(report).toContain('the useful part'); + expect(report).not.toContain('/cli/src/internal.ts'); + }); +});