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

Make sandboxes each use their own assert object #2302

Merged
merged 2 commits into from
Oct 13, 2020
Merged

Conversation

kim366
Copy link
Contributor

@kim366 kim366 commented Oct 9, 2020

Purpose (TL;DR) - mandatory

Fix issue #2298 by having assertion objects be created by each sandbox instead of using the global one

Intuitively you would think that sandboxes use isolated assertion objects. But this was not the case. If you overrode the failException or pass/fail functions, they would apply to all sandboxes and all assertions. This particularly was an issue with concurrent testing, such as with AVA.

We no longer create and export a single assert object, instead there is a createAssertionObject function that is used inside the sandboxes' constructor.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm test

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

PS: on github the diff shows very poorly. In actuality only ~3 lines changed and a test case was added, but a buch of lines were indented one step.

@@ -37,6 +37,8 @@ function Sandbox() {
var fakeRestorers = [];
var promiseLib;

sandbox.assert = sinonAssert.createAssertObject();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

test/sandbox-test.js Show resolved Hide resolved
Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

All good.

@fatso83
Copy link
Contributor

fatso83 commented Oct 13, 2020

Thanks for the changes: a very welcome improvement, Kim! 💯


Just a tip: if you think the diff is messier than what is actually happening, you can suppress all whitespace changes by passing a query parameter: ?w=1. This is equivalent to git log -w or git diff -w. So your diff would look like this.

@fatso83 fatso83 merged commit 60e2739 into sinonjs:master Oct 13, 2020
@kim366
Copy link
Contributor Author

kim366 commented Oct 13, 2020

Ok, so no refute.exception. Glad to help!

@mroderick
Copy link
Member

Very nice PR, thank you 🍰

@mroderick
Copy link
Member

This has been published with sinon@9.2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants