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

Fix sinon match with referee #249

Merged
merged 3 commits into from
Jun 29, 2023
Merged

Fix sinon match with referee #249

merged 3 commits into from
Jun 29, 2023

Conversation

mantoni
Copy link
Member

@mantoni mantoni commented Apr 6, 2023

Purpose (TL;DR) - mandatory

In sinonjs/sinon#2498 @sinonjs/samsam was upgraded to v8 and this was released with sinon v15.0.2. This beaks a use case when sinon.match is used with referee assertions.

This PR adds a failing test, demonstrating the issue and upgrades samsam to v8 to fix it.

Background (Problem in detail) - optional

The isMatcher helper in samsam checks whether the prototype of the given object is the matcher prototype (https://github.com/sinonjs/samsam/blob/main/lib/create-matcher/is-matcher.js). When there are two different versions of samsam in the dependency tree, there are two instances of that prototype. Therefore matcher detection fails if the matcher was created with the one version and the check is done with the other version of samsam.

Solution - optional

There should always be exactly one version of samsam in the dependency tree.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm test

Checklist for author

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

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d0dfef5) 100.00% compared to head (cb47483) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #249   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           81        81           
  Lines          666       666           
=========================================
  Hits           666       666           
Flag Coverage Δ
unit 100.00% <ø> (ø)

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

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.

@mantoni mantoni requested review from mroderick and fatso83 April 6, 2023 11:04
@fatso83
Copy link
Contributor

fatso83 commented Apr 11, 2023

The test seems a bit fragile when it depends on someone needing to keep two libraries in sync. Could we possibly change this for something that can handle different versions interacting? Say, check for a Symbol or something.

@mantoni
Copy link
Member Author

mantoni commented Apr 12, 2023

That wouldn't help. If there are two versions of the library in the tree, the symbol would exist twice as well. We would need a global ...

@fatso83
Copy link
Contributor

fatso83 commented Apr 12, 2023

Could employ Symbol.of("identifier") for that? Just a glorified global constant, but is not enumerated or anything like that, so a bit better than the old ES5 approaches.

@mantoni
Copy link
Member Author

mantoni commented Apr 15, 2023

That's a good idea. I like it!

@mantoni
Copy link
Member Author

mantoni commented Jun 29, 2023

@fatso83 Can I merge and release a patch of this? The current v11 of @sinonjs/referee-sinon is broken because of this. Changing the matcher detection to use Symbols has to be done in @sinonjs/samsam and the tests introduced here are only becoming less fragile once that lands, but they're still useful.

@fatso83
Copy link
Contributor

fatso83 commented Jun 29, 2023

sure, I am not a blocker :)

@mantoni mantoni merged commit 4d3e91c into main Jun 29, 2023
@mantoni mantoni deleted the fix-sinon-match branch June 29, 2023 09:06
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.

2 participants