Fix build and test bugs in #1498 and #1479 #1504

fatso83 · 2017-08-01T10:22:23Z

Purpose

Should fix the errors we are seeing in #1498, introduced by a proxyquire config bug, as well as a case of a missing file causing trouble for the builds in #1479.

Background (Problem in detail) - optional

See the discussion in #1498 and #1479.

How to verify - mandatory

Check out this branch
npm install
Set the sauce lab credentials (or run selenium-standalone locally to test with local browsers)
npm test
npm run test-cloud

Those steps cover #1498.

To test that #1479 failed before, but not after, do this for on master and on this branch:

rm -r pkg
npm test

fatso83 · 2017-08-01T10:23:56Z

The commit messages should also have quite a bit of info on the details.

fatso83 · 2017-08-01T10:44:20Z

P.S. One thing I am not totally not sure about, is where to put the build step. Maybe you have some input on that?

Right now build is called from test, prepublishOnly and the Travis script for Node v6. I was pondering about stuffing it in the test-webworker script, but that line was already far too long.

fearphage

I think we can make better usage of npm's lifetime events. Also I have questions about how run-them-all works and the merits of it.

fearphage · 2017-08-01T13:02:01Z

test/call-test.js

        it("does not throw when the call stack is empty", function (done) {
+            if (!supportPromise) { return this.skip(); }


You don't need to return here. To my knowledge, this.skip() throws a special error (SkipError?) that mocha is expecting so execution should stop immediately. Then you can remove the eslint toggle.

MINOR: You can inline the conditional since it's only used/checked once.

fearphage · 2017-08-01T13:04:26Z

test/sandbox-test.js

@@ -167,6 +169,7 @@ describe("sinonSandbox", function () {

        // eslint-disable-next-line mocha/no-identical-title


Why do they have identical titles? That's weird. I realize it isn't your change though.

I saw that too - I couldn't tell immediately, and decided not to tackle it now.

fearphage · 2017-08-01T13:06:23Z

package.json

    "test-coverage": "mochify --recursive -R dot --grep WebWorker --invert --plugin [ mochify-istanbul --exclude '**/+(test|node_modules)/**/*' --report text --report lcovonly --dir ./coverage ] test/",
    "test-cloud": "npm run test-headless -- --wd",
    "test-webworker": "browserify --no-commondir --full-paths -p [ mocaccino -R spec --color ] test/webworker/webworker-support-assessment.js | phantomic --web-security false",
-    "test": "npm run test-node && npm run test-headless && npm run test-webworker",
+    "test": "run-p test-node test-headless && run-s build test-webworker",


Is the output corrupted when running in parallel?

Could absolutely have been an issue, but the startup time of the phantom stuff actually makes it sequential by chance. You are probably right that it doesn't make sense to save 1.5 seconds in risk of having garbled output. I usually parellize tasks when having interactive/watched builds, where I don't really care all that much.

fearphage · 2017-08-01T13:06:57Z

package.json

    "lint": "eslint .",
    "precommit": "lint-staged",
-    "prepublish": "rimraf pkg && ./build.js",
+    "build": "rimraf pkg && ./build.js",


Use prebuild for the cleanup.

fearphage · 2017-08-01T13:10:17Z

package.json

@@ -16,14 +16,15 @@
  "scripts": {
    "test-node": "mocha --recursive -R dot test/",
    "test-dev": "npm run test-node -- --watch -R min",
-    "test-headless": "mochify --recursive -R dot --grep WebWorker --invert test/",
+    "test-headless": "mochify --recursive -R dot --grep WebWorker --invert --plugin [proxyquire-universal] test/",
    "test-coverage": "mochify --recursive -R dot --grep WebWorker --invert --plugin [ mochify-istanbul --exclude '**/+(test|node_modules)/**/*' --report text --report lcovonly --dir ./coverage ] test/",
    "test-cloud": "npm run test-headless -- --wd",
    "test-webworker": "browserify --no-commondir --full-paths -p [ mocaccino -R spec --color ] test/webworker/webworker-support-assessment.js | phantomic --web-security false",


Instead of changing the externals, why don't you make build a requirement for test-webworker?

pretest-webworker: 'npm run build',

This change would theoretically allow you to revert every instance where you used run-s (travis.yml and package.json).

Wasn't aware that adding pre was a general pattern outside of the standard lifecycle methods (publish, install, ...). You learn something every day 😄 Good improvement!

Having it as a dependency solves the grievances I had otherwise.

fearphage · 2017-08-01T13:12:29Z

package.json

@@ -16,14 +16,15 @@
  "scripts": {
    "test-node": "mocha --recursive -R dot test/",
    "test-dev": "npm run test-node -- --watch -R min",
-    "test-headless": "mochify --recursive -R dot --grep WebWorker --invert test/",
+    "test-headless": "mochify --recursive -R dot --grep WebWorker --invert --plugin [proxyquire-universal] test/",


Spacing is off (different) from all the others below.

mroderick · 2017-08-01T15:21:48Z

This looks great! I don't have anything to add other than what was already pointed out by @fearphage

fatso83 · 2017-08-01T15:25:24Z

Thanks for the review, @fearphage. Good tips on the lifecycle events. Regarding the merits of npm-run-all I just usually end up using it to improve readability / avoid long one-liners.

Typically:
run-s test-node test-headless build test-webworker
vs
npm run test-node && npm run test-headless && npm run build && npm run test-webworker

The parallel stuff is more of a side-note. I can remove it, no problem, but I think it has some (minor) merit, just because we can't work much with readability when being restricted to oneliners.

fearphage · 2017-08-01T15:41:04Z

I'm not against it. Just making sure it's worthwhile. If it simplifies things, then I'm all for it.

Questions:

How does it handle failure?
Can you tell what fails? For instance with npm-s A B C D, can you tell C failed?
Do you still get the exit code as expected?

fatso83 · 2017-08-01T16:11:42Z

@fearphage This is an example run where to scripts simply do exit 0 and exit 100:

$ $(npm bin)/run-s a b

> [email protected] a /home/developer/dev/sinon
> exit 0


> [email protected] b /home/developer/dev/sinon
> exit 100

npm ERR! code ELIFECYCLE
npm ERR! errno 100
npm ERR! [email protected] b: `exit 100`
npm ERR! Exit status 100
npm ERR! 
npm ERR! Failed at the [email protected] b script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/developer/.npm/_logs/2017-08-01T16_06_43_505Z-debug.log
ERROR: "b" exited with 100.

Doing $ echo $? return 1.

So yes, you can tell what failed from the output (Failed at the [email protected] b script) and what the exit status was, but as a whole the exit status is simply 1 from the tool if any of the tasks fail. It will fail on the first task to fail - unless configured to do otherwise (using --continue).

Got a strange error about `require.resolve` not being a function. This has probably to do with the browserify transform only working on the top-most scope (a theory - not vetted). By moving the require statement up this seems to have been fixed.

This commit reintroduces the proxyquire-universal changes that caused some issues (see sinonjs#1498 and commit cbf1128). What is different is that it removes proxyquire from the build step. This should never have been there, and was probably introduced to make proxyquire calls in the tests work with browsers. The problem was that the tests are not using the built sinon file, so the config changes would only be utilized for the built file. By adding the proxquire-universal plugin to the NPM script "test-headless" it would only take place for these tests and the cloud tests (which are reusing the headless config). This commit, along with the previous, fixes sinonjs#1498.

fearphage · 2017-08-01T16:21:47Z

It will fail on the first task to fail - unless configured to do otherwise

Sounds legit to me. 👍

The WebWorker test in PR sinonjs#1479 seems to fail for some mysterious reason with a synchronous XHR requests. The reason was most probably the importScript('../../pkg/sinon.js') call in the webworker file was referencing a build file that wasn't built. By now explicitly building this file as part of the test run this error has now disappeared locally, and probably will disappear on CircleCI as well. fixes the build in sinonjs#1479

fatso83 · 2017-08-01T16:36:01Z

As I have addressed all the issues, I'll merge this.

mroderick · 2017-08-01T16:37:41Z

Well done!

fatso83 · 2017-08-01T17:56:58Z

Btw, this should pave the way for releasing 3.0; not sure if it hinges on #1502

fatso83 requested review from fearphage and mroderick August 1, 2017 10:22

fatso83 assigned mroderick Aug 1, 2017

fatso83 force-pushed the fix-build-and-test-bugs branch 2 times, most recently from 4f32bc4 to 3dee7d3 Compare August 1, 2017 10:40

fatso83 mentioned this pull request Aug 1, 2017

CircleCI Integration #1479

Merged

fearphage requested changes Aug 1, 2017

View reviewed changes

fatso83 added 3 commits August 1, 2017 18:19

Skip Promise tests on unsupported engines

a712acb

fatso83 force-pushed the fix-build-and-test-bugs branch 2 times, most recently from e429322 to 47111df Compare August 1, 2017 16:24

fatso83 force-pushed the fix-build-and-test-bugs branch from 47111df to 521a0f7 Compare August 1, 2017 16:28

fatso83 merged commit 309dcef into sinonjs:master Aug 1, 2017

fatso83 deleted the fix-build-and-test-bugs branch August 1, 2017 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build and test bugs in #1498 and #1479 #1504

Fix build and test bugs in #1498 and #1479 #1504

fatso83 commented Aug 1, 2017

fatso83 commented Aug 1, 2017

fatso83 commented Aug 1, 2017

fearphage left a comment

fearphage Aug 1, 2017

fearphage Aug 1, 2017

fatso83 Aug 1, 2017

fearphage Aug 1, 2017

fatso83 Aug 1, 2017

fearphage Aug 1, 2017

fearphage Aug 1, 2017

fatso83 Aug 1, 2017

fearphage Aug 1, 2017

mroderick commented Aug 1, 2017

fatso83 commented Aug 1, 2017

fearphage commented Aug 1, 2017

fatso83 commented Aug 1, 2017 •

edited

Loading

fearphage commented Aug 1, 2017

fatso83 commented Aug 1, 2017

mroderick commented Aug 1, 2017

fatso83 commented Aug 1, 2017

		it("does not throw when the call stack is empty", function (done) {
		if (!supportPromise) { return this.skip(); }

		@@ -167,6 +169,7 @@ describe("sinonSandbox", function () {

		// eslint-disable-next-line mocha/no-identical-title

Fix build and test bugs in #1498 and #1479 #1504

Fix build and test bugs in #1498 and #1479 #1504

Conversation

fatso83 commented Aug 1, 2017

Purpose

Background (Problem in detail) - optional

How to verify - mandatory

fatso83 commented Aug 1, 2017

fatso83 commented Aug 1, 2017

fearphage 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroderick commented Aug 1, 2017

fatso83 commented Aug 1, 2017

fearphage commented Aug 1, 2017

fatso83 commented Aug 1, 2017 • edited Loading

fearphage commented Aug 1, 2017

fatso83 commented Aug 1, 2017

mroderick commented Aug 1, 2017

fatso83 commented Aug 1, 2017

fatso83 commented Aug 1, 2017 •

edited

Loading