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

chore: enable test coverage #6799

Closed
wants to merge 3 commits into from

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Aug 11, 2022

Ref jestjs/jest#12998 (comment), the issue with the test coverage is caused by @jridgewell/trace-mapping. In order to unblock, let's just force the version of that module in that repo.

@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2022

⚠️ No Changeset found

Latest commit: b6b0b04

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Aug 11, 2022

Deploy Preview for apollo-server-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b6b0b04
🔍 Latest deploy log https://app.netlify.com/sites/apollo-server-docs/deploys/62f560c34c277000088af276
😎 Deploy Preview https://deploy-preview-6799--apollo-server-docs.netlify.app/migration
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 11, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b6b0b04:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration

@glasser
Copy link
Member

glasser commented Aug 11, 2022

Thanks for tracking this down @SimenB!

I am not super comfortable relying on package-lock.json for important transitive dependency bumps so perhaps I'll just follow along with jestjs/jest#13119 and see when it ends up in a release? (The PR as written also doesn't currently actually enable coverage.)

@SimenB
Copy link
Contributor Author

SimenB commented Aug 12, 2022

see when it ends up in a release

In v29 stable sometime this weekend or next week. So it'll require a major bump (not super breaking though, so possibly no issue to upgrade).

The PR as written also doesn't currently actually enable coverage.

Oh, I just reverted the linked commit from jestjs/jest#12998 (comment) and updated the dep 🤷

@jridgewell
Copy link

I don't believe jestjs/jest#12998 will actually resolve the issue, because it's Babel that's generating the incorrect trace-mapping instance. The reason you noticed this when upgrading Jest is because Jest will reuse the invalid sourcemap as is. Maybe I should go with a better branding check, so that the old code path fails to run for this old remapping code.

@jridgewell
Copy link

I am not super comfortable relying on package-lock.json for important transitive dependency bumps so perhaps I'll just follow along with facebook/jest#13119 and see when it ends up in a release?

If you were to fresh npm i without the lockfile, the problem is fixed because @babel/core depends on @ampproject/remapping@^2.1.0, and that depends on @jridgewell/trace-mapping@^0.3.0, and it'll pick up the fix in v0.3.9. If you were to do npm ci, then it would fail because of the package-lock.json. By updating the lockfile, everything should be resolved now.

@SimenB
Copy link
Contributor Author

SimenB commented Sep 8, 2022

Seems like this is of no interest, so I'll close to keep clutter down

@SimenB SimenB closed this Sep 8, 2022
@SimenB SimenB deleted the enable-coverage branch September 8, 2022 08:28
@glasser
Copy link
Member

glasser commented Sep 8, 2022

Thanks; I am still tracking this with the assumption that the Jest 29 upgrade will also resolve this (which is blocked on ts-jest doing a final release of their 29 support).

@SimenB
Copy link
Contributor Author

SimenB commented Sep 9, 2022

Not necessarily - as mentioned the issue isn't in any of Jest's modules which'd be updated by v29 upgrade - it's transitive via @babel/core. This PR is just npm update @ampproject/remapping which fixes the issue. You can also do npm update @jridgewell/trace-mapping which forces a version which handles the output from @ampproject/remapping (and is what you'll get from updating Jest).


That said, ts-jest was released 3 hours ago, so might be you just wanna go to v29 🙂 But again, that's like 5 dependencies removed from the actual bug fix.

trevor-scheer added a commit that referenced this pull request Sep 14, 2022
Supersedes #6799

Co-authored-by: SimenB <sbekkhus91@gmail.com>
trevor-scheer added a commit that referenced this pull request Sep 14, 2022
Supersedes #6799

Co-authored-by: SimenB <sbekkhus91@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants