-
-
Notifications
You must be signed in to change notification settings - Fork 771
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
Errors with .stub() and .spy() caused by getters/setters #1762
Comments
Looks like this could be related to #1741, but that doesn't specify that there is an error thrown. We also ideally would not need to use special Sinon methods for mocking Getters and could use the existing |
Here's an example of what Webpack compiles code to under the hood. An example ES6 file with an API like export function get(url, data, options) {
}
export function post(url, data, options) {
}
export function getJSON(url, data, options) {
} Produces this code in Webpack 3 "use strict";
Object.defineProperty(__webpack_exports__, "__esModule", { value: true });
/* harmony export (immutable) */ __webpack_exports__["get"] = get;
/* harmony export (immutable) */ __webpack_exports__["post"] = post;
/* harmony export (immutable) */ __webpack_exports__["getJSON"] = getJSON;
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0_jquery__ = __webpack_require__(13);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0_jquery___default = __webpack_require__.n(__WEBPACK_IMPORTED_MODULE_0_jquery__);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_1_lodash__ = __webpack_require__(1);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_1_lodash___default = __webpack_require__.n(__WEBPACK_IMPORTED_MODULE_1_lodash__);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_2__globals_scripts_csrf_es6__ = __webpack_require__(40);
function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; } And this code in Webpack 4: // define getter function for harmony exports
__webpack_require__.d = function(exports, name, getter) {
if(!__webpack_require__.o(exports, name)) {
Object.defineProperty(exports, name, {
configurable: false,
enumerable: true,
get: getter
});
}
};
// later
"use strict";
__webpack_require__.r(__webpack_exports__);
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "get", function() { return get; });
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "post", function() { return post; });
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "getJSON", function() { return getJSON; });
/* harmony import */ var jquery__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(/*! jquery */ "jquery");
/* harmony import */ var jquery__WEBPACK_IMPORTED_MODULE_0___default = /*#__PURE__*/__webpack_require__.n(jquery__WEBPACK_IMPORTED_MODULE_0__);
/* harmony import */ var lodash__WEBPACK_IMPORTED_MODULE_1__ = __webpack_require__(/*! lodash */ "./node_modules/lodash/lodash.js");
/* harmony import */ var lodash__WEBPACK_IMPORTED_MODULE_1___default = /*#__PURE__*/__webpack_require__.n(lodash__WEBPACK_IMPORTED_MODULE_1__);
/* harmony import */ var _globals_scripts_csrf_es6__WEBPACK_IMPORTED_MODULE_2__ = __webpack_require__(/*! ../../../globals/scripts/csrf.es6 */ "./globals/scripts/csrf.es6.js");
function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; } As you can see the module implementation now uses |
I found the section in the docs saying
However I switched to The only thing that fixes it for me is to replace import * as obj from './index'; with import { foo } from './index';
const obj = { foo }; since that prevents Webpack from creating getters in the compiled code. I'd rather not do that throughout our codebase though, |
So would we, but that problem has not yet been solved. Stubbing getters/setters was added in |
The next version of Sinon However, it still uses Ping @lucasfcosta: do you have any ideas for this issue? |
As @lavelle mentioned. esm import in Webpack 4 use immutable bindings with the following config: Object.defineProperty(exports, name, {
configurable: false,
enumerable: true,
get: getter
}); Having the same issue, I've written a tiny webpack plugin to override Object.defineProperty(logger, 'createLogger', {
writable: true,
value: sinon.stub().returns(loggerStub)
}); In the same issue (see last section) I've argued that Webpack should consider having But at any rate It may be wise to consider |
@Izhaki I am not sure I understand what you're proposing as a solution. Would you mind creating a runnable example that demonstrates your idea, in a way that isn't specific to any module loaders (Webpack, Rollup), but is just pure JS and can be used directly in ESM supporting runtimes, like in evergreen browsers? |
OK. Let's do this step by step. ESM exports use immutable binding. So when you write: export const x = value; The actual emitted code will end up calling: Object.defineProperty(exports, name, {
configurable: ?, // whether this is true or false depends on the bundler at the moment.
enumerable: true,
get: getter
}); where When you write: import { x } from './X'; What's imported is the getter for That's why However, so long the bundler sets Object.defineProperty(logger, 'createLogger', {
writable: true,
value: sinon.stub().returns(loggerStub)
}); Which is what sinon needs to do in order to support stubbing es6 imports. I'm not 100% sure what this issue is about exactly - the comments seem to have diverged from the original issue. But basically, if this ticket reports the inability to stub es6 imports, than this is the solution. |
Here's a more concrete example of how we enabled stubbing es6 imports: const isEs6Import = (object, property) => {
const descriptor = Object.getOwnPropertyDescriptor(object, property) || {};
// An es6 import will have get && enumerable.
// Non-es6 imports should have writable && value.
return descriptor.get && descriptor.enumerable;
};
// Takes a property that is not writable (such as an es6 import) and makes
// it writable.
// Should throw in strict mode if the property doesn't have configurable: true.
const makePropertyWritable = (object, property) => Object.defineProperty(
object,
property,
{
writable: true,
value: object[property]
}
);
/** Create a new sandbox before each test **/
helper.sinon = sinon.sandbox.create();
const sinonStub = helper.sinon.stub.bind(helper.sinon);
helper.sinon.stub = function (object, property) {
if (object && isEs6Import(object, property)) {
// Es6 imports are read-only by default.
// So make them writable so we can mock them.
makePropertyWritable(object, property);
}
return sinonStub(object, property);
}; |
You are right this issue is all over the place. All the getter/setter/sandbox stuff detracted from the main point. There are confusion at many stages, but I think you summed it up, @lzhaki.
I do sympathize with the idea of making Sinon more "auto-nice", but I am not sure the proposed fix is sufficient or error proof. What constitutes "the hallmarks of a ESM module"? How can we reliably detect we are dealing with transpiled ESM, not some general object that happens to have a getter? We already have explicit support for overriding accessors, so that won't fly. We could add additional methods of course, called sinon.stubImport or something, but it's a method of very limited use and timespan. Mind you, this will only work/make sense in a transpile-to-ES5 world, as ES modules are truly immutable. We explicitly detect that and say we can't support that, hence https://github.com/sinonjs/sinon/blob/master/test/es2015/module-support-assessment-test.mjs. Once people are over on native ESM and HTT2 loading making bundling obsolete all of these hacks goes out the window. I think simply adding spying capabilities by default to our existing accessor stubbing is a better solution that would also be more generally usable. See #1741 for discussion on that. I am on vacation without a computer, so not in a position to test out code, but I guess the steps needed for the original poster to achieve his intended goal is simply this:
As discussed in #1741 the passed in stub currently only provides behaviour, not spying. Until someone (@RoystonS?) expands on the API you'll need to pass in a spied function as the getter to verify interactions. Better docs would be nice, agreed ... As those points should answer the original issue I regard this as having to do with meagre documentation, not a bug. Feel free to provide improvements :-) Links: |
While what you say makes a lot of sense, allow me to counter-argue. What happened?We wanted to benefit from tree-shaking, so we switch Typescript to emit es6 rather than es5 modules - that's when all our This is just the startI'd be very surprised if esm is not going to prevail in the near future. There's too much good in it compared to other paradigms. You are probably going to have more and more people hitting this issue, or opening new issue on this matter. This is not about webpackThe implementation of esm import as getter does not seem to be a choice made by webpack - it appears that this is the way to comply with the standard. So in the following, replace 'webpack' and 'tooling format' with 'the standard':
This was never a proposed solution
The code I've provided was just a basic example - far from a 'solution' as far as sinon is concerned. Mind your API surface
Well there's another way to look at that... why is there an explicit support for accessors? Why do I care if its an object with an accessor, or just a function? Why should I use Anyhow, I hope I'm wrong about all this, and that is indeed a problem of far few people I appear to believe it is. Just we'll have to wait and see. |
Yes, it is a way of complying with the standard in environments that don't support/implement ES Modules, and where you want the effect of a module system in an environment that doesn't natively support it. Transpiled code will probably be the de facto standard for how how ES Modules will be consumed for quite a while, until support is ubiquitous. But it isn't the real thing. That's a good thing for testing employing mutation of targets, though, as true ES Modules can't be stubbed. Explicitly targeting Webpack's output format doesn't seem like a good way of spending maintenance resources, as it's a moving target that perhaps won't be needed in a couple of years, but the point of making the API easier to use is a good one. As you stated, why should you care. Right now, I am not totally sure if/what technical reasons we had for making the API we did, but ATM I really can't see why we can't get away with the needless dummy stub used in accesor stubbing today. I do mention some downsides of this simplification further down though, which might be why the original author of the Sinon 2+ functionality made the API decisions he did. Property descriptors is saving a superset of the property value, right? So it should be usable wherever we save values (such as the original function) today. By always storing the original property descriptor and using property descriptors when assigning new stubs we should be able to reuse the same logic (though I suspect the changes to the Sinon codebase might be quite invasive ...). This would remove explicit support for different setter and getter logic, but that again could be solved in the stub by seeing if it was used as a setter or getter. This would make the API nicer for some cases (although accessor stubbing is relatively rare today), but it would also make other cases potentially confusing: especially stubbing Webpack bundled modules. To see this, consider what information Sinon has about the export object made by Webpack: So that is maybe the answer to your question "Why should I care?": because you are the only one that knows what is expected. This could be remedied, of course, by instrumenting Webpack (using a plugin) to add additional hints to the export objects about what is hiding behind the getters, but again: this is implementation details we don't want in the core of Sinon, but it could be a nice complementary package (like sinon-test or sinon-as-promised in the past) that adds functionality, maintained by the interested community. Or ... just bypass the issue entirely, using link seams (proxywire, rewire, etc). After all, this issue concerns module loading, which Sinon isn't about, and for which there are existing, specialized products. |
True, we should consider adding a repo for that. As well as some docs ... Hopefully, @lzhaki won't mind if we steal the code for the plugin :-) |
@fatso83 This plugin? All yours. I'd be happy to help here - I've written the nodemon webpack plugin, so I'd probably be able to write the tests and deal with webpack 3 compatibility faster. Having said that, I'm still not sure |
This is about making a Sinon webpack plugin not changing the core webpack functionality. A Sinon webpack plugin is opt-in by nature as those using Sinon have to install and add the plugin to their webpack configs. |
@Izhaki help is always wanted. If you make a minimal repo called |
Hey guys, So i have attempted to use the recommended plugin and I still get the error
|
This doesn't work with latest version of Webpack. We're stuck on |
It never ends lol. Well i guess its just time to start using jest or stop using webpack in my tests. |
This issue is global - it is about adherence to the standard, and regardless of what you'll use you're going to hit the same issue (search the jest repo for the very same issue there). You cannot mock es modules. Nothing to do with Sinon or Webpack or Jest. |
That's somewhat debatable :-) Yes, indeed, if you are following the standards, this is absolutely not possible, since ES Modules per the standard does not allow for that, but you do have stuff like ESM breaking the rules :-) So, ignoring WebPack for a second, doing Now, for the issue here, I am just going to ask @joepuzzo the same thing I did in the linked WebPack thread: mocha --require @babel/register test/**/*.js |
I've replied in the original issue. |
Webpack users are often searching for ways to ways of using Sinon to stub out dependencies. They should really be using link seams instead, as you often cannot stub out internal dependencies. See discussions in - webpack/webpack#6979 (comment) - #1762 (plus #1962 and many more)
Webpack users are often searching for ways to ways of using Sinon to stub out dependencies. They should really be using link seams instead, as you often cannot stub out internal dependencies. See discussions in - webpack/webpack#6979 (comment) - sinonjs#1762 (plus sinonjs#1962 and many more)
Speaking for myself, I test front-end code in the browser. It gives me an environment in which I can actually inspect the UI that I'm testing. |
@steve-taylor I used to use Karma to achieve that earlier. Webpack in itself does no such thing; it just creates a bunde that can be used in your tests. Is there a specific loader/plugin you use to achieve running your tests in the browser? |
I'm migrating a Vue project away from Vue CLI, and must have updated Webpack in the process or something (or maybe it was because I removed Babel?). It caused Sinon to stop working with no errors. It just didn't work. Very confusing. It took me over a day to find this issue. I'm not keen to completely remove Webpack (yet) because then I'll have to make a whole new build system that does Typescript compilation, whatever dark magic |
So, to make sure I understand correctly:
to
Edit: Oh also note that the linked Plugin uses a string search and replace, which is not very robust, as proven by the fact that it will not work in the current Webpack because it tries to replace
I feel like the best solution would be for Sinon to detect these situations (rather than just silently fail), and then try to use the Is that about right? |
Ok I had the idea to just make all of the exports mutable properties, similar to Izhaki's solution but so that it wouldn't require any changes to the code. Still using hacky string-based search and replace.
Unfortunately
with
then it does work! I had a brief look into the Sinon source code and it seems like it can't mock getter/setter properties at all. This line doesn't do the right thing:
Maybe. I don't fully understand the Sinon code yet - it's quite complicated and almost entirely uncommented. :-/ |
Ok I gave up trying to figure out how to modify Sinon so that it automatically stubs properties that are setters/getters, so I just made a wrapper that does it outside Sinon:
Use that instead of
|
Hmm unfortunately while this does work, it is not restored properly by |
Aha, I found a solution! Just turn the getter into a normal value-based property.
Combine that with the It would be nice if Sinon provided a webpack plugin like |
Just to help any poor souls who find themselves here - I have a working solution for you. Be mindful of the First, create a new, ad-hoc plugin. Use this to rewrite the source generated by Webpack. Hunt down the bit of code that manages exports and rewrite the source so that it includes
Ensure the plugin is loaded
Import the module to be affected using * notation. Rewrite the default export and give it a traditional value which is self-referential. This will only work if the property is made configurable first.
Spy will now work as expected. :)
Consider upvoting my answer on S/O to help get the word out if you found this helpful. |
What did you expect to happen?
At @thumbtack we are in the process of upgrading our build to Webpack 4. As part of this some of our unit tests started failing. We tracked it down to the cases where we are using Sinon's
.spy
and.stub
functionality on modules that are exported using a non-default ES6 export of the formexport function foo
. Digging in, it looks like under the hood Webpack creates getters and setters for these exports for its new implementation of Harmony modules. This changed from version 3 to version 4.We were also able to reproduce this independent of Webpack by attempting to stub a plain object that has a getter or setter.
What actually happens
When using
.stub
, the stubbing works, but a later call to.restore
does not, and the assertion fails.When using
.spy
, the following error is thrown:TypeError: Attempted to wrap undefined property foo as function
. For some reason Sinon thinks a property is undefined when it also exists as a getter.How to reproduce
I created a repo that has a minimal reproduction of both the
stub
andspy
issues, with the latest versions of Webpack and Sinon. It also has a base case that shows that this issue does not occur in objects that are not imported this way and therefore don't use setters.You can clone the repo here: https://github.com/lavelle/sinon-stub-error
Run
yarn install
and then runyarn pass
to see the base caseyarn fail
to see the failing stub caseyarn spy
to see the failing spy caseThanks in advance! We're happy to submit a PR to address this if you can point us in the right direction.
cc @bawjensen @dcapo
The text was updated successfully, but these errors were encountered: