create-turbo: improve test cases and make them pass #1655

mehulkar · 2022-08-08T22:05:17Z

Make existing tests pass
Add GH action to run tests in CI

Set up a new Github Action that builds the turbo CLI (since that is
required for the create-turbo tests) and runs the JS tests for that
package.

Notable things:
- Installs yarn and disables corepack as part of the test suite. This
  is the pristine state we want for the tests that exist today. For
  future tests cases that have a different setup, we may need to
  modify this more.
- Disables Windows tests. This yarn setup on Windows does not seem to
  work, but this gets us one step forward.

vercel · 2022-08-08T22:05:21Z

@mehulkar is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

.github/workflows/ci-createturbo.yml

packages/create-turbo/__tests__/cli.test.ts

nathanhammond

This will require changes to https://github.com/vercel/turborepo/blob/65f23138861483b3e13889824a91b0f9323ad63b/packages/create-turbo/src/shouldUseYarn.ts (and https://github.com/vercel/turborepo/blob/65f23138861483b3e13889824a91b0f9323ad63b/packages/create-turbo/src/shouldUsePnpm.ts)

packages/create-turbo/__tests__/cli.test.ts

mehulkar · 2022-08-09T23:50:28Z

.github/workflows/ci-js.yml

+        os: [
+          ubuntu-latest,
+          macos-latest,
+          # windows-latest,


I'd like to come back to this

A comment detailing what issues were encountered would be helpful

It didn't seem to pass in CI and I wanted to isolate the issue and fix. I'm not even sure at this point it's a real problem

vercel · 2022-08-10T16:08:16Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
turbo-site	✅ Ready (Inspect)	Visit Preview	Aug 15, 2022 at 3:25PM (UTC)

outdated! needs another look now

nathanhammond · 2022-08-10T20:36:40Z

.github/workflows/ci-js.yml

@@ -0,0 +1,78 @@
+name: CI JS


Is this 100% a copy of ci-go with the npm section and the create-turbo tests added?

pretty much, but excludes the examples action (terminology?) and excludes the Go tests

nathanhammond · 2022-08-10T20:37:22Z

.github/workflows/ci-js.yml

+        os: [
+          ubuntu-latest,
+          macos-latest,
+          # windows-latest,


I tried to track down previous runs, got a link? (Don't mind tabling, but I do want to see it.)

https://github.com/vercel/turborepo/runs/7777949925?check_suite_focus=true

I tthink the issue is that yarn -v is erroring or the npm i -g yarn isn't working. Something for a followup PR

nathanhammond · 2022-08-10T20:40:19Z

packages/create-turbo/__tests__/cli.test.ts

@@ -23,6 +23,8 @@ const DEFAULT_JEST_TIMEOUT = 10000;

 describe("create-turbo cli", () => {
  beforeAll(() => {
+    execSync("corepack disable", { stdio: "ignore" });


This would hopefully be deleted at resolution of this issue.

I think so. We could also just not install corepack in ci-js.yml, but I did it anyway so CI matches what we'll have on dev machines.

Although if/when that issue is resolved, we might still want this so we can represent a real npx usecase where a packageManager (or its shim) isn't available and we really do want to show the "not available" case. I don't have a foolproof way forward to address both cases

(That link was mostly to get a pointer to this discussion upstream.)

nathanhammond · 2022-08-10T20:40:33Z

.github/workflows/ci-js.yml

+      - name: Setup Yarn
+        run: npm i -g yarn


This would hopefully be deleted at resolution of this issue.

packages/create-turbo/__tests__/cli.test.ts

tknickman

One comment about an extra test case, but otherwise looks good

tknickman · 2022-08-11T19:08:10Z

packages/create-turbo/__tests__/cli.test.ts

+        );
+
+        expect(messages[2]).toMatch(
+          /^>>> Creating a new turborepo with the following:/


Can we add a check for:

>>> Success! Created a new Turborepo at "${DEFAULT_APP_NAME}".

as well? This would catch that pnpm error we had a few weeks ago (the install actually worked, pnpm was just non-zero exiting because it was upset about peers. (which caused us to not show this message and show install has failed

I can add that, but I wasn't able to reproduce that failure, for what it's worth :/

after this PR when tests are running in CI, we can have a bit more confidence about adding new tests if we have regressions again.

gsoltis

I'll defer to @nathanhammond for final LGTM since I think I'm a little out of the loop on the context here.

In general, this looks fine to me. Just getting these running seems like a step in the right direction. Some things we might want to consider adding, now or in the future

displaying the versions of the package managers used, if possible. if an upgrade breaks something, that will help us know
are we going to test old and new yarn ("berry")?

mehulkar · 2022-08-12T17:57:31Z

Some things we might want to consider adding, now or in the future

Good ideas. I think it makes sense to get these running first and add more as needed

are we going to test old and new yarn ("berry")?

I'd have to think about how to make this possible, since the CI setup (rather than the test case itself) is installing and activating the yarn/npm/etc.

gsoltis · 2022-08-12T23:45:32Z

Test failure? Or is it flaky?

mehulkar · 2022-08-15T16:32:29Z

Test failure? Or is it flaky?

I think they are flaky. Looks like it's the Go e2e ones, which shouldn't be affected by this PR.

cli:test: > make test-go
[26](https://github.com/vercel/turborepo/runs/7840795316?check_suite_focus=true#step:10:27)
cli:test: 
[27](https://github.com/vercel/turborepo/runs/7840795316?check_suite_focus=true#step:10:28)
cli:test: go test -race ./...

...[snipped]...

cli:test: Started
[56](https://github.com/vercel/turborepo/runs/7840795316?check_suite_focus=true#step:10:57)
cli:test: --- FAIL: TestStart (0.20s)
[57](https://github.com/vercel/turborepo/runs/7840795316?check_suite_focus=true#step:10:58)
cli:test:     child_test.go:159: process should have exited

gsoltis · 2022-08-15T16:45:07Z

@mehulkar I think one of the js tests is also failing

- Make existing tests pass - Add GH action to run tests in CI Set up a new Github Action that builds the turbo CLI (since that is required for the create-turbo tests) and runs the JS tests for that package. Notable things: - Installs yarn and disables corepack as part of the test suite. This is the pristine state we want for the tests that exist today. For future tests cases that have a different setup, we may need to modify this more. - Disables Windows tests. This yarn setup on Windows does not seem to work, but this gets us one step forward.

mehulkar · 2022-08-15T17:19:48Z

@mehulkar I think one of the js tests is also failing

npm install, et al are timing out, even at 50s timeout. Removed them from this PR, and will submit separately to isolate and add logging etc.

mehulkar requested review from jaredpalmer, gsoltis, gaspar09, nathanhammond, tknickman and chris-olszewski as code owners August 8, 2022 22:05

mehulkar commented Aug 8, 2022

View reviewed changes

.github/workflows/ci-createturbo.yml Outdated Show resolved Hide resolved

mehulkar force-pushed the mk/add-to-ci branch 2 times, most recently from d7addd8 to fc0b6b8 Compare August 8, 2022 22:23

mehulkar mentioned this pull request Aug 8, 2022

Mk/fix tests #1657

Closed

mehulkar commented Aug 8, 2022

View reviewed changes

packages/create-turbo/__tests__/cli.test.ts Outdated Show resolved Hide resolved

mehulkar added the pr: on hold Pull requests that are "on hold" and should not be merged label Aug 8, 2022

mehulkar force-pushed the mk/add-to-ci branch from 8e551bb to 82e535e Compare August 8, 2022 23:19

nathanhammond previously requested changes Aug 9, 2022

View reviewed changes

packages/create-turbo/__tests__/cli.test.ts Outdated Show resolved Hide resolved

mehulkar force-pushed the mk/add-to-ci branch 4 times, most recently from 600dc50 to 6331015 Compare August 9, 2022 22:43

mehulkar requested a review from nathanhammond August 9, 2022 22:52

mehulkar force-pushed the mk/add-to-ci branch from 246a0ec to a4b7e10 Compare August 9, 2022 23:46

mehulkar commented Aug 9, 2022

View reviewed changes

packages/create-turbo/__tests__/cli.test.ts Outdated Show resolved Hide resolved

mehulkar commented Aug 9, 2022

View reviewed changes

mehulkar force-pushed the mk/add-to-ci branch 3 times, most recently from 8881c8a to 370d010 Compare August 10, 2022 15:59

vercel bot deployed to Preview August 10, 2022 16:41 View deployment

nathanhammond reviewed Aug 10, 2022

View reviewed changes

mehulkar force-pushed the mk/add-to-ci branch from 370d010 to d123dd2 Compare August 10, 2022 23:19

mehulkar requested a review from nathanhammond August 11, 2022 00:32

mehulkar changed the title ~~Add a new action to run create-turbo package tests in CI~~ create-turbo: improve test cases and make them pass Aug 11, 2022

mehulkar removed the pr: on hold Pull requests that are "on hold" and should not be merged label Aug 11, 2022

tknickman reviewed Aug 11, 2022

View reviewed changes

mehulkar requested a review from tknickman August 11, 2022 20:53

gsoltis reviewed Aug 11, 2022

View reviewed changes

tknickman approved these changes Aug 15, 2022

View reviewed changes

vercel bot deployed to Preview August 15, 2022 15:25 View deployment

mehulkar force-pushed the mk/add-to-ci branch from b7fa2bd to 0c84c2d Compare August 15, 2022 16:51

mehulkar force-pushed the mk/add-to-ci branch from 0c84c2d to 6ad5058 Compare August 15, 2022 17:19

Merge branch 'main' into mk/add-to-ci

8025d02

kodiakhq bot merged commit 8c98361 into vercel:main Aug 15, 2022

mehulkar deleted the mk/add-to-ci branch August 15, 2022 17:54

mehulkar mentioned this pull request Aug 16, 2022

Enable JS tests on Windows in CI #1702

Closed

create-turbo: improve test cases and make them pass #1655

create-turbo: improve test cases and make them pass #1655

Uh oh!

Conversation

mehulkar commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Aug 8, 2022

Uh oh!

Uh oh!

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vercel bot commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tknickman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsoltis left a comment

Choose a reason for hiding this comment

Uh oh!

mehulkar commented Aug 12, 2022

Uh oh!

gsoltis commented Aug 12, 2022

Uh oh!

mehulkar commented Aug 15, 2022

Uh oh!

gsoltis commented Aug 15, 2022

Uh oh!

mehulkar commented Aug 15, 2022

Uh oh!

Uh oh!

mehulkar commented Aug 8, 2022 •

edited

Loading

vercel bot commented Aug 10, 2022 •

edited

Loading