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(jasmine-globals): fix transform for existing spyOn #580

Merged
merged 2 commits into from
May 24, 2024

Conversation

puglyfe
Copy link
Contributor

@puglyfe puglyfe commented May 22, 2024

Closes #579

Better handling of usage of existingSpy.and.callThrough(). This follows the same pattern as the legacy andCallThrough logic here.

Copy link

codecov bot commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.63%. Comparing base (a94e2b5) to head (a3e1b24).
Report is 51 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #580      +/-   ##
==========================================
+ Coverage   92.38%   92.63%   +0.24%     
==========================================
  Files          26       27       +1     
  Lines        1944     2010      +66     
  Branches      405      416      +11     
==========================================
+ Hits         1796     1862      +66     
  Misses        102      102              
  Partials       46       46              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -42,6 +43,7 @@ test('spyOn', () => {
jest.spyOn(stuff).mockResolvedValue('lmao');
jest.spyOn(stuff).mockRejectedValue('oh no');
const fetchSpy = jest.spyOn(window, 'fetch').mockResolvedValue({json: {}});
existingSpy;
Copy link
Owner

@skovhus skovhus May 23, 2024

Choose a reason for hiding this comment

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

This looks wrong to me. Does it make sense to keep it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't necessarily disagree (more for stylistic reasons than anything technical), but it does match the output of the andCallThrough() transform here.

But now that I'm thinking about it, I wonder if the behavior should be different for existingSpy, as simply dropping the .and.callThrough() wouldn't restore the original implementation. Maybe the more accurate transform would be:

spyOn(stuff).and.callThrough(); -> jest.spyOn(stuff);

existingSpy.and.callThrough(); -> existingSpy.mockRestore();

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

decided to make that change. let me know what you think.

Copy link
Owner

Choose a reason for hiding this comment

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

Seems to make sense, thanks.

@skovhus skovhus merged commit bb93df3 into skovhus:main May 24, 2024
3 checks passed
@puglyfe puglyfe deleted the fix-existing-spy-callthrough branch May 24, 2024 14:19
@puglyfe
Copy link
Contributor Author

puglyfe commented May 24, 2024

thanks @skovhus. would it be possible to publish a new release that contains the recent updates? it doesn't look like there's anything else currently under active development.

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.

jasmine-globals: unable to transform re-used spyOn that uses .and.callThrough()
2 participants