Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(base.spec.js): Fix Base reporter test failure due to timeout #3527

Merged
merged 1 commit into from
Oct 24, 2018

Conversation

plroebuck
Copy link
Contributor

@plroebuck plroebuck commented Oct 20, 2018

Description of the Change

The Base reporter's specification verifies reporter can interpret Chai custom error messages.
This test takes ~480ms on my somewhat memory-starved computer, which is way too close to the 500ms timeout.
Change doubles this timeout.

Alternate Designs

  • Could move the Chai require (which is likely culprit), but its scope would expand.
  • Could increase timeout, but retain the existing Chai require's test scope.

Benefits

Prevention of random test failure due to timing issue.
Failure strikes often on Appveyor (for me anyway).

Possible Drawbacks

None to knowledge.

Applicable issues

Fixes #3524
semver-patch

… to timeout

The `Base` reporter's specification verifies reporter can interpret Chai custom error messages. This
test takes ~480ms (lightly loaded CPU), which is _way_ too close to the 500ms timeout. Change
doubles this timeout.

mochajs#3524
@plroebuck plroebuck added qa semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Oct 20, 2018
@plroebuck plroebuck self-assigned this Oct 20, 2018
@@ -312,6 +312,7 @@ describe('Base reporter', function() {
});

it('should interpret Chai custom error messages', function() {
this.timeout(1000);
var chaiExpect = require('chai').expect;
Copy link
Contributor

@outsideris outsideris Oct 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead increasing timeout, how about move require('chai') to outside it? Requiring chai takes most of time.

var chaiExpect = require('chai').expect;
it('should interpret Chai custom error messages', function() {
  try {
    chaiExpect(43, 'custom error message').to.equal(42);
  } catch (err) {
...

In my local, original test time took 46ms but moving require('chai') took only 8ms.

Copy link
Contributor Author

@plroebuck plroebuck Oct 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boneskull went to the effort to minimize the scope to just this test.
This was considered, but didn't wish to increase its scope.

I have no idea about possible interactions between Chai and Unexpected, and have no desire to go down that rabbit hole. But you can borrow my flashlight...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What interactions between chai and unexpected?

@plroebuck
Copy link
Contributor Author

@boneskull, please weigh in.
I am so sick of seeing AppVeyor builds fail due to this. I want something merged to fix it!
I see no problem with the existing fix (lacking further input).

@boneskull
Copy link
Contributor

lgtm, thanks

@boneskull boneskull merged commit adb1f61 into mochajs:master Oct 24, 2018
@plroebuck plroebuck deleted the issue/3524 branch October 24, 2018 20:37
@boneskull boneskull added this to the v6.0.0 milestone Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Base reporter test failure due to timeout
3 participants