Closed fatso83 closed 1 year ago
mroderick
commented
1 year ago The Descriptor for property aMethod is non-configurable and non-writable check is good and could be re-used elsewhere, but it needs to only apply to cases where the descriptor comes from the object, not its prototype. We can use the isOwn property from get-property-descriptor.js to do this.
That's could help reduce the number of implementations 👍
mroderick
commented
1 year ago I am not sure why the mocks logic is not using any of the above (wrap-method) for spies and stubs. Probably should
It seems that wrapMethod predates mock, which was surprising to me.
function wrapMethod was introduced in https://github.com/sinonjs/sinon/commit/7fb6657f6290673ada8938aa454b312020fee427#diff-3cbef9e218193dbaed32c532449e176e0cf97d7c0427326cdd5c83ba18df20fbR2-R25 on Mar 30, 2010
function mock was introduced in https://github.com/sinonjs/sinon/commit/84dbb22ae72a94380b846ee58a7d2b0238c5ea0d#diff-ab6968d5e523c9eac55c8cd92a72dcd87f6647f3587e8d1b8c691e3e7746cc2bR12-R18 on Apr 18, 2010
mock has seen very little love over the years, probably because none of the maintainers use it. It seems likely to me that very few of the users actually use mock, or there would have been more issues and pull requests for it over the years. With that in mind, I don't find it surprising that it has it's own way of doing things.
I agree, it should use wrapMethod for spies and stubs.
mroderick
commented
1 year ago Sandbox#replace probably should not use wrap-method, as it seems to have simplified the use-cases. Not sure though
Some background: Sandbox#replace was introduced along with sinon.fake. I wanted to not do the same as sinon.stub, which creates both the stub and puts it into place on the host object. So, the logic of creating a fake and the logic of putting it into place were separated. Doing that, made things simpler. Unlike sinon.stub, Sandbox#replace only deals with existing properties, which made this much easier.
Like you, I think that it would probably be best to leave Sandbox#replace as is, at least for the time being. It's simple, it still works. Improving other parts of the codebase won't affect it negatively.
fatso83
commented
1 year ago EDIT: haha, turns out I brought the below up 3 years ago in #2195 :rofl: So maybe create sinon.define then :smile:
A side note
Sandbox#replace only deals with existing properties, which made this much easier.
That means there is no easy way of cleaning up fakes set on instances of a class, right? Say we have this:
class Foo{ method(){} }
const foo = new Foo();We could easily clean up this with a sandbox.restore():
sandbox.stub(foo, 'method')
sandbox.restore()But setting and cleaning up using fakes looks like this:
foo.method = sandbox.fake()
delete foo.methodShould we not try to aim for consistency here? I think we have touched on this before (without anyone doing anything), but I could have liked for us to have this:
sandbox.replace(foo, 'method', sandbox.fake(), {allowNonExisting: true})
sandbox.restore(); // like for everywhere elseI agree, it should use wrapMethod for spies and stubs.
mock.js is actually using wrapMethod. That is exactly what is failing (for some reason I haven't looked into yet)
codecov[bot]
commented
1 year ago Patch coverage: 100.00% and no project coverage change.
Comparison is base (
3b41aff) 95.99% compared to head (d628b19) 95.99%.:exclamation: Current head d628b19 differs from pull request most recent head 040b144. Consider uploading reports for the commit 040b144 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
eugenet8k
commented
1 year ago This change brought a decent amount of failures in our unit test coverage. We used sinon for stubbing various objects and ES imports.
For example, a unit test where we stub some method in the Chaplin JS framework:
import Chaplin from 'chaplin'
describe(__filename, () => {
beforeEach(() => {
sandbox = sinon.createSandbox()
...
sandbox.stub(Chaplin.utils, 'redirectTo')
...
})
fails with
[‣](http://localhost/test/)
TypeError: Cannot redefine property: redirectTo
at Function.defineProperty (<anonymous>)
at wrapMethod (webpack-internal:///./node_modules/sinon/pkg/sinon-esm.js:4651:16)
at Function.stub (webpack-internal:///./node_modules/sinon/pkg/sinon-esm.js:3847:44)
at Sandbox.stub (webpack-internal:///./node_modules/sinon/pkg/sinon-esm.js:3291:39)
at Context.eval (webpack-internal:///./app/components/apps/app-row-layout.spec.js:54:13)if I rollback this newly added one-liner
140: methodDesc.configurable = true;it works just fine.
The ChaplinJS source code looks something like
/*!
* Chaplin 1.2.0
*
* Chaplin may be freely distributed under the MIT license.
* For all details and documentation:
* http://chaplinjs.org
*/
(function(root, factory) {
if (typeof define === 'function' && define.amd) {
define(['backbone', 'underscore'], factory);
} else if (typeof module === 'object' && module && module.exports) {
module.exports = factory(require('backbone'), require('underscore'));
} else if (typeof require === 'function') {
factory(window.Backbone, window._ || window.Backbone.utils);
} else {
throw new Error('Chaplin requires Common.js or AMD modules');
}
}(this, function(Backbone, _) {
function require(name) {
return {backbone: Backbone, underscore: _}[name];
}
require =(function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){
'use strict';
module.exports = {
Application: require('./chaplin/application'),
Composer: require('./chaplin/composer'),
Controller: require('./chaplin/controllers/controller'),
Dispatcher: require('./chaplin/dispatcher'),
Composition: require('./chaplin/lib/composition'),
EventBroker: require('./chaplin/lib/event_broker'),
History: require('./chaplin/lib/history'),
Route: require('./chaplin/lib/route'),
Router: require('./chaplin/lib/router'),
support: require('./chaplin/lib/support'),
SyncMachine: require('./chaplin/lib/sync_machine'),
utils: require('./chaplin/lib/utils'),
mediator: require('./chaplin/mediator'),
Collection: require('./chaplin/models/collection'),
Model: require('./chaplin/models/model'),
CollectionView: require('./chaplin/views/collection_view'),
Layout: require('./chaplin/views/layout'),
View: require('./chaplin/views/view')
};@eugenet8k I am sorry to hear that, but I have no idea how this could happen from the brief description. Would it be possible for you to file a bug report with a reproducible example? Without code to reproduce an issue this will not be looked at.
Guessing this happens in some proprietary product, so for comparison, this is what I usually do myself if I do not know how to recreate a minimal example from scratch:
Here's an example of what I run all production code through before it ends up on StackOverflow:
alias websafe='util.esed -e "s/(diffia|ptflow)/acme-org/gi" -e "s/$USER/my-user/gi" -e "s/(nimble|ptflow|pete)/foo-project/gi" -e "s/(clinic|people)/app-\1/gi" 'diffia, pete, clinic, people and ptflow are project references I want to remove
So I can do
pbpaste | websafe | pbcopyto trim away all production references.
eugenet8k
commented
1 year ago @fatso83 thank you for the extended comment, I created two code sandbox examples to demonstrate the issue, please review https://github.com/sinonjs/sinon/issues/2514
fatso83
commented
1 year ago @eugenet8k Fix in #2515. Please verify.
Purpose (TL;DR) - mandatory
Fix for #2491 where it seems that we fail to restore spies set on instances if their prototypes have unconfigurable property descriptors.
Background (Problem in detail) - optional
A property descriptor must have
configurable: trueset for us to be able to restore it. If we get the property descriptor from a prototype we should be free to modify this to whatever we want.At first I hit issues when trying to achieve consistency for spies, stubs, mocks and fakes.
Fixing this in
wrap-method.jsonly fixes the issue for spies as we deal with the issues in four different ways:stubs Even though this module is also imported by the
stubsmodule it fails for stubs before getting to thewrapMethodcall, as it has checks of their own on the object descriptors:Descriptor for property aMethod is non-configurable and non-writableMocks Fixed by the above fix
fakes / sandbox.replace() Fakes do not implement anything to do with replacing properties themselves and leave that to
Sandbox#replace, which fails withCannot assign to read only property 'aMethod' of object '#<BaseClass>'. ~The "weird" thing here is that we do not use any of the oldwrap-methodlogic and instead have resorted to build new functionality for restoring. I am not sure why.~ EDIT: by intent. See discussion in #2195Solution - optional
Setting
configurable: truein wrap-method was only helpful for spies and mock.These are my thoughts for further work in this PR
The
Descriptor for property aMethod is non-configurable and non-writablecheck is good and could be re-used elsewhere, but it needs to only apply to cases where the descriptor comes from the object, not its prototype. We can use theisOwnproperty fromget-property-descriptor.jsto do this.Could the also perhaps be moved to
wrap-methodso that it would also be used for spies?Sandbox#replaceprobably should not usewrap-method, as it seems to have simplified the use-cases. Not sure thoughHow to verify - mandatory
npm i; npm testChecklist for author
npm run lintpassesisOwnandshadowsPropOnPrototypeis relevant