Sinon Chai calledWithMatch causes structuredClone to error in the test runner

[Sanderovich]

See this repo for a reproduction https://github.com/Sanderovich/web-test-runner-called-with-match-error-reproduction.

When an expect(spy).to.be.calledWithMatch() is unsuccesful the expect throws an AssertionError that web test runner can not handle which causes the following exception.

The crash seems to happen because of the actual property inside the AssertionError. The actual property is set to a function called proxy and structuredClone cannot handle functions (https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm).

A possible solution could be to JSON.stringify() and JSON.parse() the AssertionError before the structuredClone.

[CendioOssman]

This is blocking our (@noVNC) migration to web test runner, as we make heavy use of sinon. :/

Have you found any way to work around the issue?

[Sanderovich]

@CendioOssman A quick work around would be to delete the actual property from the error thrown by the expect on failure and re-throw the error so web-test-runner can handle it.

it('stops working when errors', () => {
	const sinonSpy = spy();

	sinonSpy();

	try {
		expect(sinonSpy).to.be.calledWithMatch({ foo: 'bar' });
	} catch (e) {
		delete e.actual;
		throw e;
	}
});

[CendioOssman]

Thanks. That doesn't scale terribly well. And we have 765 such assertions. Any workaround would need to be somewhere more central. :/

[Sanderovich]

For a central work around I would implement the work around in a Chai plugin:

import { use, Assertion } from 'chai';
import sinonChai from 'sinon-chai';

function overrideCallWithMatch() {
	Assertion.overwriteMethod('calledWithMatch', _super => {
		return function() {
			try {
				_super.apply(this, arguments)
			} catch (error) {
				delete error.actual;
				throw error;
			}
		}
	});
}

use(sinonChai);
use(overrideCallWithMatch);

Depending on for which Sinon Chai methods you want to implement the work around you can extend the plugin. Would this help you @CendioOssman ?

[lideen]

I have the same issue. This is an issue with any value that structuredClone cannot handle. In my projects tests started failing for HTMLElements as well as they also cannot be cloned.

My current workaround is to lock @web/[email protected]

[rschristian]

I was running into this with sinon-chai but saw no errors in the browser console at all -- seemingly just a failed test that wasn't properly detected. Wasn't using calledWithMatch either.

Both workarounds (wrapper and pinning to @web/[email protected]) worked for me though.

[CendioOssman]

Depending on for which Sinon Chai methods you want to implement the work around you can extend the plugin. Would this help you @CendioOssman ?

Possibly. It is a bit annoying that you have to do it for a bunch of methods, though. But I supposed that can be optimized a bit.

My current workaround is to lock @web/[email protected]

This workaround does not work for me. Is there a regression in 0.7.2 that causes this problem?

[lideen]

Depending on for which Sinon Chai methods you want to implement the work around you can extend the plugin. Would this help you @CendioOssman ?

Possibly. It is a bit annoying that you have to do it for a bunch of methods, though. But I supposed that can be optimized a bit.

My current workaround is to lock @web/[email protected]

This workaround does not work for me. Is there a regression in 0.7.2 that causes this problem?

I think it is this change that causes the problem: https://github.com/modernweb-dev/web/commit/4a4b6995350eaef920d85924ffbf99a030c231e9. To be more precise, the usage of structuredClone in packages/dev-server-core/src/web-sockets/webSocketsPlugin.ts

[CendioOssman]

Hmm.. Very odd that it didn't resolve the issue here for me in that case.

@lucaelin, is this issue something you are aware of and working on?

[lucaelin]

Hey, thanks for bringing this second issue to my attention, I will see what I can do this weekend,

[lucaelin]

From my initial analysis there are multiple different issues at play here:

1. Circular Objects (which the original implementation solved)
2. Frozen objects, like Redux state (which got fixed in 0.7.2)
3. Objects with getter-only properties, like window (which I don't think ever worked)
4. Non-Transferable objects, like HTMLElement (which broke in 0.7.2)

Fixing 4) should be as simple as catching the clone error and running the original serialization, BUT this would only work if the .actual is not also frozen somewhere within its tree (but I assume that this is an edge case) Fixing 3) on the other hand, I don't know how to approach, especially since we probably want to avoid mutation on window during serialization, and these objects may very well be mix of all 4 cases simultaneously. Maybe we could detect these and instead replace them with some special keyword, like '[unserializable]', analog to the existing '[Circular]' keyword, but I have no idea on how to reliably detect those.

[bashmish]

@lucaelin that's great analyses, thanks!

do you think you can write some unit tests for each of this use-cases that we need to support? that would be a great way to start a PR, I'd really appreciate it

we can then search for some better algorithms for serialization, the one we use currently is taken from https://github.com/davidmarkclements/fast-safe-stringify which hasn't been update for 3 years now, so probably there are better alternativies in the open source world now as a last resort, we can also use [unserializable] in place of objects that can't be serialized to anything useful, or at least until there is a better way, so that we can unblock people now

[lucaelin]

@bashmish Just did, see the draft PR I created. I already had those sitting around from my analysis. I wrote a message on the discord back then, asking how to proceed with this issue, let me paste it here for reference:

Original Discord message

I could use some second opinions on this though. As described in my comment on the issue there are 4 cases we need to consider as potential actual values in the message. My problem right now is that the whole serialization implementation makes use of mutation, which in case of objects like window, isn't really ideal and additionally impossible in case of immutability. I previously fixed the immutability by using structuredClone, but that obviously causes problems with non-transferable objects. Looking at this again I think a better way to solve this is to avoid mutation, but this would require replacing the entire algorithm, which may bring other issues to the table. How do you feel about this? I could try to modify the algorithm to shallow clone the problematic objects instead of deep cloning ahead of time? What do you think?

This is the problematic code: https://github.com/modernweb-dev/web/blob/master/packages/dev-server-core/src/web-sockets/webSocketsPlugin.ts#L62

we can also use [unserializable] in place of objects that can't be serialized to anything useful

The issue would be reliably detecting whats serializable and what is not, walking down into each child node and replacing only those that are not. This gets a bit messy, because it causes more mutation on those objects and might cause further problems down the line.

[robrez]

I've been having trouble with "hanging tests", too. I've found that tests that use a sinon stub or spy will hang if there is an assertion to expect the stub/spy was called and it was not called. Edit -- to be clear, there is no "called with" assertion.. just "called" or "calledOnce"

The same underlying exception is found in browser console when using "watch" to "debug in the browser" (failed to execute structuredClone on window)

I've been failing to "overwriteMethod" globally for all tests (trying in wtr config mjs)... possibly due to using open-wc test helpers and bundled chai (expect) versus chai directly like the example given above. possibly because I don't have a dependency on 'sinon-chai' so omitted its 'use' statement... Although, I do have transitive types for 'sinon-chai' from open-wc/testing

[stee-re]

So I've encountered this same (very frustrating) issue too, however, not with Sinon, but with a simple Assert statement. E.g.

customElements.define('my-test', MyTest);

describe('my pain', () => {
  let component: MyTest;

  beforeEach(async () => {
    const myDoc = new DOMParser().parseFromString(myDocStr, 'application/xml');
    component = await fixture(html`<my-test></my-test>`);
    component.element = myDoc.documentElement;
  });

  it('will fail (as intended) but timeout', () => {
    //structuredClone will throw a DataCloneError here because of the XMLDocument
    expect(component).to.have.property('element', undefined);
  });
});

If you can see past the wierdness of this test and just see that a test is failing (which is ok) but when the runner tries to report this test failure, it will fail to do so. structuredClone will not be able to clone the failing tests payload and the exception will not help the developer find out which test caused this or why. The test run will simply timeout.

I explored a few work arounds most of which I didn't like:

• Chai's "property" method can be "interferred with". Fixes one potential area were this might blow up, but not a great solution.
• Stick to @web/dev-server-core: 0.7.1. I have verified that rolling back to @web/dev-server-core: "0.7.1"will get you out of the doo doo, but you really must make sure no other dependencies are pulling in newer versions - which is a bit messy. For anyone really wanting to go down that route, you need to check your package-lock (or equv) to make sure your test runner doesn't end up loading the newer 1.7.2. E.g. In your package.json you can add some overrides to pin a specific version like so:

  "overrides": {
    "@web/dev-server-rollup": {
      "@web/dev-server-core": "0.7.1"
    },
    "@web/test-runner-core": {
      "@web/dev-server-core": "0.7.1"
    }
  },

This doesn't feel good for me, cos you're mixing versions of modules that perhaps aren't tested - you may end up swapping this problem for others.

• Solution I'm going with (for now), which I must thank @web-padawan for linking to (the vaadion fix). This worked well.
  • It requires no messing around with pinning module versions
  • the fix is very "to the point" and hopfully points to the replacement library that perhaps this module could consider.

Add @ungap/structured-clone to your devDep's and add (or amend) the testRunnerHtml in your web-test-runner.js with the following:

export default /** @type {import("@web/test-runner").TestRunnerConfig} */ ({
  
  //your other config stuff...

  testRunnerHtml: testFramework =>
    `<!DOCTYPE html>
    <html>
    <head>
      <meta charset="utf-8">
      <title>Test Runner</title>
    </head>
    <body>
      <!-- Script to replace window.structuredClone with @ungap/structured-clone (in lossy mode) -->
      <script type="module">       
        import structuredClone from '@ungap/structured-clone';
        window.structuredClone = (value) => structuredClone(value, { lossy: true });
      </script>
      <script type="module" src="${testFramework}"></script>
    </body>
    </html>`,

});

Hope this saves someone else loosing as much time as I did to this issue.