Handling non-configurable object descriptors on the prototype

[fatso83]

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: true set 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.js only 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 stubs module it fails for stubs before getting to the wrapMethod call, as it has checks of their own on the object descriptors: Descriptor for property aMethod is non-configurable and non-writable

Mocks
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 with Cannot assign to read only property 'aMethod' of object '#<BaseClass>'. The "weird" thing here is that we do not use any of the old wrap-method logic and instead have resorted to build new functionality for restoring. I am not sure why.
EDIT: by intent. See discussion in #2195

Solution - optional

Setting configurable: true in 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-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.

• Could the also perhaps be moved to wrap-method so that it would also be used for spies?

• Sandbox#replace probably should not use wrap-method, as it seems to have simplified the use-cases. Not sure though

How to verify - mandatory

1. Check out this branch
2. npm i; npm test

Checklist for author

• npm run lint passes
• References to standard library functions are cached.

isOwn and shadowsPropOnPrototype is relevant

[mroderick]

Do we need to store the original value, so it can be restored when the method is restored?

[fatso83]

The restore() function in this module already restores the original value. It only does so in the case of own descriptors, otherwise it just deletes the spy/stub (thus removing the shadowing of the prototype).

[mroderick]

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]

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 7fb6657#diff-3cbef9e218193dbaed32c532449e176e0cf97d7c0427326cdd5c83ba18df20fbR2-R25 on Mar 30, 2010

function mock was introduced in 84dbb22#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]

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]

EDIT: haha, turns out I brought the below up 3 years ago in #2195 🤣 So maybe create sinon.define then 😄

---

A side note

A little detour into setting fakes

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.method

Should 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 else

[fatso83]

I 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]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (3b41aff) 95.99% compared to head (d628b19) 95.99%.

❗ Current head d628b19 differs from pull request most recent head 040b144. Consider uploading reports for the commit 040b144 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2508   +/-   ##
=======================================
  Coverage   95.99%   95.99%           
=======================================
  Files          40       40           
  Lines        1898     1899    +1     
=======================================
+ Hits         1822     1823    +1     
  Misses         76       76           

Flag	Coverage Δ
unit	95.99% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/sinon/stub.js	95.19% <100.00%> (ø)
lib/sinon/util/core/wrap-method.js	82.35% <100.00%> (+0.17%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[eugenet8k]

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')
};

[fatso83]

@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:

1. Copy the proprietary source code to some other folder
2. Make/Find a failing test where the above issues happens
3. Delete all prod and test code that doesn't affect the test, backing up every time it crashes for other reasons
4. Modify/remove dependencies when something crashes, trimming away all dead code (like unused reducers in reduc stores)
5. Keep on pruning until you have an project consisting of only config files (babel, package.json, eslint, etc) and the minimal amount of code needed for reproddution (typically you should be able to just be left with 2-5 files)
6. Run some renaming script to rename variables/code
7. Make sure it still runs and push to github :)

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 | pbcopy

to trim away all production references.

[eugenet8k]

@fatso83 thank you for the extended comment, I created two code sandbox examples to demonstrate the issue, please review #2514

[fatso83]

@eugenet8k Fix in #2515. Please verify.