sandbox.assert.fail and sandbox.assert.pass affect global sinon configuration

[kim366]

Describe the bug
sandbox.assert.fail and sandbox.assert.pass seem to not only affect the current sandbox, but the entire sinon environment. This becomes an issue when tests run concurrently, as they do in, e.g., AVA.

To Reproduce
Steps to reproduce the behavior:
I created this function. When used in two AVA tests that run concurrently, AVA will either report Error: Test finished without running any assertions or Error: Assertion passed, but test has already finished.

export function useSinon(t: ExecutionContext) {
  const sandbox = createSandbox();
  sandbox.assert.fail = msg => t.fail(msg);
  sandbox.assert.pass = () => t.pass();
  t.teardown(() => sandbox.restore());
  sandbox.useFakeTimers(<any>{global});

  return sandbox;
}

Expected behavior
The fail and pass bindings should only affect the current sandbox, that way they don't get overridden by concurrent tests.

Context

• Library version: 9.1.0*
• Environment: Node.js 14.11.0
• Other libraries you are using: AVA 3.13.0

[mroderick]

Is there any chance you can make this a runnable example?
Also, could you make it a JavaScript example and not TypeScript?

[kim366]

@mroderick Here you go: https://github.com/kim366/sinon-issue-concurrent. If you mark the tests with test.serial, they will pass. While they run concurrently, one does not pass, because t is rebound to be the other test's context

[fatso83]

Great bug report with repro! Fearing that this might be tightly related to understanding AVA as well.

While they run concurrently, one does not pass, because t is rebound to be the other test's context

🤔 ... 🤯 Could you elaborate on that? Not sure I understand what you mean wrt how this t is being rebound. I don't see t being passed around anywhere.

[kim366]

@fatso83 Thank you. t (AVA's execution context) is being passed from the test case to useSinon.

Let's call the t from foo tFoo and the other one tBar. Here is what happens:

1. foo binds sandbox.assert.pass to tFoo.
2. bar binds sandbox.assert.pass to tBar (which overwrites the first assignment).
3. 20ms pass
4. foo finishes and calls sandbox.assert.pass, which in turn calls tBar.pass().
5. foo fais, because tFoo.pass() was never called.

This is the least bad of the symptoms of this. It is also possible that in the output it tells you that another test failed! This is extremely confusing.

When I set sandbox.assert.pass intuitively I think it is bound to this sandbox, but it is not; it's bound globally. I think this should be changed.

[kim366]

sinon/lib/sinon/sandbox.js

Line 413 in 332485a

Sandbox.prototype.assert = sinonAssert;

The global assert is just being assigned to the sandbox

[fatso83]

Great find! This should be very possible to do something with. It's supposed to be the other way around: sinon's internal sandbox should of course assign its assert to sinon.assert, not the other way around.

[kim366]

Well, I think the changes necessary are minimal. Instead of assert, it should do aomething along the lines of exporting a function createAssertObject that does all the assignments and then module.exports.assert = createAssertObject. sinon.assert then stays the same, but whenever a sandbox is created it calla this function to get its unique assertion object

[kim366]

I’ll see if I get around to doing a PR, but I’ll first have to understand the sinon project structure.

Update: unfortunately I know old-style JS too little to know how do this without messing with imports.

[fatso83]

"Old style JS" is just JS, you know :) There is not much in the source code that would be different if using ES2020+, since it simply relies on on knowing how Javascript works and utilizes the mechanics of ECMAScript's prototypal nature. Hacking on Sinon made me understand how ES2015 classes actually worked under the hood, so it's a good exercise in becoming a better JS developer. Not sure what you mean by messing with imports, btw? In CommonJS style modules, like in Node, exports are bound at runtime, not at link time, so you can do quite a lot of manipulation that you cannot do with ES Modules. Not sure how that applies here, though ... ?

[kim366]

Okay, sure, I'll try it again :) My problem was that I wanted to do module.exports = createAssertObject(); module.exports.createAssertObject = createAssertObject. But it told me cannot set property createAssertObject of undefined. The same when I first created an instance and assigned it to that. I have the unit test ready, though, so that's the only thing holding me back

[fatso83]

@kim366 If you push your fork somewhere public and give the link I can have a look.

[kim366]

I got it to work, in the end :) sorry for that

[mroderick]

This has been fixed with #2302