Update to a more recent version of mocha
Closed this issue · 9 comments
I keep getting security warnings from github and npm when depending on this package. On both the latest release and master, I see
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Prototype Pollution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ minimist │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=0.2.1 <1.0.0 || >=1.2.3 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ 80897cf53105dcd799d9cd1b63b9d548afb11e934eaeb9327be2ebafa8b… │
│ │ [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ 80897cf53105dcd799d9cd1b63b9d548afb11e934eaeb9327be2ebafa8b… │
│ │ > mocaccino > mocha > mkdirp > minimist │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://npmjs.com/advisories/1179 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Prototype Pollution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ minimist │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=0.2.1 <1.0.0 || >=1.2.3 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ 80897cf53105dcd799d9cd1b63b9d548afb11e934eaeb9327be2ebafa8b… │
│ │ [dev] │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ 80897cf53105dcd799d9cd1b63b9d548afb11e934eaeb9327be2ebafa8b… │
│ │ > mocha > mkdirp > minimist │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://npmjs.com/advisories/1179 │
└───────────────┴──────────────────────────────────────────────────────────────┘
which I suspect is both mochify and mocaccino depending on mocha v5 (the current major release of mocha is v7). I'd submit pull requests for both mochify and mocaccino to update to the latest versions of mocha but I think mocaccino's dependency on phantomjs is causing TypeError: undefined is not a function (evaluating 'Array.from(arguments)'). That error I suspect is from phantomjs using an old javascript engine. Therefore, updating mocaccino is dependent on switching its tests to chromium (or using a polyfill to make phantomjs support Array.from) and updating mochify is dependent on updating mocaccino (mochify sees the same exact error when updating mocha).
I recorded the mocaccino phantomjs issue in mantoni/mocaccino.js#29.
Yes, this is an issue and I would like to upgrade to a newer Mocha version too. I'm more than happy to merge any PRs that upgrade Mocha in the current implementation.
However, be warned that I've started a complete rewrite of Mochify with a different achitecture. It might take some time to complete, so it's great if you can help with upgrading Mocha in the current implementation. Just don't put too much effort into it 😉. Maybe you can try an Array.from polyfill, just for the tests?
I looked into upgrading mocaccino to Mocha 8 over at this branch: https://github.com/mantoni/mocaccino.js/tree/mocha-update
There were some changes in the mocha API to consider and output format changed slightly. In the end everything seems to work well in Node.
Apart from that two reporter tests fail in Phantom.js for reasons I do not understand:
uses-reporter
I have no idea at all. Polyfilling Array.from as suggested did not make a difference.
TypeError: Type error
at http://localhost:43381/js/bundle:27703
at http://localhost:43381/js/bundle:27703
at http://localhost:43381/js/bundle:3309
at http://localhost:43381/js/bundle:3394
at http://localhost:43381/js/bundle:25712
at http://localhost:43381/js/bundle:25735
at http://localhost:43381/js/bundle:31535
TypeError: Type error
at http://localhost:43381/js/bundle:27719
at http://localhost:43381/js/bundle:27719
at http://localhost:43381/js/bundle:3331
at http://localhost:43381/js/bundle:3402
at http://localhost:43381/js/bundle:25100
at http://localhost:43381/js/bundle:25645
at http://localhost:43381/js/bundle:0
at http://localhost:43381/js/bundle:31508
uses-custom-reporter
Looks like this reporter might not be compatible with Mocha 8 anymore. I tried using the next best npm package I could find but in this case it failed as the code was using const and the likes which Phantom.js does not know about.
TypeError: invalid reporter 'mocha-jenkins-reporter'
at http://localhost:37701/js/bundle:37969
at http://localhost:37701/js/bundle:39445
at http://localhost:37701/js/bundle:1
at http://localhost:37701/js/bundle:1
undefined: 'mocha-jenkins-reporter' reporter blew up with error:
commonjsRequire@http://localhost:37701/js/bundle:9333:93
reporter@http://localhost:37701/js/bundle:37953:38
http://localhost:37701/js/bundle:39445:15
o@http://localhost:37701/js/bundle:1:277
r@http://localhost:37701/js/bundle:1:440
global code@http://localhost:37701/js/bundle:1:468
Considering Mochify does not use Phantom.js anyways anymore, would it be a thing to drop the support (mantoni/mocaccino.js#29) over at mocaccino so that this does not keep us locked to old Mocha versions in mochify?
Thank you for looking into this. We can drop PhantomJS, that's fine with me.
Instead of just deleting all the browser tests in mocaccino I made the "mistake" to port them to some mini-Mochify like setup that runs the tests in a browser and found out about the following:
Mocha 8+ uses a Rollup-bundled browser version that seems to be incompatible with how mocaccino imports custom reporters, making it throw
"Error: Dynamic requires are not currently supported by @rollup/plugin-commonjs
at commonjsRequire (http://127.0.0.1:7890/:9334:9)
at Mocha.reporter (http://127.0.0.1:7890/:37954:23)
at Object.require.40 (http://127.0.0.1:7890/:39446:7)
at o (http://127.0.0.1:7890/:2:273)
at r (http://127.0.0.1:7890/:2:439)
at http://127.0.0.1:7890/:2:468"
which smells like an issue in Mocha itself (and cannot be fixed in mocaccinio as is - but I might be wrong about this) as they do prescribe a dynamic import here: https://github.com/mochajs/mocha/blob/5064c282d13259925af05845026686bbe435d763/lib/mocha.js#L340-L342
I'll dig into this further when I find the time.
Ah, I thought you are aware of that issue. I filed a PR to fix this in Mocha, but they don't seem to be interested in supporting browserify anymore.
Therefore my future strategy is to build Mochify differently from the ground up, but I didn't find the time doing this, apart from some experiments.
So I managed to work around the dynamic require issue by requiring the reporter in the setup script mantoni/mocaccino.js@df8ffea
There is one more issue remaining though that I don't fully understand yet: Some built-in reporters (list for example) now start printing format strings and their args instead of interpolating these. For example the list reporter test now prints:
✓ %s: %dms fixture passes 0
%d passing (%s) 1 4ms
when previously we'd see:
✓ fixture passes: 0ms
1 passing (4ms)
This seems to have been introduced with Mocha 6 (it fails for 6, 7 and 8), although I cannot really find anything in the changelog here https://github.com/mochajs/mocha/blob/master/CHANGELOG.md#600--2019-02-18 that sound like it could be causing this?
Is this possibly related to brout?
Failing build here: https://travis-ci.org/github/mantoni/mocaccino.js/jobs/771339993
So the log formatting change has been introduced in Mocha 6.2.0 to be exact it seems: https://github.com/mochajs/mocha/blob/master/CHANGELOG.md#620--2019-07-18
I suspect it's this change: mochajs/mocha#3725 but I don't fully understand where this breaks right now. If you have any hint @mantoni that'd be much appreciated.
It seems brout now needs to be required before mocha, but if you do it like this it works as expected: mantoni/mocaccino.js@9b4e1b2
I'll open a PR for this in the mocaccino repo tomorrow so we can have a closer look at the mess over there 🌔
@mantoni Just so we don't duplicate efforts, I started the Mocha 8 update for Mochify itself here https://github.com/mantoni/mochify.js/tree/mocha-8
There's some strange logging of deprecation notices going on in Chromium, but I think I should be able to look into these soon.