Upgrade to Mochify 5 to run tests in Headless Chrome #1629

mantoni · 2017-12-08T15:49:23Z

This brings the number of tests that are executed up to the same number
as in Chrome and Firefox (+21). These tests where skipped in PhantomJS
due to missing features.

It also allows the WebWorker test script to be simplified removing
mocaccino, phantomic and phantomjs-prebuilt from the dependencies.

Fixes #1405

Open issues:

Fix Travis build.
Fix CircleCI build. While it's green, it doesn't seem to run the browser tests at all.
Figure out why npm run test-coverage hits only 639 tests while it used to run 1138.
Verify this can be used on Windows.

This brings the number of tests that are executed up to the same number as in Chrome and Firefox (+21). These tests where skipped in PhantomJS due to missing features. It also allows the WebWorker test script to be simplified removing mocaccino, phantomic and phantomjs-prebuilt from the dependencies. Fixes #1405

mantoni · 2017-12-08T15:57:45Z

Uuuh, so the main issue with this PR is that Mochify does requires Node 6 at install time because of the Puppeteer download script being written in a Node 4 incompatible way. This is annoying because we're not even running Mochify on Node 4.

Anybody a clever idea how to either exclude Mochify from the install or how to prevent the install script from being executed?

fatso83 · 2017-12-08T15:57:59Z

Circle CI Node 8: error while loading shared libraries: libX11-xcb.so.1: cannot open shared object file: No such file or directory 🤢

fatso83 · 2017-12-08T16:09:53Z

No clever thoughts on the Node 4 thing. We could just skip it? With regards to the crashing on Node 8:
Installing the libx11-xcb1 library should probably fix it. It would require adding a pre section to the circle ci config (circle.yml). Something like

dependencies:
  pre:
    - sudo apt-get update; sudo apt-get install libx11-xcb1

fatso83 · 2017-12-08T16:10:26Z

.travis.yml

@@ -19,6 +19,8 @@ env:
 before_install:
  - npm config set strict-ssl false
  - npm install coveralls
+  # Prevent mochify -> puppeteer install script to run unnecessarily
+  - if [ "x$TRAVIS_NODE_VERSION" != "x8" ]; then npm config set ignore-scripts true; fi


That was new! Cool to know.

fatso83 · 2017-12-08T16:16:23Z

So it failed again on Node 4, but I don't understand why. It says it executed this:

npm config set strict-ssl false
npm install

But the before_install section contains

npm config set strict-ssl false
npm install coveralls

fatso83 · 2017-12-08T16:17:05Z

Oh, I mixed up the Travis config and the Circle CI. Will you update the Circle CI config, @mantoni?

mantoni · 2017-12-08T16:18:31Z

Yeah, I just experimented with the Travis build as I don't know much about CircleCI yet. I think we could do a similar hack there and maybe only run Mochify on Node 6 and disable the install script otherwise. Once is enough anyway.

mantoni · 2017-12-08T16:32:26Z

Err, I don't really understand what't going on in the CircleCI Node 6 build now. The pre-test thingy sais sh: =~: unknown operand while that works fine on the other builds. Also verified the notation locally. 😠🤔

fatso83 · 2017-12-08T16:35:13Z

@mantoni For some reason the v6 is running POSIX shell, not Bash, while the other runners use Bash.

Version 6:

#!/bin/sh -eo pipefail
npm config set strict-ssl false
if [[ ! "$(node --version)" =~ ^v6 ]]; then
  # Avoid downloading chromium unnecessarily
  npm config set ignore-scripts true
fi
npm install
sh: =~: unknown operand

Version 4:

#!/bin/bash -eo pipefail

fatso83 · 2017-12-08T16:38:43Z

Anyway, it works, so I think this is ready for merge! The change for shell is outside the config, and perhaps something @fearphage might know about? It's probably in the VM setup for Circle CI.

mantoni · 2017-12-08T16:39:31Z

Well, it doesn't seem to run the browser tests on CircleCI because of that. Testing something else now.

fatso83 · 2017-12-08T16:41:08Z

test/webworker/webworker-script.js

@@ -4,7 +4,7 @@

 // Abort if we are not running in a WebWorker
 if (typeof importScripts !== "undefined") {
-    importScripts("../../pkg/sinon.js");
+    importScripts("https://localhost:8080/pkg/sinon.js");


shouldn't this be just "/pkg/sinon.js"?

Good point. Adjusted in b3cd2d8.

fatso83 · 2017-12-08T16:41:41Z

test/webworker/webworker-support-assessment.js

@@ -8,7 +8,7 @@ if (typeof Worker !== "undefined") {

    describe("WebWorker support", function () {
        it("should not crash", function (done) {
-            var worker = new Worker("file://" + __dirname + "/webworker-script.js");
+            var worker = new Worker("https://localhost:8080/test/webworker/webworker-script.js");


ditto: could be made relative.

mantoni · 2017-12-08T16:46:46Z

Also slightly disturbing is the drop in tests being executed in npm run test-coverage. It only executes 639 tests while it used to run 1138 (see https://travis-ci.org/sinonjs/sinon/jobs/313574869#L1709 vs https://travis-ci.org/sinonjs/sinon/jobs/313396213#L1939)

mroderick · 2017-12-10T20:03:24Z

This is an exciting PR, watching from the sidelines (with popcorn)

mantoni · 2017-12-11T09:04:38Z

I got the conditionals fixed for the CircleCI build. Also, it's necessary to activate npm scripts again after installing or npm run test-node does nothing.

Now there's a new issue with launching chroming:

Error: Failed to launch chrome! spawn /root/sinon/node_modules/puppeteer/.local-chromium/linux-515411/chrome-linux/chrome ENOENT

I tried with a ssh session and don't see what the problem is. The issues that people are seeing with missing libraries seem to be something else – maybe we're going to run into those once we can actually run the chrome binary.

The thing is, we have a nicely working Travis build and if CircleCI is giving us a hard time, I don't see the point putting more effort into it.

mantoni · 2017-12-11T09:46:08Z

@sinonjs/sinon-core I'd rather remove the headless test-runs from CircleCI for now (or comment them out). What do you think?

fearphage · 2017-12-11T15:08:39Z

.circleci/config.yml

@@ -18,12 +18,19 @@ jobs:
          name: Install dependencies
          command: |
              npm config set strict-ssl false
+              NODE_VERSION=$(node --version)
+              if [[ ${NODE_VERSION:0:3} != "v6." ]]; then


MINOR: if [[ ! "$(node --version)" =~ ^v6 ]]; then

fearphage · 2017-12-11T15:09:13Z

.circleci/config.yml

          command: |
-              if [[ ! "$(node --version)" =~ ^v4 ]]; then
+              NODE_VERSION=$(node --version)
+              if [[ ${NODE_VERSION:0:3} == "v6." ]]; then


MINOR: if [[ "$(node --version)" =~ ^v6 ]]; then

Yeah, that is what I tried first, but it failed saying that =~ was an invalid operator.

Oh, fair enough. I didn't look at the history.

mantoni · 2017-12-14T20:04:34Z

Since nobody seems to have a strong opinion, I removed the headless test script invocations on CircleCI for now. The tests run successfully on Travis which has to be good enough. If we want to move entirely to CircleCI, the Headless Chromium issue is a blocker.

I also realized that we've been generating the coverage reports on each node version in the Travis build. I've put an if around the invocation to only run it on Node 8, like the other headless test runs.

With these changes the build is green. Merge?

fearphage · 2017-12-14T20:08:54Z

We can look into the circleci tests at some point, but I think it should be in a separate PR as long as Travis is running them.

coveralls · 2017-12-14T20:20:16Z

Changes Unknown when pulling b51d5b9 on headless-chrome into ** on master**.

mroderick · 2017-12-18T10:05:45Z

I think we should merge this

mantoni · 2017-12-18T12:16:11Z

Thanks everybody! 🚀

* reverts parts of sinonjs#1629

fatso83 reviewed Dec 8, 2017

View reviewed changes

mantoni force-pushed the headless-chrome branch from 7a0e230 to 538af0c Compare December 8, 2017 16:21

fatso83 approved these changes Dec 8, 2017

View reviewed changes

mantoni force-pushed the headless-chrome branch from 538af0c to 879a6a0 Compare December 8, 2017 16:39

fatso83 reviewed Dec 8, 2017

View reviewed changes

mantoni force-pushed the headless-chrome branch from b3cd2d8 to 83a9a5d Compare December 11, 2017 08:21

mantoni added 2 commits December 11, 2017 09:32

Prevent install scripts from running unnecessarily

c0c6779

Use relative URLs in WebWorker test

93f422a

mantoni force-pushed the headless-chrome branch from 83a9a5d to 93f422a Compare December 11, 2017 08:32

Fix coverage test run

07db0f4

fearphage reviewed Dec 11, 2017

View reviewed changes

Remove headless test runs from CircleCI

c1fd210

Run test-coverage script only once on Travis

b51d5b9

mantoni force-pushed the headless-chrome branch from b98be3b to b51d5b9 Compare December 14, 2017 20:08

sinonjs deleted a comment from coveralls Dec 18, 2017

mantoni merged commit 78a8986 into master Dec 18, 2017

fearphage added a commit to fearphage/Sinon.JS that referenced this pull request Dec 19, 2017

enabled headless and webwork tests

5cea23a

* reverts parts of sinonjs#1629

fearphage mentioned this pull request Dec 19, 2017

Fix headless chrome in CircleCI #1636

Merged

mantoni deleted the headless-chrome branch December 19, 2017 16:45

franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019

enabled headless and webwork tests

3374e41

* reverts parts of sinonjs#1629

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Mochify 5 to run tests in Headless Chrome #1629

Upgrade to Mochify 5 to run tests in Headless Chrome #1629

mantoni commented Dec 8, 2017 •

edited

Loading

mantoni commented Dec 8, 2017

fatso83 commented Dec 8, 2017 •

edited

Loading

fatso83 commented Dec 8, 2017

fatso83 Dec 8, 2017

fatso83 commented Dec 8, 2017

fatso83 commented Dec 8, 2017 •

edited

Loading

mantoni commented Dec 8, 2017

mantoni commented Dec 8, 2017

fatso83 commented Dec 8, 2017 •

edited

Loading

fatso83 commented Dec 8, 2017

mantoni commented Dec 8, 2017

fatso83 Dec 8, 2017

mantoni Dec 8, 2017

fatso83 Dec 8, 2017

mantoni commented Dec 8, 2017

mroderick commented Dec 10, 2017

mantoni commented Dec 11, 2017

mantoni commented Dec 11, 2017

fearphage Dec 11, 2017

fearphage Dec 11, 2017

mantoni Dec 11, 2017

fearphage Dec 11, 2017

mantoni commented Dec 14, 2017

fearphage commented Dec 14, 2017

coveralls commented Dec 14, 2017

mroderick commented Dec 18, 2017

mantoni commented Dec 18, 2017

Upgrade to Mochify 5 to run tests in Headless Chrome #1629

Upgrade to Mochify 5 to run tests in Headless Chrome #1629

Conversation

mantoni commented Dec 8, 2017 • edited Loading

mantoni commented Dec 8, 2017

fatso83 commented Dec 8, 2017 • edited Loading

fatso83 commented Dec 8, 2017

Choose a reason for hiding this comment

fatso83 commented Dec 8, 2017

fatso83 commented Dec 8, 2017 • edited Loading

mantoni commented Dec 8, 2017

mantoni commented Dec 8, 2017

fatso83 commented Dec 8, 2017 • edited Loading

fatso83 commented Dec 8, 2017

mantoni commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mantoni commented Dec 8, 2017

mroderick commented Dec 10, 2017

mantoni commented Dec 11, 2017

mantoni commented Dec 11, 2017

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

mantoni commented Dec 14, 2017

fearphage commented Dec 14, 2017

coveralls commented Dec 14, 2017

mroderick commented Dec 18, 2017

mantoni commented Dec 18, 2017

mantoni commented Dec 8, 2017 •

edited

Loading

fatso83 commented Dec 8, 2017 •

edited

Loading

fatso83 commented Dec 8, 2017 •

edited

Loading

fatso83 commented Dec 8, 2017 •

edited

Loading