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

feat(typescript): Disable type checking #2446

Merged
merged 13 commits into from
Sep 2, 2020
22 changes: 8 additions & 14 deletions packages/api/schema/stryker-core.json
Original file line number Diff line number Diff line change
Expand Up @@ -168,19 +168,8 @@
"type": "object",
"additionalProperties": false,
"properties": {
"fileHeaders": {
"type": "object",
"title": "SandboxFileHeaders",
"description": "Configure additional headers to be added to files inside your sandbox. These headers will be added after Stryker has instrumented your code with mutants, but before a test runner or build command is executed. This is used to ignore typescript compile errors and eslint warnings that might have been added in the process of instrumenting your code with mutants. The default setting should work for most use cases.",
"additionalProperties": {
"type": "string"
},
"default": {
"**/*+(.js|.ts|.cjs|.mjs)?(x)": "/* eslint-disable */\n// @ts-nocheck\n"
}
},
"stripComments": {
"description": "Configure files to be stripped of comments (either single line with `//` or multi line with `/**/`. These comments will be stripped after Stryker has instrumented your code with mutants, but before a test runner or build command is executed. This is used to remove any lingering `// @ts-check` or `// @ts-expect-error` comments that interfere with typescript compilation. The default setting allows comments to be stripped from all JavaScript and friend files in your sandbox, you can specify a different glob expression or set it to `false` to completely disable this behavior.",
"disableTypeChecking": {
nicojs marked this conversation as resolved.
Show resolved Hide resolved
"description": "Configure a pattern that matches the files of which type checking has to be disabled. This is needed because Stryker will create (typescript) type errors when inserting the mutants in your code. Stryker disables type checking by inserting `// @ts-nocheck` atop those files and removing other `// @ts-xxx` directives (so they won't interfere with `@ts-nocheck`). The default setting allows these directives to be stripped from all JavaScript and friend files in your sandbox, you can specify a different glob expression or set it to `false` to completely disable this behavior.",
"anyOf": [
{
"enum": [
Expand All @@ -191,7 +180,7 @@
"type": "string"
}
],
"default": "**/*+(.js|.ts|.cjs|.mjs)?(x)"
"default": "**/*.{js,ts,jsx,tsx,html,vue}"
Copy link
Member Author

Choose a reason for hiding this comment

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

I have second doubts about this default. It might be too broad. For example, when running Stryker on express you get this warning:

18:36:16 (20065) WARN DisableTypeCheckingPreprocessor Unable to disable type checking for file "/home/nicojs/stryker/perf/test/express/examples/ejs/views/footer.html". Shouldn't type checking be disabled for this file? Consider configuring a more restrictive "sandbox.disableTypeChecking" settings (or turn it completely off with `false`) ParseError: Parse error in /home/nicojs/stryker/perf/test/express/examples/ejs/views/footer.html (1:0) Unexpected closing tag "body". It may happen when the tag has already been closed by another tag. For more info see https://www.w3.org/TR/html5/syntax.html#closing-elements-that-have-implied-end-tags
    at ngHtmlParser (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/instrumenter/dist/src/parsers/html-parser.js:51:15)
    at async Object.parse (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/instrumenter/dist/src/parsers/html-parser.js:33:18)
    at async DisableTypeCheckingPreprocessor.disableTypeChecking [as impl] (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/instrumenter/dist/src/disable-type-checking.js:16:17)
    at async /home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/sandbox/disable-type-checking-preprocessor.js:30:32
    at async Promise.all (index 36)
    at async DisableTypeCheckingPreprocessor.preprocess (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/sandbox/disable-type-checking-preprocessor.js:27:30)
    at async MultiPreprocessor.preprocess (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/sandbox/multi-preprocessor.js:14:25)
    at async MutantInstrumenterExecutor.execute (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/process/2-MutantInstrumenterExecutor.js:24:23)
    at async Stryker.runMutationTest (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/Stryker.js:32:44)
18:36:16 (20065) WARN DisableTypeCheckingPreprocessor (disable "warnings.preprocessorErrors" to ignore this warning

Maybe a better default would be {test,src,lib}/**/*.{js,ts,jsx,tsx,html,vue}?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've implemented this.

}
}
},
Expand Down Expand Up @@ -235,6 +224,11 @@
"description": "decide whether or not to log warnings when additional stryker options are configured",
"type": "boolean",
"default": true
},
"preprocessorErrors": {
"description": "decide whether or not to log warnings when a preprocessor warning occurs. For example, when the disabling of type errors fails.",
"type": "boolean",
"default": true
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/config/OptionsValidator.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import os = require('os');

import Ajv = require('ajv');
import { StrykerOptions, strykerCoreSchema, WarningOptions } from '@stryker-mutator/api/core';
import { StrykerOptions, strykerCoreSchema } from '@stryker-mutator/api/core';
import { tokens, commonTokens } from '@stryker-mutator/api/plugin';
import { noopLogger, propertyPath, deepFreeze } from '@stryker-mutator/util';
import { noopLogger, propertyPath, deepFreeze, PropertyPathBuilder } from '@stryker-mutator/util';
import { Logger } from '@stryker-mutator/api/logging';
import type { JSONSchema7 } from 'json-schema';

Expand Down Expand Up @@ -129,7 +129,7 @@ export function markUnknownOptions(options: StrykerOptions, schema: JSONSchema7,
log.warn(`Unknown stryker config option "${unknownPropertyName}".`);
});

const p = `${propertyPath<StrykerOptions>('warnings')}.${propertyPath<WarningOptions>('unknownOptions')}`;
const p = PropertyPathBuilder.create<StrykerOptions>().prop('warnings').prop('unknownOptions').build();

log.warn(`Possible causes:
* Is it a typo on your end?
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/di/coreTokens.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export const checkerPool = 'checkerPool';
export const checkerFactory = 'checkerFactory';
export const checkerConcurrencyTokens = 'checkerConcurrencyTokens';
export const disableTypeCheckingHelper = 'disableTypeCheckingHelper';
export const execa = 'execa';
export const cliOptions = 'cliOptions';
export const configReader = 'configReader';
Expand Down
10 changes: 6 additions & 4 deletions packages/core/src/sandbox/create-preprocessor.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
import { tokens, Injector, commonTokens, PluginContext } from '@stryker-mutator/api/plugin';

import { disableTypeChecking } from '@stryker-mutator/instrumenter';

import { coreTokens } from '../di';

import { TSConfigPreprocessor } from './ts-config-preprocessor';
import { FileHeaderPreprocessor } from './file-header-preprocessor';
import { FilePreprocessor } from './file-preprocessor';
import { MultiPreprocessor } from './multi-preprocessor';
import { StripCommentsPreprocessor } from './strip-comments-preprocessor';
import { DisableTypeCheckingPreprocessor } from './disable-type-checking-preprocessor';

createPreprocessor.inject = tokens(commonTokens.injector);
export function createPreprocessor(injector: Injector<PluginContext>): FilePreprocessor {
return new MultiPreprocessor([
injector.injectClass(StripCommentsPreprocessor),
injector.provideValue(coreTokens.disableTypeCheckingHelper, disableTypeChecking).injectClass(DisableTypeCheckingPreprocessor),
injector.injectClass(TSConfigPreprocessor),
injector.injectClass(FileHeaderPreprocessor),
]);
}
63 changes: 63 additions & 0 deletions packages/core/src/sandbox/disable-type-checking-preprocessor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import path = require('path');

import minimatch = require('minimatch');
import { commonTokens, tokens } from '@stryker-mutator/api/plugin';
import { File, StrykerOptions } from '@stryker-mutator/api/core';
import type { disableTypeChecking } from '@stryker-mutator/instrumenter';
import { Logger } from '@stryker-mutator/api/logging';
import { PropertyPathBuilder } from '@stryker-mutator/util';

import { coreTokens } from '../di';
import { isWarningEnabled } from '../utils/objectUtils';

import { FilePreprocessor } from './file-preprocessor';

/**
* Disabled type checking by inserting `@ts-nocheck` atop TS/JS files and removing other @ts-xxx directives from comments:
* @see https://github.com/stryker-mutator/stryker/issues/2438
*/
export class DisableTypeCheckingPreprocessor implements FilePreprocessor {
public static readonly inject = tokens(commonTokens.logger, commonTokens.options, coreTokens.disableTypeCheckingHelper);
constructor(private readonly log: Logger, private readonly options: StrykerOptions, private readonly impl: typeof disableTypeChecking) {}

public async preprocess(files: File[]): Promise<File[]> {
if (this.options.sandbox.disableTypeChecking === false) {
return files;
} else {
const pattern = path.resolve(this.options.sandbox.disableTypeChecking);
let warningLogged = false;
const outFiles = await Promise.all(
files.map(async (file) => {
if (minimatch(path.resolve(file.name), pattern)) {
try {
return await this.impl(file, { plugins: this.options.mutator.plugins });
} catch (err) {
if (isWarningEnabled('preprocessorErrors', this.options.warnings)) {
warningLogged = true;
this.log.warn(
`Unable to disable type checking for file "${
file.name
}". Shouldn't type checking be disabled for this file? Consider configuring a more restrictive "${PropertyPathBuilder.create<
StrykerOptions
>()
.prop('sandbox')
.prop('disableTypeChecking')}" settings (or turn it completely off with \`false\`)`,
err
);
}
return file;
}
} else {
return file;
}
})
);
if (warningLogged) {
this.log.warn(
`(disable "${PropertyPathBuilder.create<StrykerOptions>().prop('warnings').prop('preprocessorErrors')}" to ignore this warning`
);
}
return outFiles;
}
}
}
27 changes: 0 additions & 27 deletions packages/core/src/sandbox/file-header-preprocessor.ts

This file was deleted.

33 changes: 0 additions & 33 deletions packages/core/src/sandbox/strip-comments-preprocessor.ts

This file was deleted.

4 changes: 2 additions & 2 deletions packages/core/test/unit/sandbox/create-preprocessor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ describe(createPreprocessor.name, () => {

it('should add a header to .ts files', async () => {
const output = await sut.preprocess([new File(path.resolve('app.ts'), 'foo.bar()')]);
assertions.expectTextFilesEqual(output, [new File(path.resolve('app.ts'), '/* eslint-disable */\n// @ts-nocheck\nfoo.bar()')]);
assertions.expectTextFilesEqual(output, [new File(path.resolve('app.ts'), '// @ts-nocheck\nfoo.bar()')]);
});

it('should strip // @ts-expect-error (see https://github.com/stryker-mutator/stryker/issues/2364)', async () => {
const output = await sut.preprocess([new File(path.resolve('app.ts'), '// @ts-expect-error\nfoo.bar()')]);
assertions.expectTextFilesEqual(output, [new File(path.resolve('app.ts'), '/* eslint-disable */\n// @ts-nocheck\n\nfoo.bar()')]);
assertions.expectTextFilesEqual(output, [new File(path.resolve('app.ts'), '// @ts-nocheck\n// \nfoo.bar()')]);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import path = require('path');

import { File } from '@stryker-mutator/api/core';
import { assertions, testInjector } from '@stryker-mutator/test-helpers';
import sinon = require('sinon');

import { expect } from 'chai';

import { coreTokens } from '../../../src/di';
import { DisableTypeCheckingPreprocessor } from '../../../src/sandbox/disable-type-checking-preprocessor';

describe.only(DisableTypeCheckingPreprocessor.name, () => {
let sut: DisableTypeCheckingPreprocessor;
let disableTypeCheckingStub: sinon.SinonStub;

beforeEach(() => {
disableTypeCheckingStub = sinon.stub();
sut = testInjector.injector
.provideValue(coreTokens.disableTypeCheckingHelper, disableTypeCheckingStub)
.injectClass(DisableTypeCheckingPreprocessor);
});

['.ts', '.tsx', '.js', '.jsx', '.html', '.vue'].forEach((extension) => {
it(`should disable type checking a ${extension} file by default`, async () => {
const fileName = `src/app${extension}`;
const expectedFile = new File(path.resolve(fileName), 'output');
const inputFile = new File(path.resolve(fileName), 'input');
const input = [inputFile];
disableTypeCheckingStub.resolves(expectedFile);
const output = await sut.preprocess(input);
expect(disableTypeCheckingStub).calledWith(inputFile);
assertions.expectTextFilesEqual(output, [expectedFile]);
});
});

it('should be able to override "sandbox.disableTypeChecking" glob pattern', async () => {
testInjector.options.sandbox.disableTypeChecking = 'src/**/*.ts';
const expectedFile = new File(path.resolve('src/app.ts'), 'output');
const input = [new File(path.resolve('src/app.ts'), 'input')];
disableTypeCheckingStub.resolves(expectedFile);
const output = await sut.preprocess(input);
assertions.expectTextFilesEqual(output, [expectedFile]);
});

it('should not disable type checking when the "sandbox.disableTypeChecking" glob pattern does not match', async () => {
testInjector.options.sandbox.disableTypeChecking = 'src/**/*.ts';
const expectedFiles = [new File(path.resolve('test/app.spec.ts'), 'input')];
disableTypeCheckingStub.resolves(new File('', 'not expected'));
const output = await sut.preprocess(expectedFiles);
assertions.expectTextFilesEqual(output, expectedFiles);
});

it('should not disable type checking if "sandbox.disableTypeChecking" is set to `false`', async () => {
const input = [
new File(path.resolve('src/app.ts'), '// @ts-expect-error\nfoo.bar();'),
new File(path.resolve('test/app.spec.ts'), '/* @ts-expect-error */\nfoo.bar();'),
new File(path.resolve('testResources/project/app.ts'), '/* @ts-expect-error */\nfoo.bar();'),
];
testInjector.options.sandbox.disableTypeChecking = false;
const output = await sut.preprocess(input);
assertions.expectTextFilesEqual(output, input);
});

it('should not crash on error, instead log a warning', async () => {
const input = [new File('src/app.ts', 'input')];
const expectedError = new Error('Expected error for testing');
disableTypeCheckingStub.rejects(expectedError);
const output = await sut.preprocess(input);
expect(testInjector.logger.warn).calledWithExactly(
'Unable to disable type checking for file "src/app.ts". Shouldn\'t type checking be disabled for this file? Consider configuring a more restrictive "sandbox.disableTypeChecking" settings (or turn it completely off with `false`)',
expectedError
);
expect(testInjector.logger.warn).calledWithExactly('(disable "warnings.preprocessorErrors" to ignore this warning');
assertions.expectTextFilesEqual(output, input);
});
});
Loading