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

Bug mocking with SWC #2528

Closed
assertnotnull opened this issue Jul 25, 2023 · 14 comments
Closed

Bug mocking with SWC #2528

assertnotnull opened this issue Jul 25, 2023 · 14 comments

Comments

@assertnotnull
Copy link

Describe the bug
Sinon is unable to mock the function when compiled with SWC but works with ts-node

To Reproduce
clone the sample repo with bug

pnpm i
pnpm test

Outcome:

1) main
       should mock:
     TypeError: Descriptor for property toBeMocked is non-configurable and non-writable
      at assertValidPropertyDescriptor (node_modules/.pnpm/sinon@15.2.0/node_modules/sinon/lib/sinon/stub.js:138:15)
      at Function.stub (node_modules/.pnpm/sinon@15.2.0/node_modules/sinon/lib/sinon/stub.js:89:5)
      at Sandbox.stub (node_modules/.pnpm/sinon@15.2.0/node_modules/sinon/lib/sinon/sandbox.js:389:39)
      at Context.<anonymous> (src/main.spec.ts:12:22)
      at processImmediate (node:internal/timers:476:21)

To see it working with ts-node replace "@swc/register" with "ts-node/register"
Then run pnpm test again and it passes.

Expected behavior
It should mock

@fatso83
Copy link
Contributor

fatso83 commented Jul 30, 2023

Hi and thanks for making a good reproduction case! Great to have a simple reproduction repository.

Sinon makes mocks, stubs and spies. Those are plain, normal functions, and we do not do any magic to the runtime, so whatever is to be done needs to be done using normal Javascript in the given runtime. Sinon clearly states what the issue is:

Descriptor for property toBeMocked is non-configurable and non-writable

If the transpiled code restricts everyone from modifying those exports, Sinon cannot do anything by itself. This issue is not an issue with Sinon, but with your transpiler tooling.

While ts-node clearly exports a writeable object descriptor, SWC does not. This is in-line with how ES Modules are supposed to work, so SWC seems to be the correct behavior here.

Just googling "swc writable object descriptor" comes up with a few suggestions, some ok workarounds. I would still try another approach, if you just need to do this to have mutable namespaces for your modules, and that is to use the Dalton's ESM loader. It has an option to make namespaces mutable and you can register it before your SWC module.

The option in ESM:

"mutableNamespace":true 

@fatso83 fatso83 closed this as completed Jul 30, 2023
@assertnotnull
Copy link
Author

I've tried the suggestion in stackoverflow as I also found this before but it's not working either.
Other libraries complexifies the tests too much that it outweighs the benefits.
I guess running tests with mocks is not an option with SWC.

@fatso83
Copy link
Contributor

fatso83 commented Aug 4, 2023

You can use mocks in the original sense, but nowadays mocking is more colloquially used as a term for intercepting loading of dependencies and replacing those with some test double (stub, fake, mock, dummy). That was easier when "everyone" was using derivatives of the CommonJS module system, either in Node or using some bundler like Browserify or Webpack. This was because the importing of other modules happened at runtime, so you could if/else different dependencies at runtime or hook into the module loading like proxyquire and similar did. This changes in 2015 when ES Modules were introduced, as they are resolved earlier and cannot be programmatically replaced like we used to, except if you do stuff to the runtime like Jest is doing; essentially transpiling everything to require calls . This is why we essentially had issues like this from 2015 (see #831, for instance).

Solutions

Replace module loader

Once you are in true ESM land (not transpiled to ES5 and/or CJS), you cannot mutate exports in the normal runtime. For that to be allowed, you need to hook into the process earlier, by replacing the module loader. This is actually been allowed in Node for ages (Node 13?) and used by various tools. One alternative to Sinon is TestDouble, which has most of the basic functionality of Sinon, but it also bundles its own module loader for use with ESM! You can either go all in on that, or just use the module loading.

Here's the link for documentation on how to use this (it's quite painless): td.replaceEsm()
You basically just need to supply an extra flag for node to use TD's module loader, and it should work. TD has very few dependencies, but one of them is Quibble, which is actually the library responsible for doing the module replacement. So you could alternatively just use that directly:

node --loader=quibble ...
# or mocha --loader=quibble ...

then

  await quibble.esm('./a-module.mjs', {life: 41}, 'replacement universe');

Check the docs.

P.S. By now, I actually cannot remember if using SWC results actual ES Modules (which have immutable namespace) or something else (seeing the object descriptor thingies above. Sorry. I have the memory of a goldfish), so this might or might not work for your 🤷 If it turns out the resulting transpiled code is not ESM you can just use proxyquire (Sinon guide).

Alternative to hooking into the link layer: pure dependency injection

Pure dependency injection (previously called "poor man's DI") is something I often reach for to avoid complicating my setup. You get a slight change in production code to enable better tests. You can see my describing a fuller example of this in #831 (comment)

Issues related to ESM stubbing: #831, #1623, #1711,
Issues related to CJS stubbing: #1248, #1648

@fatso83
Copy link
Contributor

fatso83 commented Aug 4, 2023

P.S. I have some extra time today, so thought I might as well try getting this working. Testing out Quibble now.

@fatso83
Copy link
Contributor

fatso83 commented Aug 4, 2023

Working fine! I forked your repo and got it working: https://github.com/fatso83/sinon-swc-bug/tree/cjs-mocking

Be aware, that this is relying on CommonJS modules. At first, I configured Quibble to do ESM replacements, but I was not able to get it working. The problem was that this was never ESM to begin with. Your .swcrc file explicitly specifies that the module system should be CommonJS. Furthermore, I found that the defaults for the SWC configuration targets ES5. Once I figured that I was really just working with CJS it took just a minute to get it working:

  it("should mock", () => {
    const mocked = sandbox.fake.returns("mocked");
    quibble("./other.ts", { toBeMocked: mocked });
    const {main} = require("./main");

    main();
    expect(mocked.called).to.be.true;
  });

P.S. This is the first time I have used Quibble (TestDouble). You could just as well have used Proxyquire, which is what we suggest in our how-to for replacing modules.

@fatso83
Copy link
Contributor

fatso83 commented Aug 4, 2023

Another approach is of course to just modify the output of SWC to make the exports mutable (writable object descriptors). The jest-workaround plugin for SWC does just this, so I opened magic-akari/swc_mut_cjs_exports#65 to ask if it was possible to re-use this in other test runners. That would make it re-usable in Ava, Mocha, etc.

The issue with non-mutable exports is the same for Webpack and other transpilers, for that matter.

@fatso83
Copy link
Contributor

fatso83 commented Aug 4, 2023

Some notes

I looked into how to configure a TS project into using an ESM (not-CommonJS module transpilation) based approach and ran into one problem: when configuring TypeScript to produce ES Modules, you need to specify file extensions in your imports (not a problem). You also need to make Node aware how the ts extension is to be handled when loading modules, as you will otherwise get TypeError: Unknown file extension ".ts" for /Users/myuser/code/sinon-swc-bug/src/main.spec.ts. That is usually handled by setting --loader=ts-node/esm, but you also need Quibble to be used as the loader for module replacement to work! While there is a project in Node dedicated to add support for using multiple loaders, this is not complete, so you cannot use two loaders today.

As you can currently only specify a single loader, you would then need to implement a custom loader that did the juggling of making the output of { load, resolve } in one loader work together with the same ones from another loader. I stumbled over this example, which could be used as a starting point. I do not intend to do that right now 😄

So if using SWC, I would configure your TS project to use CJS as the module target for the time being (which is the default!), if possible and just use the approach I showed above (in the cjs-mocking branch).

If you do not need TypeScript and just need to generally stub ESM, I put a working ESM example in the
esm-no-ts branch.

https://github.com/fatso83/sinon-swc-bug/tree/cjs-mocking

@fatso83
Copy link
Contributor

fatso83 commented Aug 9, 2023

@assertnotnull Did you see the fix/workaround I provided for you? Was just wondering if that worked fine, as I was thinking of updating the old link seams article with examples for modern bundlers/transpilers, as this comes up quite a bit.

@fatso83
Copy link
Contributor

fatso83 commented Aug 9, 2023

I was also thinking that it might be useful to have error messages point directly to relevant docs? So instead of "Descriptor for property toBeMocked is non-configurable and non-writable" change it to something like

The descriptor for the property toBeMocked is non-configurable and non-writable. Sinon cannot do anything about this, as it is essentially immutable. Read https://sinonjs.org/faqs#immutable-objectdescriptor for tips on what could cause this and how to possibly resolve it.

(The link is made up by the way).

@fatso83
Copy link
Contributor

fatso83 commented Aug 11, 2023

OK, I have found yet another option that works: the SWC plugin that is confusingly called jest_workaround, which makes the exports mutable. Super easy. See my example:

https://github.com/fatso83/sinon-swc-bug/tree/swc-with-mutable-exports

The issue is that the exports of import * as Other from './other' are getters, not fields, but since these getters are actually configurable (due to the plugin), I just instructed Sinon to replace the getter.

There is also another option I am working on.

In any case, #2534 needs to fixed as well, though it's not blocking (as you can manually restore the accessor stubs).

I will try to gather all of these options in an article on the Sinon documentation page, if time be willing.

@assertnotnull
Copy link
Author

Thank you @fatso83 for spending much time looking into this. I find this is quite complicated to use different loaders, specially if such change was applied to a big project but the last solution is interesting. I will give it a try.

@fatso83
Copy link
Contributor

fatso83 commented Aug 22, 2023

Good to hear from you! I did write that article in the end, btw, but no-one has commented and reviewed my PR. I should contain all the details one could need. Would you have a chance at reviewing it to see if this looks useful for people?

It's #2540. I attached/linked the article as PDF for easy reading

@assertnotnull
Copy link
Author

The changes https://github.com/fatso83/sinon-swc-bug/tree/swc-with-mutable-exports works!
I only had to modify the definition of mocks in the beforeEach and add the plugin!
image

@assertnotnull
Copy link
Author

I must add, this is in a Nestjs project where Jest has been replaced with Mocha + Sinon. We faced memory leaks with Jest in the past.

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

No branches or pull requests

2 participants