Plugin mochify-istanbul fails to provide coverage if --url is specified

[rafaelsortoxo]

Hi, good day.

I have noticed that the command line gets mixed up and the plugin mochify-istanbul fails to provide the correct coverage if we specify the --url for the file to use in the test cases.

I have created one repo that explains the problem mochify-istanbul-plugin-issue

If we execute the test script normally, mochify-istanbul correctly reports the coverage and notices that only 1 function is being evaluated.

~/SD_Card/mochify-issue$ npm run test
> mochify-issue@1.0.0 test /media/rafael/SD_Card/mochify-issue
> npx mochify --plugin [ mochify-istanbul --exclude **/*.spec.js --dir coverage --report text --report html ] *.spec.js

# chromium:

  index
    ✓ Should return the sum of 2 numbers

  1 passing (6ms)

----------------|----------|----------|----------|----------|----------------|
File            |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------------|----------|----------|----------|----------|----------------|
 mochify-issue/ |       80 |      100 |       50 |       80 |                |
  index.js      |       80 |      100 |       50 |       80 |              2 |
----------------|----------|----------|----------|----------|----------------|
All files       |       80 |      100 |       50 |       80 |                |
----------------|----------|----------|----------|----------|----------------|

If we then create a basic html file like the one included in the fixtures and specify the --url, even though the test continues to work, mochify-istanbul provides the report before waiting for the tests to actually run. There seems to be an out of order execution of options or something similar.

~/SD_Card/mochify-issue$ npm run test -- --url file:/tmp/index.html

> mochify-issue@1.0.0 test /media/rafael/SD_Card/mochify-issue
> npx mochify --plugin [ mochify-istanbul --exclude **/*.spec.js --dir coverage --report text --report html ] *.spec.js "--url" "file:/tmp/index.html"

----------|----------|----------|----------|----------|----------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
----------|----------|----------|----------|----------|----------------|
All files |      100 |      100 |      100 |      100 |                |
----------|----------|----------|----------|----------|----------------|

# chromium:

  index
    ✓ Should return the sum of 2 numbers

  1 passing (5ms)

We are having some issues with the default fixture location when the test is executed as part of CI/CD build, because the branch being used has some characters that are not recognized by puppeter thus it is reporting that the URL File is not Found. Changing the url with --url works, but now the code coverage is broken.

Thanks in advance for your support to have a look at this issue.

[m90]

I just had a quick look into this and @rafaelsortoxo's assertion about the order of execution seems to be spot on: when using no --url parameter withServer: https://github.com/mantoni/mochify.js/blob/3ef326f44786d7c36db7062f2b71af38eb2ca4c4/lib/mochify.js#L27-L46 is synchronous, whereas it will be asynchronous when passing --url. This makes mochify register the plugins that are passed additionally either before or after registering the chromium one here: https://github.com/mantoni/mochify.js/blob/3ef326f44786d7c36db7062f2b71af38eb2ca4c4/lib/mochify.js#L177-L179

In turn, this means that mochify-istanbul will either get to see the full bundle (which it can make no sense of, thus printing no coverage), or nicely prepared data.

I don't have enough browserify-fu to know if this is expected to be done like this, or if it's a bug in mochify that should always register the passed plugins last but doesn't (which would sound kind of reasonable to me). Any input on this @mantoni ?

EDIT: I also can confirm that hackily moving the registration of plugins into the withServer callback does fix the issue in the example repo and will make it work correctly in both cases.

[mantoni]

@m90 Good analysis. This sounds like a bug and should be fixed in Mochify. Can you put your hack into a PR? EDIT Thanks, you're quick 😄

[rafaelsortoxo]

Wow, @m90 @mantoni thank you for looking at this issue and working to get it fixed so quickly. I wasn't expecting to get it moving so fast. I really appreciate it :+1:

[m90]

@rafaelsortoxo I just released the fix to npm as mochify@5.8.1 and am able to successfully use it in your example repo:

➜  mochify-istanbul-plugin-issue git:(master) ✗ cat package.json | grep "\"mochify\""
    "mochify": "^5.8.1",
➜  mochify-istanbul-plugin-issue git:(master) ✗ npm t

> mochify-issue@1.0.0 test /home/frederik/projects/mochify-istanbul-plugin-issue
> mochify --plugin [ mochify-istanbul --exclude **/*.spec.js --dir coverage --report text --report html] *.spec.js --url=file:/tmp/index.html

# chromium:

  index
    ✓ Should return the sum of 2 numbers

  1 passing (6ms)

--------------------------------|----------|----------|----------|----------|----------------|
File                            |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
--------------------------------|----------|----------|----------|----------|----------------|
 mochify-istanbul-plugin-issue/ |       80 |      100 |       50 |       80 |                |
  index.js                      |       80 |      100 |       50 |       80 |              2 |
--------------------------------|----------|----------|----------|----------|----------------|
All files                       |       80 |      100 |       50 |       80 |                |
--------------------------------|----------|----------|----------|----------|----------------|

➜  mochify-istanbul-plugin-issue git:(master) ✗ nano package.json 
➜  mochify-istanbul-plugin-issue git:(master) ✗ npm t

> mochify-issue@1.0.0 test /home/frederik/projects/mochify-istanbul-plugin-issue
> mochify --plugin [ mochify-istanbul --exclude **/*.spec.js --dir coverage --report text --report html] *.spec.js

# chromium:

  index
    ✓ Should return the sum of 2 numbers

  1 passing (6ms)

--------------------------------|----------|----------|----------|----------|----------------|
File                            |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
--------------------------------|----------|----------|----------|----------|----------------|
 mochify-istanbul-plugin-issue/ |       80 |      100 |       50 |       80 |                |
  index.js                      |       80 |      100 |       50 |       80 |              2 |
--------------------------------|----------|----------|----------|----------|----------------|
All files                       |       80 |      100 |       50 |       80 |                |
--------------------------------|----------|----------|----------|----------|----------------|

Could you let us know if it works in your actual setup as well?

You're most welcome, and thanks a lot for the effort of creating a repo for reproducing the issue, that helped a lot.

[rafaelsortoxo]

Hi, thank you for releasing the patch! It is working on our Jenkins environment again!

This was very quick and to the point :smile: