Implement hot reload for mocha test runner

[nicojs]

As discussed with @Lakitna in #1514

Hot reload. Test runners won't have to clear their test suite between tests (there is a caveat here regarding static code, see hurdles).

We want to implement this feature in the mocha test runner, which supports hot reload since ~7.2 (see mochajs/mocha#4234).

I want to make it optional, since it might break some test suites that don't cleanup nicely between test runs.

For example:

describe('foo', () => {
  const myStub = sinon.stub(); // Bad practice, will be used across runs
  it('should work', () => {
    myStub();
    sinon.assert.calledOnce(spy)
  });
});

This test will succeed the first time, but fail the second time. This would mark all mutants as killed.

Suggestions:

1. "hotReloadEnabled": true
false is default.`
2. "hotReload": true
false is default`
3. "reloadMode": "hot"
"reload" by default. Leaves room for adding other modes in the future (for example "restart", which restarts the test runner process between each run)

[Lakitna]

Nice to have a way to track this :)

I want to make it optional, since it might break some test suites that don't cleanup nicely between test runs.

I get where you're coming from. The first priority is that it must work. The second is speed. But on the other hand, hot reload might be forgotten about when it's not on by default.

Is it possible to warn about disabling hot reload when these bad practice errors occur? That way you can turn it on by default, without the annoyance for first-time users.

Thinking about it, one way you might identify this is basically this linting rule https://github.com/lo1tuma/eslint-plugin-mocha/blob/master/docs/rules/no-setup-in-describe.md. Though this will not always be an indication for not being able to use hot reload as it will also flag [...].forEach((a) => it(a, function() {...}); which should be fine.

Another way to identify these bad practices is another tool I use. Choma is a little Mocha plugin that randomizes your tests before execution. It will catch most of these bad practices and could potentially be applied to the initial test run. It won't catch these things when used on a single test and since it's random it won't catch it every time either.

3. "reloadMode": "hot"
   "reload" by default. Leaves room for adding other modes in the future (for example "restart", which restarts the test runner process between each run)

If you expect more reload things to be added, I'd go for this one. If you don't expect others I would use option 2.

[nicojs]

I've been thinking about ways to "discover" if you rely on a bad practice to run your tests. I don't want to be adding checks that rely on a specific test runner or framework. I think maybe we can add a stryker diagnose mode (instead of run). Diagnose could do a couple of things:

• Instrument the code and do a dry run, to see if a dry run can be performed
  • If not, try with in-place mutation (Allow to mutate files in place #2163) and try again
• Run your tests twice with hot reload and see if the outcome differs.
  • If output differs, turn off hot reload and try again
• Run your tests in parallel and see if that doesn't affect the test outcome (for example, running tests on a database might impact the outcome).

We could also add some clever metrics to see if the CPU/IO isn't strained too much and suggest a --concurrency, for example.

I'm curious to know your take in this.

I think I would like to start with hot reload off by default, just to see how it functions in practice. We can always change the default later on 🤷‍♂️

[Lakitna]

What you're describing is basically an auto-configurator.

I think it makes sense, as it would make it really straightforward to setup Stryker in the most efficient way possible. Right now that takes some research for non-trivial projects. It would be especially useful if you use it to tell people why settings do not work for them, what the impact is, and what they have to do to fix it. It would basically allow anyone to troubleshoot most things themselves without much effort. Example: Disabling hot reload because it seems some of your tests have side effects that do not get cleaned up properly. This comes with a performance hit. [potentially link to some (third party) docs on how to find side effects]

I imagine doing this directly after stryker init.

I imagine it'll be quite a bit of work to get this done, but it's also something that can be build up over time. Maybe starting with a single configuration thing like reloadMode/hotReload and then building out to other things like concurrency.

I do think it will take a bunch of work to get it working, but I also think it would make the barrier of entry lower. That being said, this is not something that should get focus before 4.x has been somewhat polished.

With this in mind, I really don't mind hot-reload being off by default. And even if this doesn't get built, the default can always be changed as you said.

Additional thoughts

With an auto-configurator as described above Stryker would become more than just mutation testing. Basically you're doing everything you can to detect issues in the tests themselves (in this case; bad practices). Linting can do something in this area, but in this case, we're executing the tests to find of if they are good. We're basically talking about high-level testing your tests without the recursive trap of testing tests.

The recursive trap of testing tests:

1. Code should be tested
2. I test my code
3. Tests are code
4. Code should be tested
5. I test the tests for my code
6. Tests are code
7. Code should be tested
8. I test the tests that are testing the tests for my code
9. ???
10. BrainExplosionException

[nicojs]

We've discussed this in the June 24'th community meetup and decided to not make this behavior configurable. Instead, the test runner plugin itself should decide whether or not to reload the environment (true for static mutants). See #2922 for more information.

[nicojs]

Closed with #3393, but won't be released before StrykerJS v6 because of breaking changes.