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

[sinon][ts] convert sinon.SinonStub -> jest.Mock, sinon.SinonSpy -> jest.SpyInstance #359

Merged
merged 3 commits into from
Aug 23, 2022
Merged

[sinon][ts] convert sinon.SinonStub -> jest.Mock, sinon.SinonSpy -> jest.SpyInstance #359

merged 3 commits into from
Aug 23, 2022

Conversation

danbeam
Copy link
Contributor

@danbeam danbeam commented Aug 19, 2022

Reviewers (other than @skovhus): @catc @lencioni @jayrobin

@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Merging #359 (3d699fc) into main (b4ca35b) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 3d699fc differs from pull request most recent head 6a2712b. Consider uploading reports for the commit 6a2712b to get more accurate results

@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
- Coverage   92.30%   92.28%   -0.03%     
==========================================
  Files          26       26              
  Lines        1924     1905      -19     
  Branches      404      398       -6     
==========================================
- Hits         1776     1758      -18     
+ Misses        102      101       -1     
  Partials       46       46              
Impacted Files Coverage Δ
src/transformers/sinon.ts 89.43% <100.00%> (-0.50%) ⬇️
src/utils/test-helpers.ts 90.00% <100.00%> (+4.28%) ⬆️

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

export default function transformer(fileInfo: FileInfo, api: API, options) {
const j = api.jscodeshift
const j = api.jscodeshift.withParser(options.parser)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been looking for this forever.

Copy link
Owner

Choose a reason for hiding this comment

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

Lazy web: what is this actually doing? The right parser is already selected by the CLI, so I’m curious what this is doing. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lazy web: what is this actually doing?

this took a while for me to figure out as well. this PR adds the first test case in which the code fed to expectTransformation() is not parseable by the babel parser. and it turns out that passing { parser: 'ts' } to tests aren't actually causing the ts parser to be used instead of babel. all tests were still using babel.

The right parser is already selected by the CLI, so I’m curious what this is doing. :)

without this, typescript in the input to expectTransformation() yields the cryptic:

  ● updates types › rewrites types for mocks & stubs

    SyntaxError: Missing semicolon. (4:15)

      at Object._raise (node_modules/@babel/parser/src/parser/error.js:147:45)

with this line the test succeeds because the correct parser is selected. I just tried moving this to src/utils/test-helpers.ts instead of in the "prod" code but it isn't working.

I can keep trying to find a way to make this work in test-only code or we can land for now and I'll continue as a followup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, moved this withTransform() call to test-only code

Copy link
Owner

Choose a reason for hiding this comment

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

👏

@danbeam
Copy link
Contributor Author

danbeam commented Aug 19, 2022

thanks @jayrobin and @catc! on to @skovhus

@skovhus skovhus enabled auto-merge (squash) August 23, 2022 07:00
@skovhus skovhus merged commit c9c541c into skovhus:main Aug 23, 2022
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.

4 participants