Sinon: mockBrowser.verify() not passing when doing a minor update to 7.4.1

[Tchiller]

Describe the bug
While trying to update the sinon package for our project, we got one test broken after a minor update.

t will then fail on this test on line 52:

    it('redirects to user chat page', () => {
       mockBrowser
         .expects('goTo')
         .once()
         .withArgs(`/?page=msg&cid=${conversationId}`)

       script.chat(fsId)
       server.respond()
     })

in its afterEach hook:

1) script
"after each" hook for "redirects to user chat page":
ExpectationError: Expected goTo(/?page=msg&cid=10[, ...]) once (never called)

when doing
mockBrowser.verify()

To Reproduce

npm install yarn -g
git clone https://gitlab.com/foodsharing-dev/foodsharing.git
cd foodsharing/client
yarn install
yarn upgrade sinon@latest
yarn test

Expected behavior
test should still run after minor update.

Screenshots

Context (please complete the following information):

• Library version: 7.4.1 (still valid for 8.1.1)

Thank you all.

[fatso83]

Excellent bug report, and an easily reproducible test case to boot! I think we should be able to find the regression easily using one of our git bisect test script. Not sure if the issue is in sinon or the fake server (previously: nise) sub project, but that should surface quickly enough.

[mroderick]

@Tchiller have you tried using the git bisect test script mentioned above to determine which was the first failing version?

[Tchiller]

Thank you, @fatso83. We are running on nise 1.5.3 currently.
@mroderick no, I did not. I would have to download the sinon project on my own to bisect each commit? Beacuse I already did spot the minor version step where it happend. (from 7.3.2 to 7.4.0 or 7.4.1).

TBH, I do not feel comfortable in JS at all :-| I think, running my local sinon would be harder than staying on this version... Sorry, for not bringing more skills. In any case, thank you, @mroderick

[fatso83]

Finally have a few hours to look at this. While I was able to reproduce your basic test case, I am still struggling a bit to do so using the source package of sinon checked out (using yarn link to use the local version), so there is some finicky details I need to get right in the test script I use for git bisect. Hopefully on the right track :-)

I would have to download the sinon project on my own to bisect each commit? Beacuse I already did spot the minor version step where it happend. (from 7.3.2 to 7.4.0 or 7.4.1).

Yeah, knowing that it broke approximately around these versions is a great time saver, but to fix the issue I need to know what is causing it, and finding the exact commit makes that a lot easier. I might still need to debug manually in the end, but in 9/10 cases I can usally see what is wrong by the git show output.

A good test script for git bisect basically implements the manual steps needed to clean the state between the different versions and then run a full test, making it possible to run on its own, as sometimes the process of doing a single test of one commit can take minutes. For instance, due to the amount of dependencies, Webpack, my slow laptop and the terrible I/O performance of WSL a single run of doing rm -r node_modules; yarn install; yarn test takes 3m 16s 😅 That means testing even 5 commits might involve a lot of waiting around, as you often repeat the process a few times until you get the script right (reproduce the issue).

TBH, I do not feel comfortable in JS at all :-| I think, running my local sinon would be harder than staying on this version..

TBH, modifying this template does not require any JS at all, just adding the right cli commands. And you already made a better bug report than most people, so judging by that, I think you would have gotten it right 😄

[fatso83]

OK, it took me two hours to realize yarn link is useless for anything, but I found yalc in the process, so I finally made a script that made it easier to test for the breaking change. It was the adding of the browser field in fe7347e6. Not sure why that had an effect, though ...

[fatso83]

So the thing that is happening is that you are not experiencing a bug _per se in_ Sinon. Rather it's a side-effect of #2060, which _actually_ was meant to improve things for people using Webpack to run test builds in the browser. That PR added a browser field:

If your module is meant to be used client-side the browser field should be used instead of the main field. This is helpful to hint users that it might rely on primitives that aren’t available in Node.js modules. (e.g. window)

The browser field was added in response to people experiencing trouble after we added the module field. The module field, in turn, points to an ES Module build of Sinon, but it caused problems for some people using Webpack (which prefers ES Modules over Common JS), as that build contained syntax that is not valid in ES 5 browsers, and as external modules such as Sinon is not transpiled by default, this caused errors when the resulting builds were run in browsers such as IE11. The browser field remedies this situation by pointing to a starting point that should be used for bundles running in the browser.

You started using Sinon in July 2018, while we added the module field in May 2018, so you have used the module field all along, since Webpack prefers entry points in this order: browser > module > main. That module field points to pkg/sinon-esm.js.

You can override the behavior of preferring web builds by default in Webpack by setting a target option in your webpack config, but I tried this, and setting target: 'node' ends up with errors when running your test bundle (something about Vue SSR).

The reason is probably related to your environment being neither this nor that.

You are testing your Webpack build for _browsers_ in a _server environment_, not a browser. This is not something we pretend to support, so it's not really a bug with our setup. Now, while we do not explicitly support this and I do not presume to know what is _really_ wrong*, I do have a hack that makes it continue to work. After all, the only problem here is that Webpack chooses the "wrong" file when importing Sinon! That is where Webpack's alias feature can help and adding this patch to your webpack config for tests work even when using the latest Sinon:

diff --git a/client/webpack.test.config.js b/client/webpack.test.config.js
index e020d678c..6c56e1b50 100644
--- a/client/webpack.test.config.js
+++ b/client/webpack.test.config.js
@@ -17,6 +17,11 @@ module.exports = merge(webpackBase, {     path: resolve('test'),     filename: '_compiled.js'   },
+  resolve: {
+    alias: {
+      sinon: path.resolve(__dirname, 'node_modules/sinon/pkg/sinon-esm.js')
+    }
+  },   
    module: {
     rules: [
       {

(*) I am guessing it has something to do with Webpack patching process or affecting timers in a way that might deal with your promise library.

[mroderick]

Thank you @fatso83 for your time and effort into investigating and explaining this challenge ⭐️

[Tchiller]

Dear @fatso83, thank you very much for your time and help. But I fear, it is not working still.
I have applied your patch and get the very same error.
Do you have some typo in your snippet?

[fatso83]

@Tchiller Not sure what I did to get it working, because I certainly did not get it working this time. I did use some time to manually verify the aliasing did work, though, so you do get the right file, but it failed regardless of using pkg/sinon.js, pkg/sinon-esm.js or lib/sinonjs.

So I'll reopen for now.

[fatso83]

Heh. Got confused by version branching 👎 I think was due to something not being pushed and needing to push the changelog for 7.4.0 after 7.4.1 had been released. I am diving down again.

$ git tag --contains v7.4.0
v7.4.0

carlerik at ubuntu-i9-vm in ~/dev/sinon ((v7.4.1))
$ git tag --contains v7.4.1
v7.4.1
v7.4.2
v7.5.0
v8.0.0
v8.0.1
v8.0.2
v8.0.3
v8.0.4
v8.1.0
v8.1.1
v9.0.0
v9.0.1

[fatso83]

OK, tested this again.
It was still the same commit, fe7347e, that made the difference. And the fix I showed for webpack does in fact work for 7.4.1, 7.42 and 7.5.0. So that works.

Unfortunately, your tests starts failing again at version 8.0.0, so that's something else ... It's the same test, though, pointing to something environment specific (again, guessing at timers/promises).

Re-running the bisect script, this time from v7.5-v8.0, but with the webpack patch applied.

[fatso83]

This is weird:

155cf8ff3df1fd068214e9258fead4b393ad31c7 is the first bad commit
commit 155cf8ff3df1fd068214e9258fead4b393ad31c7
Author: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Date:   Mon Sep 30 08:58:41 2019 +0000

    Bump rollup-plugin-commonjs from 9.2.0 to 9.3.4

It is true that this is failing on that exact test. The commit before it passes. Why? Totally unable to see it from the changelog in Rollup: https://github.com/rollup/rollup-plugin-commonjs/compare/v9.2.0...v9.3.4

[fatso83]

Instead of pursuing the whole thing with "which dependency broke in v8 and why" (where I did not really get to the end) I just chose to manually debug the test case and try to see what was different between a failing test and a passing test. To do that I basically toggled the fix I proposed above, so that webpack would either use the pre-built sinon-esm.js file or the default /lib/sinon.js. The difference was basically that when tests were passing Sinon would fake global.window.XMLHttpRequest and leave global.XMLHttpRequest alone. In the failing test it would be the reverse: global.window.XMLHttpRequest would be untouched and global.XMLHttpRequest would be faked. Since your code uses jQuery.ajax, which in turn uses window.XMLHttpRequest, only the first approach would ever work.

When loading the packaged file, something is clearly different when encountering global than when you load the normal module. This is where the mystery of what broke between 7.5 and 8.0 was revealed after inspecting how sinon-esm.js was built: it changed 6 months ago in 1c607a2c. Before this, it was built using Rollup, which would create a build suitable for browsers where it would substitute typical Node globals with browser globals. So that global probably is equal to window in such a build. 1c607a2c changed that when we dropped Rollup in favor of using Browserify for every build and also use our own @sinonjs/commons when asking for global. After that commit, when asking for require(@sinonjs/commons).global we consistently know that it is global when running in Node and window when running in a Browser. But when you create something in between, like you did with Node+JSDOM - a fake browser inside Node, that doesn't really work that well, as we will detect that as always being a Node environment and export global as the global, not the window of JSDOM.

And which versions does this apply to? As we thought:

git tag --contains 1c607a2c
v8.0.0
v8.0.1
v8.0.2
v8.0.3
v8.0.4
v8.1.0
v8.1.1
v9.0.0
v9.0.1

So basically 7.5.0 is the last version of Sinon where your setup (using that webpack alias) would work. It might be possible to work around it somehow, but I think that would be really hacky (wrapping @sinonjs/commons to export window as .global).

[Tchiller]

thank you so much. I have successfully upgraded us to 7.5 now and will rebuild the culprit test asap.
Your effort is very much appreciated.

[fatso83]

I just realised when thinking about this that nise is victim to a lack of feature parity. In Lolex (now renamed to @sinonjs/fake-timers) we deal with issues like this by allowing you to override what the target is, for instance by passing in a target property to the optional config to createFakeServer(). That would solve issues like this and look something like:

const { window } = new JSDOM(`...`);
global.window = window;
// .... some time later
sinon.createFakeServer({target: window});

If you are interested in such a thing, you can file an issue with the nise project. Creating it would probably not be a lot of effort, if you were to try ;)

[fatso83]

You can track https://github.com/sinonjs/nise/issues/149.