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

Can't stub the module (object) function in the latest update #2514

Closed
eugenet8k opened this issue May 15, 2023 · 5 comments
Closed

Can't stub the module (object) function in the latest update #2514

eugenet8k opened this issue May 15, 2023 · 5 comments

Comments

@eugenet8k
Copy link

Describe the bug
After #2508 was merged and the new sinon v15.0.4 is published a lot of code in our project stopped working. We can an error when trying to stub the object function in a special case. The same approach work in v15.0.2

The example code that shows the problem based on attempt to stub ChaplinJS utils object.

import Chaplin from "chaplin";
import sinon from "sinon";
import routes from "./routes";

new Chaplin.Application({ routes });

console.log("before stub", Chaplin.utils.redirectTo({ url: "/test/" }));

const sandbox = sinon.createSandbox();
sandbox.stub(Chaplin.utils, "redirectTo").callsFake(() => "stubbed");

console.log("after stub", Chaplin.utils.redirectTo({ url: "/test/" }));

To Reproduce
Open code sandbox based on v15.0.4 and see console log https://codesandbox.io/p/sandbox/sinon-module-stub-broken-15-0-4-9e1f4u

Console:

before stub true
bootstrap:27 Uncaught TypeError: Cannot redefine property: redirectTo
    at Function.defineProperty (<anonymous>)
    at wrapMethod (sinon-esm.js:4607:16)
    at Function.stub (sinon-esm.js:3803:44)
    at Sandbox.stub (sinon-esm.js:3247:39)
    at ./src/index.js (index.js:10:9)
    at __webpack_require__ (bootstrap:24:1)
    at startup:6:1
    at startup:6:1

Expected behavior
Open code sandbox based on v15.0.2 and see console log https://codesandbox.io/p/sandbox/sinon-module-stub-working-15-0-2-tipumo

Console:

before stub true
index.js:12 after stub stubbed
@fatso83
Copy link
Contributor

fatso83 commented May 16, 2023

Yup, seeing the descriptor is non-configurable, so need to look into the previous version to understand how this could have worked before.

Skjermbilde 2023-05-16 kl  09 03 37

@fatso83
Copy link
Contributor

fatso83 commented May 16, 2023

Found the issue. Previously we the object descriptor was pretty much left unchanged (apart from the value prop), but leaving configurable: false on a stub would make it impossible to remove, so this was added in 15.0.4 to make it possible to create stubs that would shadow a method on the prototype.

Unfortunately, it introduced a regression for cases where the object descriptor was not from the prototype, but the object itself. If configurable is false then it cannot be changed back into something else, which is why it throws. Will add a regression test and fix asap with a check for isOwn before modifying the descriptor.

@fatso83
Copy link
Contributor

fatso83 commented May 16, 2023

The sandboxes were fine, but I was unable to get them running locally as unit tests as they depended on window existing. If you want to test out the fix in #2515 and see that it fixes your issue, you can just have a look at the diff and copy it manually into the right section (the line it fails) in node_modules/sinon/pkg/sinon-esm.js:

-        methodDesc.configurable = true;
+
+        // you are not allowed to flip the configurable prop on an
+        // existing descriptor to anything but false (#2514)
+        if (!owned) {
+            methodDesc.configurable = true;
+        }
+

@eugenet8k
Copy link
Author

@fatso83 yes, the fix works well for our case, thank you!

@fatso83
Copy link
Contributor

fatso83 commented May 18, 2023

Out as Sinon 15.1.0 (minor due to also getting the new clock.jump() method)

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

No branches or pull requests

2 participants