Question: can mocha dependency be upgraded?

[pixelhandler]

I'm curious if we can upgrade mocha to "^5.1.1" instead of "mocha": "^2.5.3" ?

I tried out (with npm-link) using the latest mocha and tests worked fine.

[Turbo87]

see ember-cli/ember-cli-mocha#167

[apellerano-pw]

I know there's no real security concern because this is a development plugin; but in the name of keeping warnings channels tidy so they don't get ignored, this package is noisy on a couple of npm warning outputs now.

npm audit displays:

1. growl as a Critical risk
2. minimatch as a High risk

And the deprecation warnings produced by installing the package:

$ npm install --save-dev ember-mocha
npm WARN deprecated [email protected]: to-iso-string has been deprecated, use @segment/to-iso-string instead.
npm WARN deprecated [email protected]: Jade has been renamed to pug, please install the latest version of pug instead of jade
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue

I'm not sure what other incentives there are to upgrading the mocha dependency but maybe if we start listing them here it will give a clearer picture of the benefits of doing the upgrade.

[Turbo87]

@apellerano-pw don't get me wrong, I would love to update. but last time I tried I couldn't find a way to support the "old testing APIs" with the newer Mocha releases. fortunately we have the new testing APIs now that should work with the newer Mocha, but we'd have to drop support first before upgrading Mocha.

[apellerano-pw]

Ah, that's bittersweet. It will probably take a while for dropping support to make sense and doing it earlier would strand ppl on the 0.14.x line. But at least something is in motion.

Do you know exactly what the issue is with getting old testing apis working with newer mocha? Is it something other people could take a stab at?

[Turbo87]

@apellerano-pw the specific issue is https://github.com/emberjs/ember-mocha/blob/master/vendor/ember-mocha/ember-mocha-adapter.js, which allows the "implicit async helper chaining tests" where you use andThen() instead of calling done() at the end or returning a Promise. that is not supported by default in Mocha and that adapter hacked it so that it works, but as I said above, with newer Mocha releases I haven't been able to make it work.

[a13o]

@Turbo87 I've been messing around with mocha@3 in this project. I'm still a little confused as to exactly what is holding back the upgrade, likely because I'm new to this project. Here's what I've observed so far:

• The andThen tests seem to work fine.
• There are two tests that fail due to mocha@3's overspecify constraint (don't use done and return a promise). These tests are easily fixable but it might be considered a breaking change. Does ember-mocha want to continue supporting this use case which was dropped by the parent lib? We would have to choose which of the overspecifications to ignore...
• I was able to get mocha@3's new it.only behavior working in ember-mocha-adapter.js by mostly copying what the other adapters are doing in mocha itself https://github.com/mochajs/mocha/blob/e838a774ac238c7e5556838dafb6163aad49cf9e/lib/interfaces/common.js#L133-L137

Is there some combination of these issues holding back the upgrade? An example of a test in this project that fails under mocha@3 but is supposed to pass would be a helpful place to start.

btw here is the changes I made to context.it.only to make it work:

context.it.only = function(title, fn){
  var test = context.it(title, fn);
  // old and busted
  //mocha.grep(test.fullTitle());

  // new hotness
  test.parent._onlyTests = test.parent._onlyTests.concat(test);
  mocha.options.hasOnly = true;
};

I assume something similar could be done for context.describe.only. edit: it can.

context.describe.only = function(title, fn){
  var suite = context.describe(title, fn);
  suite.parent._onlySuites = suite.parent._onlySuites.concat(suite);
  mocha.options.hasOnly = true;
};

[a13o]

I have it working and tested with the new .only behavior.

For the overspecification issue, I made every it use the invoke wrapper (previously fns with arity=1 were passed straight to mocha) and when invoking the function it passes a stub in place of the done. So now all tests coming from the ember-mocha-adapter are technically async as far as mocha is concerned and no done call in anyone's tests actually does anything. This fixes those tests but there may be a use case I can't think of yet where someone actually expects that done call to be doing some work. Any ideas?

At this point I believe I have ember-mocha working with mocha@3. Do you have any additional test cases that might fail in mocha@3?

How I silenced done calls to avoid overspecification:

   function invoke(context, fn, d) {
     done = d;
     isPromise = false;
-    var result = fn.call(context);
+    // In case fn expects a done hook, stub it
+    var result = fn.call(context, function () {});

       context.it = context.specify = function(title, fn){
         var suite = suites[0], test;
         if (suite.pending) {
           fn = null;
         }
-        if (!fn || fn.length === 1) {
+        if (!fn) {

[jasonworden]

@a13o Where are you getting access to context in order to do context.it.only = ...?

I'm looking for a way to do it without making a new Mocha interface e.g. Mocha.interfaces['ember-mocha'].

edit: looks like I can do this in addon-test-support/mocha/index.js

[a13o]

@jasonworden I think this repo has updated a bunch since I was last looking into this issue. My old PR is still viewable here: #216

There was a vendor/ember-mocha/ember-mocha-adapter.js containing all this package's monkey patches for mocha behavior, and I believe the line you're referencing was there.

I see you've since found it in your edit, just wanted to provide any addtl context I could (no pun intended)

[mileslane]

Two years later, growl is still showing up as a critical vulnerability. I guess we won't remediate it since this is a dev dependency.