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

fix: make it().timeout() work again #4599

Merged
merged 3 commits into from Mar 12, 2021
Merged

fix: make it().timeout() work again #4599

merged 3 commits into from Mar 12, 2021

Conversation

alexander-fenster
Copy link
Contributor

@alexander-fenster alexander-fenster commented Mar 8, 2021

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

Fixes #4598. The code change in #4574 had a breaking change: constructs like it(...).timeout(...), it(...).retries(...), etc. stopped working.

Replacing exports.it = context.it || context.test; with

exports.it = function(...args) {
  (currentContext.it || currentContext.test).apply(this, args);
};

changes the return value of it() to undefined, thus introducing a breaking change in the patch release.

This PR brings this functionality back, and adds an integration test.

Alternate Designs

None. This is a breaking change in a patch release. The behavior, albeit undocumented officially, is widely used (see this StackOverflow answer, for example).

Why should this be in core?

Just bringing back the old behavior.

Benefits

No breaking change :)

Possible Drawbacks

None that I'm aware of.

Applicable issues

#4598

@coveralls
Copy link

coveralls commented Mar 8, 2021

Coverage Status

Coverage remained the same at 94.155% when pulling 080699f on alexander-fenster:it-timeout into c6f874f on mochajs:master.

Copy link
Contributor

@giltayar giltayar left a comment

Choose a reason for hiding this comment

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

LGTM. We should have thought to proxy the return value too in the PR that provoked this fix...

@juergba
Copy link
Member

juergba commented Mar 9, 2021

@giltayar #4574 did break it() without requireing or importing it. Could you explain to me, please?

@juergba
Copy link
Member

juergba commented Mar 9, 2021

@giltayar #4574 did break it() without requireing or importing it. Could you explain to me, please?

No sorry, I'm wrong. But then your test doesn't prove that your fix is working, @alexander-fenster.

@alexander-fenster
Copy link
Contributor Author

@juergba Oops, you're right. I updated the test fixture to do const {describe, it} = require('../../../index.js'); and verified it fails without added returns and passes with them.

@juergba juergba added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Mar 10, 2021
@juergba juergba added this to the next milestone Mar 10, 2021
Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@alexander-fenster thanks for this PR.

I would prefer putting the tests to a different location.
We already have some tests for our require interface (from #4574):

  • test/integration/common-js-require.spec.js
  • test/integration/fixtures/common-js-require.fixture.js

Please just append a timeout and retries test to our existing tests. Thank you.

lib/mocha.js Outdated Show resolved Hide resolved
@alexander-fenster
Copy link
Contributor Author

@juergba Done, please take a look.

Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@alexander-fenster thank you.
The cancelled browser test is off-topic.

This was referenced Mar 15, 2021
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" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mocha 8.3.1 breaks ability to add configuration to tail of it blocks
5 participants