Update Mocha to v8, drop Phantomic, run browser tests in Chromium #35
Conversation
| @@ -44,6 +44,11 @@ module.exports = function (b, opts) { | |||
| return { main : pkg.browser }; | |||
| } | |||
| }); | |||
| var broutPath = path.relative(process.cwd(), broutFile); | |||
There was a problem hiding this comment.
This change is not strictly necessary, but it's a leftover from moving things around and I felt the order is now easier to understand. It could also just be reverted again.
| @@ -139,7 +138,9 @@ module.exports = function (b, opts) { | |||
| var self = this; | |||
| listener.then(function (err, res) { | |||
| if (!err) { | |||
| setupSource = (res.mocha || '') + res.setup; | |||
| setupSource = (opts.node ? '' : 'require(\'brout\');\n') | |||
There was a problem hiding this comment.
Mocha 6.2.0 starts adding behavior to console.log mochajs/mocha#3725 so it's now needed to require brout before requiring mocha. I'm not sure if this ad-hoc string doing it here for us is the most elegant way to do it, but then I also didn't want to add a single line file. Any opinions?
There was a problem hiding this comment.
My opinion is that any hack that get's us there is fine 😆
| if (Mocha.reporters['{{REPORTER}}']) { | ||
| mocha.reporter('{{REPORTER}}', '{{REPORTER_OPTIONS}}'); | ||
| } else { | ||
| mocha.reporter(require('{{REPORTER}}'), '{{REPORTER_OPTIONS}}'); |
There was a problem hiding this comment.
Mocha is now bundled using Rollup which does not support dynamic requires for CommonJS. This way we pass in the reporter module instead of its name so that Mocha doesn't have to do any module resolution anymore.
| @@ -22,7 +22,7 @@ if ('{{INVERT}}' === true) { // eslint-disable-line no-constant-condition | |||
| } | |||
| _mocha.reporter('{{REPORTER}}', '{{REPORTER_OPTIONS}}'); | |||
| _mocha.timeout('{{TIMEOUT}}'); | |||
| _mocha.useColors('{{USE_COLORS}}'); | |||
| _mocha.color('{{USE_COLORS}}'); | |||
There was a problem hiding this comment.
useColors seems to have been deprecated a while ago and was removed in Mocha 8 mochajs/mocha#4178
| }); | ||
| s.pipe(p.stdin); | ||
| } else if (proc === 'chromium') { | ||
| b.bundle(function (err, result) { |
There was a problem hiding this comment.
This is a stripped down version of what is happening in Mochify.
| + '# tests 1\n' | ||
| + '# pass 1\n' | ||
| + '# fail 0\n'); | ||
| + '# fail 0\n' | ||
| + '1..1\n'); |
There was a problem hiding this comment.
I don't know too much about the TAP spec, but it seems the plan seems to get printed after the results now? I think it's this commit: mochajs/mocha@2078c32#diff-e8a8c7ad26fed3d12b4bd90577d2fb603f34e78ac146e603c043964f96f5a37e that is causing it. I have no idea if this will be a problem when using this in Mochify?
There was a problem hiding this comment.
When linking this version of mocaccino into my local version of mochify this will also make quite a few tests fail because they assert the previous format.
There was a problem hiding this comment.
Seems TAP is fine with the plan being either printed at the beginning or the end of the output https://testanything.org/tap-specification.html#the-plan so this is not a bug or anything.
There was a problem hiding this comment.
Thank you for researching. This will be a major release, and also a major of mochify, so it's fine if the output format changes.
| run('phantomic', [], b, passOutputAssert(done)); | ||
| }); | ||
|
|
||
| it('passes --brout test', function (done) { |
There was a problem hiding this comment.
I removed these tests as we do not pass any flags to Chromium in any way.
|
Awesome! This is great 🥇 📦 Released in |
As discussed over in mantoni/mochify.js#213, this updates Mocha to the most recent version.
As more and more packages (e.g. Mocha reporters) seem to use syntax that Phantom.js does not seem to handle well anymore, this PR also moves the browser based test to a Chromium based setup that is pretty close to what is happening in Mochify itself (which is probably a good canary here too in case something goes wrong).
I would assume this will be a new major version, so I also went ahead and changed the Node versions used in Travis to all currently supported ones.