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

Jump to bottom

Fix turbo usage in tests #44715

Open
wants to merge 48 commits into
base: canary
Choose a base branch
from
Open

Fix turbo usage in tests #44715

wants to merge 48 commits into from
+301 −259

Conversation

JanKaifer
Copy link
Member

@JanKaifer JanKaifer commented Jan 9, 2023 •
edited

  • install ts-node
  • start packing things with turbo properly
  • refactored repo-setup.js to use new turbo packing
  • migrated to .mts for scripts

I made the script pnpm pack compatible for easier switching in the future.
Right now pnpm pack can't be used because it doesn't include swc binaries in the resulting tarball. Will solve it in a separate PR.

The stats action is failing because canary doesn't have the test-pack action in turbo.json.

So we can either merge this in 2 phases or just ignore that one failing Action.

Testing

I tried testing this by editing the next core, but could someone double check please?

Questions

I made test-pack turbo task depend only on source files (that are not gitignored). Which is not great, it would be better if I could mark it as dependent on build but that could be a bit slower since people use dev.

Also If the dev produces something janky, test-pack might cache it.
I'm not sure how common that is.

@ijjk ijjk added area: create-next-app CLI (create-next-app) created-by: Next.js team PRs by the Next.js team type: next labels Jan 9, 2023
@JanKaifer JanKaifer marked this pull request as draft Jan 9, 2023
@JanKaifer
Copy link
Member Author

The stats action is failing because canary doesn't have the test-pack action in turbo.json.

So we can either merge this in 2 phases or just ignore that one failing Action.

JanKaifer added 16 commits Jan 10, 2023
@JanKaifer
Copy link
Member Author

It's weird I ran those CI actions locally (that docker command on top) and it passes 🤷.

@JanKaifer
Copy link
Member Author

Hmm, it passes now.
Build, test, and deploy / Test Development (18, 3) (pull_request) seems to be actual regression. I'll need to check the updated packages and what caused the issue (i installed ts-node).

@JanKaifer JanKaifer marked this pull request as ready for review Jan 12, 2023
timneutkens
timneutkens previously approved these changes Jan 17, 2023
View changes
styfle
styfle reviewed Jan 18, 2023
View changes
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Looks like the pnpm pack bug was fixed.

pnpm/pnpm#2941 (comment)

Should we try updating pnpm to the latest version?

styfle
styfle reviewed Jan 18, 2023
View changes
packages/eslint-config-next/package.json
@@ -8,6 +8,9 @@
"url": "vercel/next.js",
"directory": "packages/eslint-config-next"
},
"scripts": {
"test-pack": "cd ../../ && pnpm test-pack eslint-config-next"
Copy link
Member

Choose a reason for hiding this comment

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

This seems redundant that the package has to pass the name. Can we update the script to derive the current package by reading process.cwd() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I dig into it. And I would need to repeat that ts-node magical initialization.
Not worth it. But good point.

@JanKaifer
Copy link
Member Author

Should we try updating pnpm to the latest version?

I want to merge this one. And we probably want to merge pnpm upgrade as its own PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@styfle styfle

@timneutkens timneutkens

@ijjk ijjk

@shuding shuding

@vercel/web-tooling Web Tooling

@huozhi huozhi

@padmaia padmaia

@balazsorban44 balazsorban44

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area: create-next-app CLI (create-next-app) created-by: Next.js team PRs by the Next.js team type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JanKaifer @styfle @timneutkens @ijjk