Error when mutating code that uses any-promise

[frostmar]

Summary

If the code under test uses package any-promise, and registers any promise implementation other than the default, running stryker exits with an error like this:

11:35:55 (26800) DEBUG MochaTestRunner Using files: [
  "C:\\firefly\\repos\\firefly-request-helper\\.stryker-tmp\\sandbox4709835\\test\\requestHelper-test.js"
]
11:35:55 (916) DEBUG ChildProcessProxy Disposing of worker process 26800
11:35:55 (916) DEBUG ChildProcessProxy Kill 26800
11:35:55 (916) ERROR InitialTestExecutor One or more tests resulted in an error:
        Error: any-promise already defined as "global.Promise".  You can only register an implementation before the first  call to require("any-promise") and an implementation cannot be changed
Error: any-promise already defined as "global.Promise".  You can only register an implementation before the first  call to require("any-promise") and an implementation cannot be changed
    at register (C:\myrepo\node_modules\any-promise\loader.js:53:13)
    at Object.<anonymous> (C:\myrepo\node_modules\any-promise\register\bluebird.js:2:23)
    at {snipped}
11:35:55 (916) ERROR StrykerCli an error occurred Error: Something went wrong in the initial test run
    at InitialTestExecutor.validateResult (C:\firefly\repos\firefly-request-helper\node_modules\stryker\src\process\InitialTestExecutor.js:125:15)
    at ... {snipped}

This is because any-promise only allows one promise implementation to be registered, and once that's done it cannot be changed. In our case the code under test contains require('any-promise/register/bluebird')

From an initial look, stryker itself doesn't use any-promise directly, but the ChildProcessProxyWorker uses mz/fs which depends on any-promise. mz doesn't require any specific promise implementation, but causes any-promise to register the default native promises, and does so first (before the code under test gets required).

---

We'd love to use stryker on code that uses any-promise, would either of these solutions be acceptable?

1. Refactor the child worker code so it doesn't use any dependencies that require any-promise
   This lets the code under test be the first use of any-promise, and select the promise implementation.
   The npm dependency tree suggests the only dependency in the child worker using any-promise is mz :

   $ npm ls any-promise
   ...
   +-- request-promise-any@1.0.5
   | `-- any-promise@1.3.0
   `-- stryker@0.29.2
     `-- mz@2.7.0
       +-- any-promise@1.3.0  deduped
       `-- thenify-all@1.6.0
         `-- thenify@3.3.0
           `-- any-promise@1.3.0  deduped
   

2. Add a stryker config option to declare the promise implementation desired by the code under test; allowing it to be pre-registered with any-promise
   ChildProcessProxyWorker would be passed the config option, and call require(any-promise/register/${anyPromiseLibrary) so the preferred promise implementation would be registered before requiring any libraries in the stryker child worker which also use any-promise

Stryker config

module.exports = function (config) {
  config.set({
    testRunner: 'mocha',
    mutator: 'javascript',
    transpilers: [],
    reporters: ['html', 'baseline', 'clear-text', 'progress'],
    packageManager: 'npm',
    testFramework: 'mocha',
    coverageAnalysis: 'all',
    mutate: ['lib/*.js'],
    maxConcurrentTestRunners: 1,
    logLevel: 'trace'
  })
}

Stryker environment

$ npm ls | grep stryker
+-- stryker@0.29.2
+-- stryker-api@0.21.1
+-- stryker-baseline-reporter@1.0.3
+-- stryker-html-reporter@0.16.2
+-- stryker-javascript-mutator@0.10.0
npm+-- stryker-mocha-framework@0.12.2
ERR!`-- stryker-mocha-runner@0.14.2
 peer dep missing: ajv@^6.0.0, required by ajv-keywords@3.2.0

note sure what the missing peer-dep is about?

mocha@5.2.0

Your Environment

software	version(s)
node	v8.9.3
npm	6.4.0
Operating System	Windows

Add stryker.log

stryker.log

[frostmar]

The code we're testing isn't in a public repo, but if it'd help I can make a cut-down example which reproduces the problem.

[simondel]

@frostmar Thanks for letting us know! As a test, could you try to configure the command testrunner?
https://www.npmjs.com/package/stryker#test-runner

[frostmar]

Yup, swapping the config to testRunner: 'command' let stryker run successfully (after I disabled some pretest and posttest npm scripts which do linting and coverage enforcement for us).

I guess the stryker ChildProcessProxyWorker and code under test are then in separate processes?

[simondel]

The command test runners runs the command in a separate process if I recall correctly. Other test runners run directly inside the worker (which is already a child process).

So it looks like the dependency inside of stryker is not the issue, but instead the fact that your tests are executed multiple times in the same process.

Is it possible for you to perform a cleanup of the promise functionality instead of relying on the closing of the process to clean it up?

[frostmar]

I believe it's a dependency in the stryker worker that leads to the problem, rather than running tests repeatedly.
Hacking require('any-promise/register/bluebird') in as the first line of stryker/src/child-proxy/ChildProcessProxyWorker lets stryker run OK.

The flow of control is something like:

• stryker spawns ChildProcessProxyWorker.js in a child process
• ChildProcessProxyWorker.js requires "../PluginLoader"
• PluginLoader.js requires "mz/fs"
• mz.fs.js runs var Promise = require('any-promise')
  which would return the previously registered promise, but since noone has registered a promise implementation with any-promise yet, causes native promises to be registered. This is before any code under test has been require'd into the process.
• later, our code under test is loaded, which contains require('any-promise/register/bluebird') and triggers the error. The code being tested normally gets loaded very early, and gets to choose the promise implementation

any-promise should be OK running the same tests multiple times, as re-registering the same implementation is harmless.
https://www.npmjs.com/package/any-promise#usage-with-registration

It is safe to call register multiple times, but it must always be with the same implementation.

[nicojs]

@frostmar thanks for spotting this issue and all your hard work!

It is difficult to guarantee a clean environment. One would think you can use fs/mz without side effects... too bad 😓

The most pragmatic thing we can do is just remove mz in it's entirely from our dependencies. @simondel what do you think?

[simondel]

I think we can remove fs/mz, but I believe you had some convincing arguments on why we wanted to use this package instead of the regular fs package. Do you still recall them @nicojs?

[nicojs]

Most definitely. We mostly (only?) use it to get a promisyfied api around fs. I would like to start using fs.promises, or util.promisify, but those are node 10 and node 8 only (respectively).

The main issue with mz/fs is that it supports any-promise, which causes the side effects. We could move to a wrapper that supports native es promises, like https://www.npmjs.com/package/promisified-fs (that one has no type definitions see cnlon/promisified-fs#1).

[simondel]

I like the fact of keeping promises. If that's not possible, we could always promisify the methods we use ourselves.

[nicojs]

I'm trying to remove 'mz' entirely, but I'm running into some problems.

TLDR;

I thing we should introduce a utility library to the packages.

Long version:

• 'mz' is used in all plugins.
• there is no good alternative IMHO. You can find a lot of them, but all have issues:
  • They are not maintained
  • and/or they don't have typescript definition files (this is not trivial, it is a lot of work to write/maintain the definition file of the entire fs api).
  • and/or they have dependencies themselves, which they shouldn't have IMHO.

That's why I've decided to create our own promisified api. The problem is that it should be centralized an a shared dependency. I don't want to pollute the stryker-api with it. That's why I think we should introduce a dependency 'stryker-lib' or 'stryker-util'. Can be useful for other code sharing efforts as well. It can play a role #667 as well.

@simondel do you agree?

[simondel]

@nicojs Sounds good!