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

fix(JestTestRunner): run jest with --findRelatedTests #1235

Merged
merged 10 commits into from
Nov 29, 2018
2 changes: 2 additions & 0 deletions packages/stryker-api/src/test_runner/RunOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ interface RunOptions {
* It should be loaded right after the test framework but right before any tests can run.
*/
testHooks?: string;

mutatedFileName?: string;
}

export default RunOptions;
13 changes: 10 additions & 3 deletions packages/stryker-jest-runner/src/JestTestRunner.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { getLogger } from 'stryker-api/logging';
import { RunnerOptions, RunResult, TestRunner, RunStatus, TestResult, TestStatus } from 'stryker-api/test_runner';
import { RunnerOptions, RunResult, TestRunner, RunStatus, TestResult, TestStatus, RunOptions } from 'stryker-api/test_runner';
import * as jest from 'jest';
import JestTestAdapterFactory from './jestTestAdapters/JestTestAdapterFactory';

export default class JestTestRunner implements TestRunner {
private readonly log = getLogger(JestTestRunner.name);
private readonly jestConfig: jest.Configuration;
private readonly processEnvRef: NodeJS.ProcessEnv;
private readonly enableFindRelatedTests: boolean;

public constructor(options: RunnerOptions, processEnvRef?: NodeJS.ProcessEnv) {
// Make sure process can be mocked by tests by passing it in the constructor
Expand All @@ -15,19 +16,25 @@ export default class JestTestRunner implements TestRunner {
// Get jest configuration from stryker options and assign it to jestConfig
this.jestConfig = options.strykerOptions.jest.config;

// Get enableFindRelatedTests from stryker jest options or default to true
this.enableFindRelatedTests = options.strykerOptions.jest.enableFindRelatedTests;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can log on debug what we are doing with this setting?

if (this.enableFindRelatedTests === undefined) {
this.enableFindRelatedTests = true;
}

// basePath will be used in future releases of Stryker as a way to define the project root
// Default to process.cwd when basePath is not set for now, should be removed when issue is solved
// https://github.com/stryker-mutator/stryker/issues/650
this.jestConfig.rootDir = options.strykerOptions.basePath || process.cwd();
this.log.debug(`Project root is ${this.jestConfig.rootDir}`);
}

public async run(): Promise<RunResult> {
public async run(options?: RunOptions): Promise<RunResult> {
Copy link
Member

Choose a reason for hiding this comment

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

options here don't need a ?. It is always filled. No need for null/undefined checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I initially did that because tests started failing and I was unsure whether or not running without RunOptions was a case. I've taken a look at the other *TestRunnerSpecs and I've added some RunOptions to the Jest ones. Is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is great. We have RunnerOptions as options when creating a runner and RunOptions as options when calling the run() method. Confusing, I know 🙈

this.setNodeEnv();

const jestTestRunner = JestTestAdapterFactory.getJestTestAdapter();

const { results } = await jestTestRunner.run(this.jestConfig, process.cwd());
const { results } = await jestTestRunner.run(this.jestConfig, process.cwd(), this.enableFindRelatedTests, options && options.mutatedFileName);

// Get the non-empty errorMessages from the jest RunResult, it's safe to cast to Array<string> here because we filter the empty error messages
const errorMessages = results.testResults.map((testSuite: jest.TestResult) => testSuite.failureMessage).filter(errorMessage => (errorMessage)) as string[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@ import { Configuration, runCLI, RunResult } from 'jest';
export default class JestPromiseTestAdapter implements JestTestAdapter {
private readonly log = getLogger(JestPromiseTestAdapter.name);

public run(jestConfig: Configuration, projectRoot: string): Promise<RunResult> {
public run(jestConfig: Configuration, projectRoot: string, enableFindRelatedTests: boolean, mutatedFileName?: string): Promise<RunResult> {
jestConfig.reporters = [];
const config = JSON.stringify(jestConfig);
this.log.trace(`Invoking Jest with config ${config}`);
if (enableFindRelatedTests && mutatedFileName) {
this.log.trace(`Only running tests related to ${mutatedFileName}`);
Copy link
Member

Choose a reason for hiding this comment

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

I like this log message.

}

return runCLI({
...(enableFindRelatedTests && mutatedFileName && { _: [mutatedFileName], findRelatedTests: true}),
config,
runInBand: true,
silent: true
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { RunResult } from 'jest';

export default interface JestTestAdapter {
run(config: object, projectRoot: string): Promise<RunResult>;
run(config: object, projectRoot: string, enableFindRelatedTests: boolean, mutatedFileName?: string): Promise<RunResult>;
Copy link
Member

Choose a reason for hiding this comment

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

Could we leave the name mutatedFileName out of this interface. It is not a clean separation of concerns. The JestTestAdapter shouldn't care about mutants, only about running tests. I think these 2 options can be combined into 1: fileNameUnderTest or something. If it is provided, it can add {findRelatedTests: true, _: [fileNameUnderTest].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. Do you mean to only pass the fileNameUnderTest when running related to a specific file? I.e. when the flag is true and we're testing a mutant?

Copy link
Member

Choose a reason for hiding this comment

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

I.e. when the flag is true and we're testing a mutant?

Yeah that is what I mean. The JestTestAdapter should be an adapter for jest. No more, no less. It shouldn't make decisions.

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ describe('JestPromiseTestAdapter', () => {
});

it('should set reporters to an empty array', async () => {
await jestPromiseTestAdapter.run(jestConfig, projectRoot);
await jestPromiseTestAdapter.run(jestConfig, projectRoot, true);

expect(jestConfig.reporters).to.be.an('array').that.is.empty;
});

it('should call the runCLI method with the correct projectRoot', async () => {
await jestPromiseTestAdapter.run(jestConfig, projectRoot);
await jestPromiseTestAdapter.run(jestConfig, projectRoot, true);

assert(runCLIStub.calledWith({
config: JSON.stringify({ rootDir: projectRoot, reporters: [] }),
Expand All @@ -38,7 +38,7 @@ describe('JestPromiseTestAdapter', () => {
});

it('should call the runCLI method and return the test result', async () => {
const result = await jestPromiseTestAdapter.run(jestConfig, projectRoot);
const result = await jestPromiseTestAdapter.run(jestConfig, projectRoot, true);

expect(result).to.deep.equal({
config: {
nicojs marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -51,7 +51,7 @@ describe('JestPromiseTestAdapter', () => {
});

it('should trace log a message when jest is invoked', async () => {
await jestPromiseTestAdapter.run(jestConfig, projectRoot);
await jestPromiseTestAdapter.run(jestConfig, projectRoot, true);

const expectedResult: any = JSON.parse(JSON.stringify(jestConfig));
expectedResult.reporters = [];
Expand Down
4 changes: 3 additions & 1 deletion packages/stryker-jest-runner/types/jest/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ declare namespace Jest {
config: string;
runInBand: boolean;
silent: boolean;
findRelatedTests?: boolean;
_?: string[];
}

// Taken from https://goo.gl/qHifyP, removed all stuff that we are not using
Expand All @@ -18,7 +20,7 @@ declare namespace Jest {
collectCoverage: boolean;
verbose: boolean;
testResultsProcessor: Maybe<string>;
testEnvironment: string
testEnvironment: string;
}

interface RunResult {
Expand Down
6 changes: 3 additions & 3 deletions packages/stryker/src/Sandbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ export default class Sandbox {
return sandbox.initialize().then(() => sandbox);
}

public run(timeout: number, testHooks: string | undefined): Promise<RunResult> {
return this.testRunner.run({ timeout, testHooks });
public run(timeout: number, testHooks: string | undefined, mutant?: TestableMutant): Promise<RunResult> {
return this.testRunner.run({ timeout, testHooks, ...(mutant && { mutatedFileName: this.fileMap[mutant.fileName]}) });
Copy link
Member

Choose a reason for hiding this comment

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

Could we move the locating of the mutated file to the runMutant call? It is specific to the case of running when targeting a mutant. I feel like the separation of concerns is intermingled now.

}

public dispose(): Promise<void> {
Expand All @@ -58,7 +58,7 @@ export default class Sandbox {
this.log.warn(`Failed find coverage data for this mutant, running all tests. This might have an impact on performance: ${transpiledMutant.mutant.toString()}`);
}
await Promise.all(mutantFiles.map(mutatedFile => this.writeFileInSandbox(mutatedFile)));
const runResult = await this.run(this.calculateTimeout(transpiledMutant.mutant), this.getFilterTestsHooks(transpiledMutant.mutant));
const runResult = await this.run(this.calculateTimeout(transpiledMutant.mutant), this.getFilterTestsHooks(transpiledMutant.mutant), transpiledMutant.mutant);
await this.reset(mutantFiles);
return runResult;
}
Expand Down
8 changes: 6 additions & 2 deletions packages/stryker/test/unit/SandboxSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,17 @@ describe('Sandbox', () => {
expect(testFrameworkStub.filter).to.have.been.calledWith(transpiledMutant.mutant.selectedTests);
});

it('should provide the filter code as testHooks and correct timeout', async () => {
it('should provide the filter code as testHooks, correct timeout and mutatedFileName', async () => {
options.timeoutMS = 1000;
const overheadTimeMS = 42;
const totalTimeSpend = 12;
const sut = await Sandbox.create(options, SANDBOX_INDEX, files, testFrameworkStub, overheadTimeMS, LOGGING_CONTEXT);
await sut.runMutant(transpiledMutant);
const expectedRunOptions = { testHooks: wrapInClosure(testFilterCodeFragment), timeout: totalTimeSpend * options.timeoutFactor + options.timeoutMS + overheadTimeMS };
const expectedRunOptions = {
mutatedFileName: path.resolve('random-folder-3', 'file1'),
testHooks: wrapInClosure(testFilterCodeFragment),
timeout: totalTimeSpend * options.timeoutFactor + options.timeoutMS + overheadTimeMS
};
expect(testRunner.run).calledWith(expectedRunOptions);
});

Expand Down