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

Handling non-configurable object descriptors on the prototype #2508

Merged
merged 6 commits into from
Apr 20, 2023

Conversation

fatso83
Copy link
Contributor

@fatso83 fatso83 commented Apr 20, 2023

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

image

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
image

@fatso83 fatso83 marked this pull request as draft April 20, 2023 08:59
@fatso83 fatso83 changed the title Handling non-configurable object descriptors on the prototype WIP Handling non-configurable object descriptors on the prototype Apr 20, 2023
@@ -137,6 +137,7 @@ module.exports = function wrapMethod(object, property, method) {
for (i = 0; i < types.length; i++) {
mirrorProperties(methodDesc[types[i]], wrappedMethodDesc[types[i]]);
}
methodDesc.configurable = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

mroderick commented Apr 20, 2023

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
Copy link
Contributor Author

fatso83 commented Apr 20, 2023

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
Copy link
Contributor Author

fatso83 commented Apr 20, 2023

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)

@fatso83 fatso83 marked this pull request as ready for review April 20, 2023 11:39
@fatso83 fatso83 changed the title WIP Handling non-configurable object descriptors on the prototype Handling non-configurable object descriptors on the prototype Apr 20, 2023
This shows an incoherent appraoch to how we deal with object
descriptors across different code paths.
never supported undefined or protypal props in the first place.
See sinonjs#2195 for backing discussion on creating sinon.define()
@codecov
Copy link

codecov bot commented Apr 20, 2023

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.

@fatso83 fatso83 merged commit e9042c4 into sinonjs:main Apr 20, 2023
@fatso83 fatso83 deleted the issue-2491 branch April 20, 2023 11:54
@eugenet8k
Copy link

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
Copy link
Contributor Author

fatso83 commented May 5, 2023

@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
Copy link

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

@fatso83
Copy link
Contributor Author

fatso83 commented May 16, 2023

@eugenet8k Fix in #2515. Please verify.

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

Successfully merging this pull request may close these issues.

3 participants