Make CI run browser tests #2231

dasilvacontin · 2016-04-28T22:25:16Z

Resolves #2079 – still WIP.

"Difficulties" found so far:

zuul comes with Mocha bundled (they actually have our bundle file in their source 👌 ) – this makes it hard to make some tests work, as they rely on the Mocha code being tested to be also the code that is running the tests. Might be worth taking a look at using Karma-PhantomJS framework-less (without having Mocha bundled).
zuul didn't accept arrays for the included / excluded browserify options, had to fix and fire a PR: Support Arrays in browserify conf defunctzombie/zuul#280.
a lot of the tests are written in a way that you have to execute Mocha for each file (hence the super large Makefile with lots of different test-xxx). Together with the fact that quite a few tests can only work in node (using fs, http.createServer and such) makes that the subset of tests that can be run in the browser is not as big as one would hope. Rewriting some of the tests + adding new ones would be ideal, but not sure it's super worth considering that we are looking forward to the Mocha's rewrite. Still, those tests could likely be ported to the rewrite, not so bad I guess...
I might need admin permissions over the org/repo to generate and encrypt sauce labs credentials – not sure yet, but it wouldn't hurt having them I guess. /cc @boneskull

Feedback is welcome – looking forward to getting this done and rocking the of the issues/PRs!

danielstjules · 2016-04-28T23:20:28Z

Have you thought of just using phantomjs (w/ mochify) or nightmare for browser tests as discussed here #2079 (comment) ?

I'd probably not recommend using zuul w/ sauce labs for all our browser tests as it's a flakey setup, though it works great locally. There's just something about running it on Travis. :) I use zuul for https://github.com/zendesk/cross-storage The builds end up experiencing frequent connection errors (between travis, localtunnel or saucelabs, etc) Here's an example: https://travis-ci.org/zendesk/cross-storage/builds/123155396 Looking at the build history you can see that they eventually pass, it just often takes a restart or two.

Edit: Though this could have been resolved in newer releases :)

dasilvacontin · 2016-04-29T17:10:54Z

Edit: Though this could have been resolved in newer releases :)

What are you referring to?

I'm gonna give PhantomJS and other solutions a go – thanks for sharing!

danielstjules · 2016-04-29T17:30:04Z

Oh, just meant that maybe stability improved in a more recent release, though I'm not sure.

dasilvacontin · 2016-04-29T20:18:44Z

Going good so far using mocha-phantomjs. I'm able to test the source using the source itself as the test framework.

dasilvacontin · 2016-05-04T12:15:29Z

Status update: I've been sick. I'm feeling better now – looking forward to make progress on the PR.

dasilvacontin · 2016-05-04T23:51:17Z

lib/browser/tty.js

@@ -1,5 +1,5 @@
 exports.isatty = function isatty() {
-  return true;
+  return false;


self-note: look into why were we reporting tty as true in the browser

It's been there since @tj first committed the file. Not sure why yet.

ooh, hmm, missed this. it shouldn't change unless we have a good reason.

ScottFreeCode · 2016-05-12T02:36:20Z

For what it's worth, if the idea of using Karma in order to use a git checkout of Mocha instead of a published/bundled version is still being kicked around, Karma + Karma's Mocha runner seems to work with a local checkout of Mocha if you add Mocha's folder to the NODE_PATH environment variable (so it gets picked up the same way it would if it were in node_modules). And by "work" I mean it will at least try to run and presumably should work just as well as using Mocha with Karma for any other project, not that I actually have got to the point where tests are passing instead of having weird failures like Karma restarting the browser three times for no apparent reason. ;^) Karma's also supposed to a Sauce Labs runner that I would hope can run IE 8, since that's probably the browser that most needs testing against beyond picking any modern browser just to make sure some Node library doesn't break browsers altogether; I haven't even started to try that, though.

The other test runner I've heard of is Testling, which I'm pretty sure I've heard of being used with Mocha, but I haven't yet looked into whether that allows you to provide your own copy of the test library without having to write your own adapter with which to use it.

ScottFreeCode · 2016-05-12T02:45:02Z

Addendum: You also have to build an up-to-date mocha.js file to use Karma's Mocha plugin with NODE_PATH including Mocha's folder. That poses a certain amount of potential conflict with the "don't update mocha.js till a release" policy... although not much -- CI isn't getting checked back into Git, so it shouldn't matter if it updates the build in order to run tests, whereas local testing... you'd just have to be sure to tell Git to discard the changes to mocha.js I guess. Well, that's that for what it's worth; I'll report back if I discover anything else worthy of note in the process of actually getting to where tests pass.

ScottFreeCode · 2016-05-12T03:20:10Z

So, I can't seem to get bundling the test files (and by extension their dependencies) like the phantomjs bundle to work with Karma, but I did figure out that it works fine if I give Karma the list of test files instead, as long as:

I also give it the files for assertion libraries like "should" and any similar dependencies.
The test doesn't have any requires that can't be satisfied by the browser files for such libraries.

boneskull · 2016-05-13T05:43:31Z

it appears we'll want to run IE and phantomjs (pre 2.0), so I think the most direct route to that might be to just pick up karma and eschew mocha-phantomjs.

boneskull · 2016-05-13T07:01:43Z

where are the builds?

boneskull · 2016-05-13T07:02:09Z

I have a feeling I screwed something up w/ travis but I have no idea what that could be.

dasilvacontin · 2016-05-13T09:51:23Z

scripts/upgrade-npm.js

+        throw new Error(err);
+      }
+      // I'm unsure what this one is for, but whatever.
+      exec('npm install --global npm', function (err) {


npm v1.x can't upgrade directly to v3.x, so we first upgrade to v2, and then to v3.

Why was the script rewritten? With what purpose? The only behavioural change I see is the if condition, and that could have been easily changed in the bash script afaik.

npm v1.x can't upgrade directly to v3.x, so we first upgrade to v2, and then to v3.

we don't need to actually upgrade to v3? so, we can skip that.

the shell script won't run on windows. this is premature optimization, because I know that we'll need to run IE, and I don't want to have anything in here that isn't portable. it also opens us up to running unit tests on diff platforms eventually.

we don't need to actually upgrade to v3? so, we can skip that.

But maybe it wouldn't be a bad idea to test if dependencies install correctly while using the current version of npm.

the shell script won't run on windows. this is premature optimization, because I know that we'll need to run IE, and I don't want to have anything in here that isn't portable. it also opens us up to running unit tests on diff platforms eventually.

Good call.

But maybe it wouldn't be a bad idea to test if dependencies install correctly while using the current version of npm.

Nvm, we are already doing that when testing on newer Node versions.

dasilvacontin · 2016-05-13T09:57:37Z

@boneskull what builds? afaik the package.json and/or Makefile hasn't been changed so that phantom.js is run on CI. I don't see any code related to spinning up karma either.

If you mean Travis builds, I can see them now, but maybe you've experienced some kind of delay. (?)

dasilvacontin · 2016-05-13T09:58:52Z

I just saw from the other PR that you meant Travis. 🙈

boneskull · 2016-05-13T18:24:27Z

yes, there was a significant delay. anyway, the phantomjs version is failing to install on 0.8 for some reason. but the older ones (1.9.7.x) fail to install on newer versions of node, so ugh.

ScottFreeCode · 2016-05-15T02:28:42Z

Something that crossed my mind: Will the mocha.js need to have its timestamp tweaked so that make will be able to tell whether to update it for the tests, e.g. like https://gist.github.com/davidwindell/fbfef588c6295666c6a1 or maybe https://gist.github.com/jeffery/1115504 ?

ScottFreeCode · 2016-05-19T23:22:52Z

Check it out! The tests are finding the actual IE8 incompatibilities now (and otherwise passing*)!

*Well, I've still got to figure out what to do about PhantomJS; latest doesn't work on Node 0.8, but we got a failure [seemingly unrelated to any change or code on our end] downloading 1.9.8 recently.

ScottFreeCode · 2016-05-20T00:42:46Z

Some additional comments:

I've been testing with IE11 with Karma's HTML tweaked to trigger IE8 compatibility mode (not sure if that's how I ought to be doing it, but it works for most things...). I do not get the failure on the test for length that cannot be coerced to number. Is it likely to be an issue that exists in real IE8 but not IE8 compatibility mode? (EDIT: could it be the use of toString as the property name? Obviously I can't easily check except by pushing and seeing if a different property name fixes it.)
Looks like a lot of these are the toISOString issue (cf Fix missing methods/globals in IE7, IE8 environments #2259). If I run the interface browser tests a lot of them are indexOf. (Note to self: look at which object/prototype is missing indexOf and whether it's being used inside Mocha itself or just in the test code.) (EDIT: These are all handled now, at least assuming my IE8 test for the prototypeless object is valid.)
After we fix the ones we can fix quickly, do we want to disable the difficult IE8-failing tests (say, using reverse grep?) in the IE8 test run only, in order to go ahead and start getting other things tested and merged and be able to come back and fix those tests for IE8 in due time? (EDIT: there's only one left, the length thing.)

ScottFreeCode · 2016-05-20T00:43:44Z

Do we want to remove [revert] any of the earlier changes that were aimed at mocha-phantomjs (e.g. to test/browser/...)?

ScottFreeCode · 2016-05-21T00:23:03Z

@dasilvacontin, @boneskull, I think I've got everything working! Take a look and let me know if there are any issues, or if there's anything else we need to do before merging this. (Thanks!)

ScottFreeCode · 2016-05-21T05:45:01Z

Attempted to squash this a bit... David's contributions are a bit scarce between the fact that some of them ended up not being used and the fact that his improvements to the makefile were absorbed into Christopher's; and I'm ambivalent about whether that one commit that changed return ' "dave"'; to get "dave" from the array being mapped should be absorbed into the previous squash (what I've got here), or put in its own commit for obviousness, or just dropped... But it'll do if it looks good to everyone else. I've saved the original line of commits in the branch add-browser-to-CI-detailed if anyone wants to compare (although I don't know that GitHub's comparison features are going to give meaningful info on them, since they amount to the same net changes but by entirely unrelated commit history) or if we need to roll back the squashing and try again.

boneskull · 2016-05-21T09:03:39Z

restarting some failed jobs due to failed d/l of phantomjs prebuilt binary. once these pass, I think we're good.

boneskull · 2016-05-21T09:04:34Z

(thereafter I'm going to squash this in at least a couple places)

boneskull · 2016-05-21T09:19:03Z

package.json

@@ -266,7 +267,8 @@
    "node": ">= 0.8.x"
  },
  "scripts": {
-    "test": "make test-all"
+    "preinstall": "node scripts/fake-karma-mocha-peer-dep.js",


karma-mocha has a peer dep on mocha, which is a problem. we want it to use, well, this mocha, so we just symlink node_modules/mocha to the root.

this is causing a problem atm because we're not publishing the scripts dir. I'll figure out a solution; it doesn't need to run anyway unless you're running npm install from a working copy

- ⬆️ browserify@13.0.0 Some fixes that we didn't have were required. - 🐛 Fix browser not being able to require describe/it `support/browser-entry.js` was removing the listener that is in charge of keeping `Mocha.describe/it/suite` etc up-to-date. That listener is now added everytime a `ui` is set up. - Correct isatty for browser

boneskull · 2016-05-21T23:48:13Z

.travis.yml

-  - '0.8'
+matrix:
+  include:
+    - node_js: '6'


we could also add an osx target here, if anyone cares.

Worth trying, but probably best done after we get the basic browser testing merged.

boneskull · 2016-05-21T23:54:48Z

scripts/upgrade-npm.js

+
+if (process.env.TRAVIS_NODE_VERSION === '0.8') {
+  var exec = require('child_process').exec;
+  // ensure *dependencies* are installed using provided npm


(because if they don't, then mocha won't install on vanilla 0.8)

gonna amend this to can be installed

- add sauce connect - prepare to use karma on saucelabs; fix potential windows problems - rewrite `scripts/ensure-compatible-npm.sh` in JS - detect travis node 0.8 env; just upgrade if found - symlink `mocha` to itself for `karma-mocha` - if explicit `--production` flag is present, skip this - don't know how to skip this part if `mocha` is a devDep of some other package - use phantomjs@1.9.19; don't try to install npm@3 - browser tests with karma & phantom - move `support/browser-entry.js` to `browser-entry.js` to keep paths sane - `karma.conf.js` loads data out of `.karma.conf.js` - we have a "main" browser suite - then we have suites for each interface - each suite is a single run of Karma - each suite is executed by the `Makefile` - build `./mocha.js` upon test execution for `karma-mocha` to use - bonus: 3rd-party browserification should theoretically now be possible as per the `./index.js` entry in `package.json`'s `browser` field - add dev deps for karma - test changes: - split `lookupFiles` test into its own file so we can ignore it in Karma - fix broken `test/acceptance/throw.js` for browser - fix `test/acceptance/utils.js`'s `type` tests for PhantomJS - try to reduce build matrix - fix missing targets - fix infinite loop in Makefile - downgrade phantomjs to 1.9.8 - remove some cruft from karma config; try to add sauce labs - try again w/ the saucelabs - typo - remove karma-source-map-support as it's a no-go on IE8 - Require up-to-date mocha.js for any browser tests - Make CI rebuild mocha.js. Make won't rebuild when just checked out by CI because timestamps on source and mocha.js are the same. We could use a script to set the timestamps to the commit time, but it still wouldn't work if mocha.js were committed along with other changes that hadn't been built into it. Which shouldn't happen anyway -- but then again, the point of CI is to see what commits change, it's usually going to need to rebuild mocha.js if it's working right, so it's not going to hurt much to rebuild it every time even on the few times it doesn't need to.

- Avoid `Array.prototype.map` in test - Workaround for missing `Object.create` - Use a shim for `Date.prototype.toISOString` - Use simple number math instead of array indexing for interface tests - Use `expect` instead of `should` - Avoid builtin function in stringify test (A quick check revealed that stringify does not treat toString specially anyway, and IE8 ignores the toString assigned to the object, so use a different property name) - Use `karma-expect` for automatic browser tests - Remove `karma-should`

- add note about exporting `global` in `browser-entry.js` - remove too-short timeout for "throw" unit tests (because SauceLabs) - revert modification to `isatty()` function of `lib/browser/tty.js` - switch reporter to `karma-spec-reporter` - use custom `karma-no-mocha` package - fixtures in `test/browser-fixtures/` - use single `karma.conf.js`

boneskull · 2016-05-22T20:50:57Z

@mochajs/mocha unless someone has an objection, I'll likely merge this today or tomorrow

to-iso-string is used in lib/utils.js since mochajs#2231 Fixes mochajs#2272

ScottFreeCode · 2016-05-23T20:17:26Z

Sorry about the to-iso-string mishap; I added it where the other new dependencies were without thinking about the fact that it's used in the framework code itself rather than just the tests.

dasilvacontin · 2016-05-23T21:47:43Z

@ScottFreeCode No worries. Sorry for not noticing that during code review ;)

dasilvacontin force-pushed the add-browser-to-CI branch 2 times, most recently from 904993d to 649a023 Compare April 29, 2016 21:53

ScottFreeCode mentioned this pull request May 4, 2016

Update glob for node 6 compatibility #2234

Closed

dasilvacontin reviewed May 4, 2016
View reviewed changes

dasilvacontin reviewed May 13, 2016
View reviewed changes

boneskull mentioned this pull request May 14, 2016

Bump major version of glob #2247

Closed

ScottFreeCode force-pushed the add-browser-to-CI branch 2 times, most recently from a895e63 to 907011e Compare May 19, 2016 22:26

ScottFreeCode force-pushed the add-browser-to-CI branch 3 times, most recently from 929bc2e to ff3664a Compare May 20, 2016 22:15

ScottFreeCode force-pushed the add-browser-to-CI branch 2 times, most recently from d24b6e8 to f9a727e Compare May 20, 2016 22:43

ScottFreeCode force-pushed the add-browser-to-CI branch from a7196e5 to f4d8066 Compare May 21, 2016 05:32

boneskull reviewed May 21, 2016
View reviewed changes

boneskull force-pushed the add-browser-to-CI branch from d328c48 to 4313dec Compare May 21, 2016 23:45

boneskull reviewed May 21, 2016
View reviewed changes

boneskull force-pushed the add-browser-to-CI branch from 4313dec to dd68d84 Compare May 21, 2016 23:53

boneskull reviewed May 21, 2016
View reviewed changes

boneskull force-pushed the add-browser-to-CI branch from dd68d84 to e20a003 Compare May 22, 2016 04:06

boneskull and others added 3 commits May 22, 2016 21:59

dasilvacontin force-pushed the add-browser-to-CI branch from e20a003 to 8ee7795 Compare May 22, 2016 20:06

boneskull merged commit c04c1d7 into master May 23, 2016

entertainyou added a commit to entertainyou/mocha that referenced this pull request May 23, 2016

Move to-iso-string from dev dependency to dependency

d459560

to-iso-string is used in lib/utils.js since mochajs#2231 Fixes mochajs#2272

entertainyou mentioned this pull request May 23, 2016

Move to-iso-string from dev dependency to dependency #2274

Closed

ScottFreeCode mentioned this pull request May 23, 2016

Fix missing methods/globals in IE7, IE8 environments #2259

Closed

dasilvacontin deleted the add-browser-to-CI branch May 23, 2016 21:47

Make CI run browser tests #2231

Make CI run browser tests #2231

Conversation

dasilvacontin commented Apr 28, 2016

danielstjules commented Apr 28, 2016 • edited

dasilvacontin commented Apr 29, 2016

danielstjules commented Apr 29, 2016

dasilvacontin commented Apr 29, 2016 • edited

dasilvacontin commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottFreeCode commented May 12, 2016

ScottFreeCode commented May 12, 2016

ScottFreeCode commented May 12, 2016

boneskull commented May 13, 2016

boneskull commented May 13, 2016

boneskull commented May 13, 2016

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

dasilvacontin commented May 13, 2016 • edited

dasilvacontin commented May 13, 2016

boneskull commented May 13, 2016

ScottFreeCode commented May 15, 2016

ScottFreeCode commented May 19, 2016 • edited

ScottFreeCode commented May 20, 2016 • edited

ScottFreeCode commented May 20, 2016 • edited

ScottFreeCode commented May 21, 2016

ScottFreeCode commented May 21, 2016 • edited

boneskull commented May 21, 2016

boneskull commented May 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull May 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented May 22, 2016

ScottFreeCode commented May 23, 2016

dasilvacontin commented May 23, 2016

danielstjules commented Apr 28, 2016 •

edited

dasilvacontin commented Apr 29, 2016 •

edited

dasilvacontin commented May 13, 2016 •

edited

ScottFreeCode commented May 19, 2016 •

edited

ScottFreeCode commented May 20, 2016 •

edited

ScottFreeCode commented May 20, 2016 •

edited

ScottFreeCode commented May 21, 2016 •

edited

boneskull May 21, 2016 •

edited