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: Fixed travis windows failures related to the FakeStdin used in the unit tests #1403

Merged
merged 3 commits into from
Dec 14, 2018

Conversation

rpl
Copy link
Member

@rpl rpl commented Dec 13, 2018

This PR applies some changes needed to fix the windows-only failures on travis CI jobs which are triggered by the FakeStdin class used in some of the unit tests.

This travis ci windows-only issue started to trigger failures in most of the recently created pull requests, e.g. https://travis-ci.org/mozilla/web-ext/jobs/459221488#L713

By fixing the above (new issue) we can also fix #1386 (which is another test that uses the FakeStdin class and it has been disabled because it was consistently failing on the travis windows workers).

Quite unfortunately, while testing this PR travis has started to fail consistently (on any platform this time) because nsp check wasn't able to reach "api.nodesecurity.io" anymore (e.g. see https://travis-ci.org/rpl/web-ext/jobs/467447950#L2162), and so I had to look into fixing also #1390 as part of this PR (so that I could double-check that the unit tests were now able to pass on all the configured travis workers).

The commit that fixed #1390, replaces nsp check with a small nodejs script which run npm audit --json (using npx to run the last available version of npm also on node 6), identifies the advisories to ignore based on the .nsprc file (as it was supported by nsp) and then make the job to fail if there are any advisories that are not explicitly listed in the nsprc's exceptions.

Fixes #1386
Fixes #1390

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling be57c2a on rpl:fix/fake-stdin-travis-window-worker-issue into b65226a on mozilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling be57c2a on rpl:fix/fake-stdin-travis-window-worker-issue into b65226a on mozilla:master.

@coveralls
Copy link

coveralls commented Dec 13, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7a67354 on rpl:fix/fake-stdin-travis-window-worker-issue into b65226a on mozilla:master.

@Rob--W Rob--W mentioned this pull request Dec 13, 2018
@Rob--W
Copy link
Member

Rob--W commented Dec 13, 2018

The commit that fixed #1390, replaces nsp check with a small nodejs script which run npm audit --json (using npx to run the last available version of npm also on node 6), identifies the advisories to ignore based on the .nsprc file (as it was supported by nsp) and then make the job to fail if there are any advisories that are not explicitly listed in the nsprc's exceptions.

I don't think that we should care about Node.js 6 with npm audit. As long as we run npm audit with the latest npm on at least machine, we're good. Also, please consider adding a comment to the audit script and point to the npm bug that I referenced in #1390 ( https://github.com/npm/npm/issues/20565 ) so that we can remove the script (and new shelljs dependency) if npm audit ever starts supporting nsprc.

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

First two commits look fine; please consider the comment about npm audit that I added above.

@rpl rpl force-pushed the fix/fake-stdin-travis-window-worker-issue branch from be57c2a to 2effc87 Compare December 14, 2018 10:57
@rpl rpl force-pushed the fix/fake-stdin-travis-window-worker-issue branch from 2effc87 to 7a67354 Compare December 14, 2018 11:35
@rpl
Copy link
Member Author

rpl commented Dec 14, 2018

I just updated this PR with the following changes:

  • added the inline comment to script/audit-deps as suggested
  • added nodejs 10 to the travis config (on which npm audit --json is already available)
  • splitted the travis job into stages (so that part of the stages are only going to run on the desidered combination of platform and nodejs version)

and I have also filed #1404, to remove the currently ignored advisories in a follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants