fix CI; closes #2867 #2868

boneskull · 2017-06-07T18:59:58Z

I think the package-lock.json creation created by npm@5 was causing us to be unable to do npm install after an npm install --production.

boneskull · 2017-06-07T19:01:53Z

See npm/npm#16967

coveralls · 2017-06-07T19:30:49Z

Coverage remained the same at 88.395% when pulling 7483f0d on issue/2867 into f20de56 on master.

boneskull · 2017-06-07T19:50:28Z

Nope, that wasn't it. I have no idea. just gonna downgrade npm.

coveralls · 2017-06-07T20:33:21Z

Coverage remained the same at 88.395% when pulling 5db8121 on issue/2867 into f20de56 on master.

boneskull · 2017-06-07T20:36:40Z

This may have been an incompatible "Git-style" dependency.

coveralls · 2017-06-07T21:14:32Z

Coverage remained the same at 88.395% when pulling 778b4b6 on issue/2867 into f20de56 on master.

boneskull · 2017-06-14T00:08:24Z

Trying to just downgrade Node.js itself back to v7 for browser tests...

coveralls · 2017-06-14T00:42:35Z

Coverage remained the same at 88.395% when pulling fd326d6 on issue/2867 into 7647e18 on master.

coveralls · 2017-06-14T03:02:17Z

Coverage remained the same at 88.395% when pulling c7abeca on issue/2867 into 7647e18 on master.

coveralls · 2017-06-14T05:17:02Z

Coverage remained the same at 88.395% when pulling 9cdb4c2 on issue/2867 into 7647e18 on master.

coveralls · 2017-06-14T05:23:25Z

Coverage remained the same at 88.395% when pulling fa0169b on issue/2867 into 7647e18 on master.

coveralls · 2017-06-14T09:07:31Z

Coverage remained the same at 88.395% when pulling 8a8afc7 on issue/2867 into 7647e18 on master.

coveralls · 2017-06-14T09:07:31Z

Coverage remained the same at 88.395% when pulling 31a8acf on issue/2867 into 7647e18 on master.

boneskull · 2017-06-14T09:08:35Z

@dasilvacontin @ScottFreeCode if this goes green please merge it unless I get to it first. we should then be good to rebase the debug upgrade in branch debug onto master--at that point, it's probably worth cutting the release.

coveralls · 2017-06-14T09:34:17Z

Coverage remained the same at 88.395% when pulling 1fe4b74 on issue/2867 into 7647e18 on master.

dasilvacontin · 2017-06-14T09:40:24Z

Looks pretty green – but it's be nice to review it nevertheless. 😬

…

On 14 Jun 2017, at 11:34, Coveralls ***@***.***> wrote: Coverage remained the same at 88.395% when pulling 1fe4b74 on issue/2867 into 7647e18 on master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

boneskull · 2017-06-14T19:03:35Z

karma.conf.js

+      'karma-expect',
+      'karma-mocha',
+      'karma-spec-reporter',
+      require('@coderbyheart/karma-sauce-launcher')


The plugin is named 'karma-sauce-launcher', but the package is named something else, so we have to do this. Unfortunately, if you start specifying plugins, you have to specify all of them.

boneskull · 2017-06-14T19:05:39Z

package.json

@@ -320,6 +320,7 @@
    "supports-color": "3.1.2"
  },
  "devDependencies": {
+    "@coderbyheart/karma-sauce-launcher": "coderbyheart/karma-sauce-launcher#5259942cd6d40090eaa13ceeef5b0b8738c7710f",


karma-sauce-launcher was a misnomer. npm@3 and npm@4 install this anyway, but npm@5 does not; see npm/npm#17193.

I'm using the changeset SHA here to avoid pulling down any subsequent commits to master, since this is someone else's repo.

I have another branch wherein the Mocha org owns a fork, so we have complete control over how it behaves -- and what upgrades it receives.

(This was done in the first place because the original package is not-very-well maintained)

Looking forward to switching to our fork, although I have some questions I need to get on there and ask about that too...

boneskull · 2017-06-14T19:06:57Z

test/jsapi/index.js

@@ -9,8 +9,7 @@ var mocha = new Mocha({
  growl: true
 });

-// mocha.reporter('spec');
-require('should');
+global.expect = require('expect.js');


As noted in #2882, should is problematic for non-ES5-compliant environments, so we can't run tests using should in those environments.

jsapi runs a subset of the unit tests, which now all use expect rather than should.

boneskull · 2017-06-14T19:08:13Z

test/unit/runnable.spec.js

+              return;
+            }
+
+            try {


this test was written in a way that assertions would still be made even if the callback received an error.

boneskull · 2017-06-14T19:08:58Z

test/unit/utils.spec.js

        .equal(false);
    });
  });

  describe('map()', function () {
    var map = utils.map;
    it('should behave same as Array.prototype.map', function () {
+      if (!Array.prototype.map) {


this comparison isn't possible in non-ES5-compliant environments

ScottFreeCode · 2017-06-14T19:09:31Z

There are a couple points where this skips tests if Object.create is absent. In one case this seems to be using Object.create as a proxy for defineProperty, which is being used to get a non-writable stack on an error to test that Mocha is able to handle those; I'd prefer to have that one check whether defineProperty is available and can create a non-writable property, but I suppose what we've got is OK since the important thing is we test this in a modern environment where that works and don't attempt it in older environments where it would blow up for lack of defineProperty. In the other case, I am not actually sure what Object.create is being checked for -- is it needed for Mocha's behavior? If so, should we be attempting to polyfill that or something instead?

Other than that, this looks fine. I mean, I'm still not sure I understand what we're fixing, but the code after the changes looks OK. And, let's face it, switching from should to expect was long overdue. 🎉

boneskull · 2017-06-14T19:09:35Z

test/unit/runner.spec.js

        done();
      });
      runner.fail(test, err);
    });

    it('should recover if the error stack is not writable', function (done) {
+      if (!Object.create) {


checking for Object.defineProperty seems reasonable, but it isn't really, because IE8 implements the function incorrectly.

boneskull · 2017-06-14T19:10:08Z

test/unit/runner.spec.js

@@ -119,9 +125,12 @@ describe('Runner', function () {
    });

    it('should not fail when a new common global is introduced', function () {
+      if (process.browser) {


This test makes some assumptions about global properties that we can't guarantee across browsers.

though I think the lines here might mean this never happens.

..no, actually, we don't want this to run in any browser, regardless if it's ES5-spec-compliant.

boneskull · 2017-06-14T19:12:11Z

@ScottFreeCode @dasilvacontin I went thru and made a bunch of explanations. please let me know if you're still unsure about something

boneskull · 2017-06-14T19:13:47Z

the Object.defineProperty failure in IE8 would manifest itself as an exception, I believe. it only works on DOM nodes. so it exists, but...

To be clear, I'm not just handling quirks in IE7 and IE8; PhantomJS as well.

ScottFreeCode · 2017-06-14T19:30:29Z

Thanks!

Yeah, if it's going to be... complicated... to check that Object.defineProperty is not only available but correctly functioning, then we may as well just check for Object.create instead. Ideally we may want a comment in the code alluding to the issues checking for Object.defineProperty, but I'm not going to be a stickler about that.

I'm still unsure about the purpose of the other Object.create check -- the one in the checkGlobals suite I think it was.

boneskull · 2017-06-15T00:00:15Z

@dasilvacontin Speak now or forever hold your peas

dasilvacontin · 2017-06-16T19:46:22Z

I like my veggies.

…

On 15 Jun 2017, at 02:00, Christopher Hiller ***@***.***> wrote: @dasilvacontin Speak now or forever hold your peas ― You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

* load karma plugins explicitly; closes mochajs#2867 * don't use "should" for unit tests; closes mochajs#2882 * fix some broken browser tests * fix some problem tests in obsolete browsers * phantomjs fixes

boneskull force-pushed the issue/2867 branch from 7483f0d to 5db8121 Compare June 7, 2017 19:52

boneskull force-pushed the issue/2867 branch from 5db8121 to 778b4b6 Compare June 7, 2017 20:36

boneskull changed the title ~~obliterate package-lock.json after sanity test install; closes #2867~~ fix CI; closes #2867 Jun 7, 2017

boneskull self-assigned this Jun 7, 2017

boneskull added the qa label Jun 7, 2017

boneskull added this to the next-minor milestone Jun 7, 2017

popomore mentioned this pull request Jun 13, 2017

upgrade to [email protected]; closes #2859 #2860

Merged

boneskull force-pushed the issue/2867 branch from 778b4b6 to fd326d6 Compare June 14, 2017 00:07

boneskull force-pushed the issue/2867 branch from fd326d6 to c7abeca Compare June 14, 2017 02:31

boneskull added 2 commits June 13, 2017 21:34

load karma plugins explicitly; closes #2867

377b1bb

don't use "should" for unit tests; closes #2882

9cdb4c2

boneskull force-pushed the issue/2867 branch from bbdb993 to 9cdb4c2 Compare June 14, 2017 04:34

boneskull mentioned this pull request Jun 14, 2017

replace uses of should with expect.js #2882

Closed

fix some broken browser tests

fa0169b

fix some problem tests in obsolete browsers

31a8acf

boneskull force-pushed the issue/2867 branch from 8a8afc7 to 31a8acf Compare June 14, 2017 06:11

phantomjs fixes

1fe4b74

boneskull commented Jun 14, 2017

View reviewed changes

boneskull added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Jun 15, 2017

boneskull mentioned this pull request Jun 15, 2017

use @mocha/karma-sauce-launcher #2861

Closed

ScottFreeCode mentioned this pull request Jun 15, 2017

Cover Browser Tests (and all Node versions) #2886

Closed

boneskull merged commit 50fc47d into master Jun 16, 2017

boneskull deleted the issue/2867 branch June 16, 2017 19:33

boneskull removed their assignment Dec 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix CI; closes #2867 #2868

fix CI; closes #2867 #2868

boneskull commented Jun 7, 2017

boneskull commented Jun 7, 2017

coveralls commented Jun 7, 2017

boneskull commented Jun 7, 2017

coveralls commented Jun 7, 2017

boneskull commented Jun 7, 2017

coveralls commented Jun 7, 2017

boneskull commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

boneskull commented Jun 14, 2017

coveralls commented Jun 14, 2017

dasilvacontin commented Jun 14, 2017 via email

boneskull Jun 14, 2017

boneskull Jun 14, 2017

ScottFreeCode Jun 14, 2017

boneskull Jun 14, 2017

boneskull Jun 14, 2017

boneskull Jun 14, 2017

ScottFreeCode commented Jun 14, 2017

boneskull Jun 14, 2017

boneskull Jun 14, 2017

boneskull Jun 14, 2017

boneskull Jun 14, 2017

boneskull commented Jun 14, 2017

boneskull commented Jun 14, 2017

ScottFreeCode commented Jun 14, 2017

boneskull commented Jun 15, 2017

dasilvacontin commented Jun 16, 2017 via email

+                            return;
+                          }
+                          try {

fix CI; closes #2867 #2868

fix CI; closes #2867 #2868

Conversation

boneskull commented Jun 7, 2017

boneskull commented Jun 7, 2017

coveralls commented Jun 7, 2017

boneskull commented Jun 7, 2017

coveralls commented Jun 7, 2017

boneskull commented Jun 7, 2017

coveralls commented Jun 7, 2017

boneskull commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

boneskull commented Jun 14, 2017

coveralls commented Jun 14, 2017

dasilvacontin commented Jun 14, 2017 via email

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottFreeCode commented Jun 14, 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

boneskull commented Jun 14, 2017

boneskull commented Jun 14, 2017

ScottFreeCode commented Jun 14, 2017

boneskull commented Jun 15, 2017

dasilvacontin commented Jun 16, 2017 via email