From fe79d49cbdf414bce007799c8930ec3a506fed6c Mon Sep 17 00:00:00 2001 From: Giovanni van der Schelde <27761321+Giovds@users.noreply.github.com> Date: Tue, 22 Nov 2022 22:09:55 +0100 Subject: [PATCH] fix(disable-comment): log a warning when a specified mutator doesn't exist(#3842) Log a warning when the name of the mutator inside a Stryker disable comment does not exist: For example: > WARN Unused 'Stryker disable next-line' directive. Mutator with name 'RandomName' not found. Directive found at: example.ts:1 --- packages/instrumenter/src/instrumenter.ts | 2 +- .../src/transformers/babel-transformer.ts | 4 +- .../src/transformers/directive-bookkeeper.ts | 46 +++++++++++--- .../src/transformers/transformer.ts | 5 +- packages/instrumenter/test/helpers/stubs.ts | 3 + .../test/integration/transformers.it.spec.ts | 7 ++- .../test/unit/instrumenter.spec.ts | 6 +- .../transformers/babel-transformer.spec.ts | 62 +++++++++++++++++++ 8 files changed, 118 insertions(+), 17 deletions(-) diff --git a/packages/instrumenter/src/instrumenter.ts b/packages/instrumenter/src/instrumenter.ts index 8a590e9c6b..172a814b08 100644 --- a/packages/instrumenter/src/instrumenter.ts +++ b/packages/instrumenter/src/instrumenter.ts @@ -37,7 +37,7 @@ export class Instrumenter { const parse = this._createParser(options); for await (const { name, mutate, content } of files) { const ast = await parse(content, name); - this._transform(ast, mutantCollector, { options, mutateDescription: toBabelLineNumber(mutate) }); + this._transform(ast, mutantCollector, { options, mutateDescription: toBabelLineNumber(mutate), logger: this.logger }); const mutatedContent = this._print(ast); outFiles.push({ name, diff --git a/packages/instrumenter/src/transformers/babel-transformer.ts b/packages/instrumenter/src/transformers/babel-transformer.ts index 57ebbe7ec3..fe912b29d9 100644 --- a/packages/instrumenter/src/transformers/babel-transformer.ts +++ b/packages/instrumenter/src/transformers/babel-transformer.ts @@ -27,7 +27,7 @@ type PlacementMap = Map>; export const transformBabel: AstTransformer = ( { root, originFileName, rawContent, offset }, mutantCollector, - { options, mutateDescription }, + { options, mutateDescription, logger }, mutators = allMutators, mutantPlacers = allMutantPlacers ) => { @@ -39,7 +39,7 @@ export const transformBabel: AstTransformer = ( const placementMap: PlacementMap = new Map(); // Create the bookkeeper responsible for the // Stryker ... directives - const directiveBookkeeper = new DirectiveBookkeeper(); + const directiveBookkeeper = new DirectiveBookkeeper(logger, mutators, originFileName); // Now start the actual traversing of the AST // diff --git a/packages/instrumenter/src/transformers/directive-bookkeeper.ts b/packages/instrumenter/src/transformers/directive-bookkeeper.ts index 572aecdee1..c95957885f 100644 --- a/packages/instrumenter/src/transformers/directive-bookkeeper.ts +++ b/packages/instrumenter/src/transformers/directive-bookkeeper.ts @@ -1,6 +1,10 @@ import type { types } from '@babel/core'; import { notEmpty } from '@stryker-mutator/util'; +import { Logger } from '@stryker-mutator/api/logging'; + +import { NodeMutator } from '../mutators/node-mutator.js'; + const WILDCARD = 'all'; const DEFAULT_REASON = 'Ignored using a comment'; @@ -47,18 +51,26 @@ export class DirectiveBookkeeper { private readonly strykerCommentDirectiveRegex = /^\s?Stryker (disable|restore)(?: (next-line))? ([a-zA-Z, ]+)(?::(.+)?)?/; private currentIgnoreRule = rootRule; + private readonly allMutatorNames: string[]; + + constructor(private readonly logger: Logger, private readonly allMutators: NodeMutator[], private readonly originFileName: string) { + this.allMutatorNames = this.allMutators.map((x) => x.name.toLowerCase()); + } public processStrykerDirectives({ loc, leadingComments }: types.Node): void { leadingComments - ?.map( - (comment) => - this.strykerCommentDirectiveRegex.exec(comment.value) as - | [fullMatch: string, directiveType: string, scope: string | undefined, mutators: string, reason: string | undefined] - | null - ) - .filter(notEmpty) - .forEach(([, directiveType, scope, mutators, optionalReason]) => { - const mutatorNames = mutators.split(',').map((mutator) => mutator.trim().toLowerCase()); + ?.map((comment) => ({ + comment, + matchResult: this.strykerCommentDirectiveRegex.exec(comment.value) as + | [fullMatch: string, directiveType: string, scope: string | undefined, mutators: string, reason: string | undefined] + | null, + })) + .filter(({ matchResult }) => notEmpty(matchResult)) + .forEach(({ comment, matchResult }) => { + const [, directiveType, scope, mutators, optionalReason] = matchResult!; + let mutatorNames = mutators.split(',').map((mutator) => mutator.trim()); + this.warnAboutUnusedDirective(mutatorNames, directiveType, scope, comment); + mutatorNames = mutatorNames.map((mutator) => mutator.toLowerCase()); const reason = (optionalReason ?? DEFAULT_REASON).trim(); switch (directiveType) { case 'disable': @@ -89,4 +101,20 @@ export class DirectiveBookkeeper { mutatorName = mutatorName.toLowerCase(); return this.currentIgnoreRule.findIgnoreReason(mutatorName, line); } + + private warnAboutUnusedDirective(mutators: string[], directiveType: string, scope: string | undefined, comment: types.Comment) { + for (const mutator of mutators) { + if (mutator === WILDCARD) continue; + if (!this.allMutatorNames.includes(mutator.toLowerCase())) { + this.logger.warn( + // Scope can be global and therefore undefined + `Unused 'Stryker ${ + scope ? directiveType + ' ' + scope : directiveType + }' directive. Mutator with name '${mutator}' not found. Directive found at: ${this.originFileName}:${comment.loc!.start.line}:${ + comment.loc!.start.column + }.` + ); + } + } + } } diff --git a/packages/instrumenter/src/transformers/transformer.ts b/packages/instrumenter/src/transformers/transformer.ts index 21fece51cd..4a1dbdb6f7 100644 --- a/packages/instrumenter/src/transformers/transformer.ts +++ b/packages/instrumenter/src/transformers/transformer.ts @@ -1,6 +1,8 @@ import { MutateDescription } from '@stryker-mutator/api/core'; import { I } from '@stryker-mutator/util'; +import { Logger } from '@stryker-mutator/api/src/logging/logger'; + import { Ast, AstByFormat, AstFormat } from '../syntax/index.js'; import { TransformerOptions } from './transformer-options.js'; @@ -18,7 +20,7 @@ import { MutantCollector } from './mutant-collector.js'; export function transform( ast: Ast, mutantCollector: I, - transformerContext: Pick + transformerContext: Pick ): void { const context: TransformerContext = { ...transformerContext, @@ -42,4 +44,5 @@ export interface TransformerContext { transform: AstTransformer; options: TransformerOptions; mutateDescription: MutateDescription; + logger: Logger; } diff --git a/packages/instrumenter/test/helpers/stubs.ts b/packages/instrumenter/test/helpers/stubs.ts index e4320caaaf..cf45fef40c 100644 --- a/packages/instrumenter/test/helpers/stubs.ts +++ b/packages/instrumenter/test/helpers/stubs.ts @@ -1,5 +1,7 @@ import sinon from 'sinon'; +import { testInjector } from '@stryker-mutator/test-helpers'; + import { ParserContext } from '../../src/parsers/parser-context.js'; import { PrinterContext } from '../../src/printers/index.js'; import { TransformerContext } from '../../src/transformers/index.js'; @@ -23,5 +25,6 @@ export function transformerContextStub(): sinon.SinonStubbedInstance { const htmlAst = createHtmlAst(); htmlAst.root.scripts.push(createJSAst({ rawContent: 'const foo = 40 + 2' })); const mutantCollector = new MutantCollector(); - transform(htmlAst, mutantCollector, { options: createTransformerOptions(), mutateDescription: true }); + transform(htmlAst, mutantCollector, { options: createTransformerOptions(), mutateDescription: true, logger: testInjector.logger }); expect(mutantCollector.mutants).lengthOf(1); expect(htmlAst).matchSnapshot(); }); it('should transform a js file', () => { const jsAst = createJSAst({ rawContent: 'const foo = 40 + 2' }); const mutantCollector = new MutantCollector(); - transform(jsAst, mutantCollector, { options: createTransformerOptions(), mutateDescription: true }); + transform(jsAst, mutantCollector, { options: createTransformerOptions(), mutateDescription: true, logger: testInjector.logger }); expect(mutantCollector.mutants).lengthOf(1); expect(jsAst).matchSnapshot(); }); it('should transform a ts file', () => { const tsAst = createTSAst({ rawContent: 'const foo: number = 40 + 2' }); const mutantCollector = new MutantCollector(); - transform(tsAst, mutantCollector, { options: createTransformerOptions(), mutateDescription: true }); + transform(tsAst, mutantCollector, { options: createTransformerOptions(), mutateDescription: true, logger: testInjector.logger }); expect(mutantCollector.mutants).lengthOf(1); expect(tsAst).matchSnapshot(); }); diff --git a/packages/instrumenter/test/unit/instrumenter.spec.ts b/packages/instrumenter/test/unit/instrumenter.spec.ts index ae904a2c39..ac956b1fbf 100644 --- a/packages/instrumenter/test/unit/instrumenter.spec.ts +++ b/packages/instrumenter/test/unit/instrumenter.spec.ts @@ -66,7 +66,11 @@ describe(Instrumenter.name, () => { const expected: transformers.TransformerOptions = createInstrumenterOptions({ excludedMutations: [], }); - expect(actual).deep.eq({ options: expected, mutateDescription: [{ start: { line: 1, column: 0 }, end: { line: 7, column: 42 } }] }); + expect(actual).deep.eq({ + options: expected, + mutateDescription: [{ start: { line: 1, column: 0 }, end: { line: 7, column: 42 } }], + logger: testInjector.logger, + }); }); it('should log about instrumenting', async () => { diff --git a/packages/instrumenter/test/unit/transformers/babel-transformer.spec.ts b/packages/instrumenter/test/unit/transformers/babel-transformer.spec.ts index 38fe5b11bd..4a0f899cf3 100644 --- a/packages/instrumenter/test/unit/transformers/babel-transformer.spec.ts +++ b/packages/instrumenter/test/unit/transformers/babel-transformer.spec.ts @@ -443,6 +443,68 @@ describe('babel-transformer', () => { expect(notIgnoredMutants()).lengthOf(1); }); + it('should warn when a mutator name without scope does not match any of the enabled mutators.', () => { + const ast = createTSAst({ + rawContent: `// Stryker disable RandomName: This Mutator does not exist + function test(a, b) { + return a - b >= 0 ? 1 : -1; + } + `, + }); + act(ast); + + expect(context.logger.warn).calledWithMatch( + sinon.match("Unused 'Stryker disable' directive. Mutator with name 'RandomName' not found. Directive found at: example.ts:1") + ); + }); + + it('should warn when a mutator name with scope does not match any of the enabled mutators.', () => { + const ast = createTSAst({ + rawContent: `// Stryker disable next-line RandomName: This Mutator does not exist + function test(a, b) { + return a - b >= 0 ? 1 : -1; + } + `, + }); + act(ast); + + expect(context.logger.warn).calledWithMatch( + sinon.match("Unused 'Stryker disable next-line' directive. Mutator with name 'RandomName' not found. Directive found at: example.ts:1") + ); + }); + + it('should warn when a mutator name does not match any of the enabled mutators.', () => { + const ast = createTSAst({ + rawContent: `// Stryker disable Foo + // Stryker disable RandomName: This Mutator does not exist + // Stryker disable Plus + function test(a, b) { + return a - b >= 0 ? 1 : -1; + } + `, + }); + act(ast); + + expect(context.logger.warn).calledWithMatch( + sinon.match("Unused 'Stryker disable' directive. Mutator with name 'RandomName' not found. Directive found at: example.ts:2") + ); + }); + + it('should not warn when a disabled Mutator exists.', () => { + const ast = createTSAst({ + rawContent: ` + // Stryker disable Foo + // Stryker disable all + function test(a, b) { + return a - b >= 0 ? 1 : -1; + } + `, + }); + act(ast); + + expect(context.logger.warn).not.called; + }); + it('should allow to restore for next-line using a specific "Stryker restore next-line mutator" comment', () => { const ast = createTSAst({ rawContent: `