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

RobertBroersma · 2018-11-08T10:50:53Z

Pass sandbox fileName to the test runner.

It's probably not the best code, but it works. It speeds up my stryker runs from around 2h45m to around 45m (70% faster!).

It would be great if someone could assist me in writing tests and giving me some pointers for cleaning up/refactoring my code, as the project is quite a lot to take in for me atm.

Cheers, let me know what you think!

PS. As I'm not entirely clear on how to run the project locally on my own code, I had to replace importModule(name); with importModule(__dirname + '../../../' + name); in PluginLoader.ts in order for stryker to be able to find its plugins. Not sure why.

Fixes #1226

Pass sandbox fileName to the test runner.

bartekleon · 2018-11-08T14:37:16Z

@RobertBroersma in packages/stryker-jest-runner/src/jestTestAdapters/JestPromiseTestAdapter.ts in line 22 there is unnecessary comma. You have to remove it in order to tests can pass.
And remember to use npm test or npm run lint-info before PR so other tests can pass (not blocked by linter :) )

RobertBroersma · 2018-11-09T07:04:32Z

@kmdrGroch You are right!

I had some trouble running tests and checks locally, but I think I got it now. Will push some fixes soon.

nicojs

Awesome to see you found you're way around our repository. I've already taken a look and it looks good. I've got 2 suggestions already.

packages/stryker-api/src/test_runner/RunOptions.ts

packages/stryker-jest-runner/src/jestTestAdapters/JestPromiseTestAdapter.ts

Also add to Sandbox test

RobertBroersma · 2018-11-09T17:59:47Z

@nicojs Thanks for the feedback! I've processed your comments and expanded the test for Sandbox.ts.

Should we consider adding a configuration option for running with or without --findRelatedTests? I don't see any reason why you would run it without the flag, but maybe someone else does.

nicojs · 2018-11-12T07:58:23Z

Should we consider adding a configuration option for running with or without --findRelatedTests? I don't see any reason why you would run it without the flag, but maybe someone else does.

Can you explain to me how the filtering works? If I have a fooTest.js file that tests foo.js, but foo.js requires bar.js and tests it as well implicitly. What would happen if I run --findRelatedTests on bar.js?

I have high doubts that it would work in 100% of the cases, because of the dynamic nature of JavaScript.

RobertBroersma · 2018-11-12T10:02:09Z

Can you explain to me how the filtering works? If I have a fooTest.js file that tests foo.js, but foo.js requires bar.js and tests it as well implicitly. What would happen if I run --findRelatedTests on bar.js?

I have high doubts that it would work in 100% of the cases, because of the dynamic nature of JavaScript.

I've never questioned the working of it tbh, but I've went and looked it up. From a curious stackoverflow user: https://stackoverflow.com/questions/44066996/how-does-jest-findrelatedtests-work-under-the-hood

When running --findRelatedTests path/to/src-code.js, the first thing that happens is Jest creates an instance of an internal package, jest-resolve-dependencies. It's a pretty straightforward class with two methods: resolve and resolveInverse.

findRelatedTests calls resolveInverse on the paths you provided, looking up every source and test file that requires your file, in our example path/to/src-code.js. This lookup relies directly on some Jest configuration, specifically roots and/or rootDir, to help resolve paths.

If the found file is a test file, Jest runs it, simple enough. If the found file is a source file, call it found-file.js, then any test files that import found-file.js and the test files that import any of the source files that themselves import found-file.js will be run.

It's a clever implementation of, as the maintainers put it, a resolver of "transitive inverse dependencies". You can see for yourself in this while loop.

However there have been reports of it not working optimally, which have been recognized by the Jest team, e.g. jestjs/jest#6062

nicojs · 2018-11-12T11:34:39Z

Thanks for figuring this out @RobertBroersma. With all side effects in mind, let's make it configurable. Would you mind making the change?

The default can be enabled in my opinion. That way, it is a nice performance boost out-of-the box. Can we call it jest.enableFindRelatedTests? Or do you have another proposal?

RobertBroersma · 2018-11-12T12:28:53Z

Thanks for figuring this out @RobertBroersma. With all side effects in mind, let's make it configurable. Would you mind making the change?

The default can be enabled in my opinion. That way, it is a nice performance boost out-of-the box. Can we call it jest.enableFindRelatedTests? Or do you have another proposal?

No problem! I agree making it configurable would be good. I'll make the change. jest.enableFindRelatedTests sounds good to me.

RobertBroersma · 2018-11-13T13:04:07Z

@nicojs I've made the flag configurable. Not sure if this is the best way or if and how I should add any tests for it. Let me know what you think!

nicojs

Great, this PR is looking better and better. I've got some small remarks about naming and functionality location.

We should indeed add some tests in "JestPromiseTestAdapterSpec.ts" to see if the correct options are provided. Are you willing to add those as well?

We also shouldn't forget to document this feature in the readme.

nicojs · 2018-11-16T11:02:44Z

packages/stryker-jest-runner/src/JestTestRunner.ts

@@ -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;


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

nicojs · 2018-11-16T11:08:24Z

packages/stryker/src/Sandbox.ts

-  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]}) });


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.

packages/stryker-jest-runner/test/unit/jestTestAdapters/JestPromiseTestAdapterSpec.ts

nicojs · 2018-11-16T11:15:02Z

packages/stryker-jest-runner/src/jestTestAdapters/JestTestAdapter.ts

@@ -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>;


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].

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?

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.

nicojs · 2018-11-16T11:16:24Z

packages/stryker-jest-runner/src/jestTestAdapters/JestPromiseTestAdapter.ts

    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}`);


I like this log message.

RobertBroersma · 2018-11-21T14:54:01Z

I'll be working on your feedback hopefully soon. Got a bit busy at work the past days!

nicojs · 2018-11-27T11:52:35Z

@RobertBroersma just let us know if you need some more help.

RobertBroersma · 2018-11-28T07:52:50Z

@nicojs I'm hoping to find some time this evening. I'll let you know if I run into any issues.

RobertBroersma · 2018-11-28T20:27:28Z

@nicojs I've processed your feedback. I think I understood most of it, let me know what you think!

I've added some unit tests. Was thinking about some integration tests, but couldn't quite figure them out yet. You think we should add some?

nicojs · 2018-11-29T08:24:14Z

packages/stryker-jest-runner/src/JestTestRunner.ts

    // 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> {


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

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?

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 🙈

nicojs · 2018-11-29T08:25:17Z

packages/stryker-jest-runner/test/unit/jestTestAdapters/JestPromiseTestAdapterSpec.ts

@@ -37,6 +38,18 @@ describe('JestPromiseTestAdapter', () => {
    }, [projectRoot]));
  });

+  it('should call the runCLI method with the --findRelatedTests flag', async () => {


Great test!

nicojs · 2018-11-29T11:02:34Z

Thanks for the quick response! I first want to merge #1260. In there we have an integration test that tests the score of an actual stryker run with the jest runner. After that, I'll merge master in this branch and see if it works locally as expected (also will keep an eye on performance to see it improved).

After that, I'll merge. Should still be today.

Awesome job 👍

nicojs · 2018-11-29T11:33:03Z

Worked great, although no performance improvement for the integration test (takes 1 minute and 1 second for both cases locally on my machine).

Will merge when green 👍

RobertBroersma · 2018-11-29T14:28:33Z

Thanks @nicojs !

fix(JestTestRunner): run jest with --findRelatedTests

202f7d8

Pass sandbox fileName to the test runner.

ghost added the 🔎 Needs review label Nov 8, 2018

fix(JestTestRunner): set debug log to trace log

eb97511

RobertBroersma added 2 commits November 8, 2018 16:25

style: remove unnecessary comma and whitespace

d8a0f6c

test: reset log to original

604ce6e

RobertBroersma force-pushed the jest-findrelatedtests branch from 2c456da to 604ce6e Compare November 8, 2018 15:42

nicojs requested changes Nov 9, 2018

View reviewed changes

packages/stryker-api/src/test_runner/RunOptions.ts Outdated Show resolved Hide resolved

packages/stryker-jest-runner/src/jestTestAdapters/JestPromiseTestAdapter.ts Outdated Show resolved Hide resolved

fix: change ambiguous fileName to mutatedFileName

87bb365

Also add to Sandbox test

feat(jest-runner): make usage of findRelatedTests configurable

8e64e42

RobertBroersma mentioned this pull request Nov 13, 2018

Running Jest with --findRelatedTests #1226

Closed

nicojs requested changes Nov 16, 2018

View reviewed changes

RobertBroersma added 2 commits November 28, 2018 20:52

fix(jest-runner): separation of concerns, add some unit tests

bff462e

test(sandbox): add mutatedFileName to test cases

1d9e7f6

nicojs reviewed Nov 29, 2018

View reviewed changes

test(jest-runner): add runoptions to testrunner parameters

1de852f

nicojs approved these changes Nov 29, 2018

View reviewed changes

Merge branch 'master' into jest-findrelatedtests

6070dd7

ghost assigned nicojs Nov 29, 2018

nicojs merged commit 5e0790e into stryker-mutator:master Nov 29, 2018

ghost removed the 🔎 Needs review label Nov 29, 2018

simondel mentioned this pull request Feb 13, 2019

Feature request - Add coverage analysis for Jest #965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RobertBroersma commented Nov 8, 2018 •

edited by nicojs

Loading

bartekleon commented Nov 8, 2018

RobertBroersma commented Nov 9, 2018

nicojs left a comment

RobertBroersma commented Nov 9, 2018

nicojs commented Nov 12, 2018 •

edited

Loading

RobertBroersma commented Nov 12, 2018

nicojs commented Nov 12, 2018

RobertBroersma commented Nov 12, 2018

RobertBroersma commented Nov 13, 2018

nicojs left a comment

nicojs Nov 16, 2018

nicojs Nov 16, 2018

nicojs Nov 16, 2018

RobertBroersma Nov 21, 2018

nicojs Nov 23, 2018

nicojs Nov 16, 2018

RobertBroersma commented Nov 21, 2018

nicojs commented Nov 27, 2018

RobertBroersma commented Nov 28, 2018

RobertBroersma commented Nov 28, 2018

nicojs Nov 29, 2018

RobertBroersma Nov 29, 2018

nicojs Nov 29, 2018

nicojs Nov 29, 2018

nicojs commented Nov 29, 2018

nicojs commented Nov 29, 2018

RobertBroersma commented Nov 29, 2018

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

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

Conversation

RobertBroersma commented Nov 8, 2018 • edited by nicojs Loading

bartekleon commented Nov 8, 2018

RobertBroersma commented Nov 9, 2018

nicojs left a comment

Choose a reason for hiding this comment

RobertBroersma commented Nov 9, 2018

nicojs commented Nov 12, 2018 • edited Loading

RobertBroersma commented Nov 12, 2018

nicojs commented Nov 12, 2018

RobertBroersma commented Nov 12, 2018

RobertBroersma commented Nov 13, 2018

nicojs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertBroersma commented Nov 21, 2018

nicojs commented Nov 27, 2018

RobertBroersma commented Nov 28, 2018

RobertBroersma commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs commented Nov 29, 2018

nicojs commented Nov 29, 2018

RobertBroersma commented Nov 29, 2018

RobertBroersma commented Nov 8, 2018 •

edited by nicojs

Loading

nicojs commented Nov 12, 2018 •

edited

Loading