-
Notifications
You must be signed in to change notification settings - Fork 841
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
[INFRA] Parallelize Buildkite test job, take 3 #7242
[INFRA] Parallelize Buildkite test job, take 3 #7242
Conversation
@cee-chen Feel free to add reviewers if you'd like a second look at the Bash script updates. |
|
||
cypress:18) | ||
echo "[TASK]: Running Cypress tests against React 18" | ||
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn test-cypress --node-options=--max_old_space_size=2048") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to yarn cypress install
for these tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good call, I meant to remove those. I'll update quick. Maybe? IIRC, the test logs ignore the install because it's already added in the Dockerfile
. I think we could be more discerning and add Cypress here or a better approach might be to pin the Dockerfile Cypress install to the same version we're using in our package.json
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a better approach might be to pin the Dockerfile Cypress install to the same version we're using in our package.json file.
I'm not a super huge fan of that because it's another fragility point between local and CI that could fall out of date if we forget (e.g. we update package.json but not the dockerfile). It's 100% happened in the past before as well, so I'd rather the Docker container reuse package.json
for simplicity if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's valid. I'll add the yarn install cypress
back to these three Cypress run calls so we have an "at the metal" install.
.buildkite/scripts/pipeline_test.sh
Outdated
|
||
cypress:17) | ||
echo "[TASK]: Running Cypress tests against React 17" | ||
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn install cypress && yarn test-cypress --node-options=--max_old_space_size=2048 --react-version=17") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I brainfarted on the command - it looks like it's yarn cypress install
not yarn install cypress
, which is why CI is failing/complaining
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn install cypress && yarn test-cypress --node-options=--max_old_space_size=2048 --react-version=17") | |
DOCKER_OPTIONS+=(bash -c "/opt/yarn*/bin/yarn && yarn cypress install && yarn test-cypress --node-options=--max_old_space_size=2048 --react-version=17") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although, wait, hang the F on lol, if yarn cypress
already exists why are we calling install on it again?....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay go ahead and revert that last commit, I have no idea what yarn cypress install
was even doing in the first place haha 💀
- "cypress/screenshots/**/*.png" | ||
|
||
- command: .buildkite/scripts/pipeline_test.sh | ||
label: ":cypress: Cypress tests on React 18" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these labels and icons are heckin beautiful!
Buildkite test this |
Preview staging links for this PR:
|
💚 Build Succeeded
History
cc @1Copenut |
Summary
This PR is a second attempt to split our Buildkite
test
job and run the linter, unit tests, and Cypress tests in parallel. It switches back to usingnpm
instead ofyarn
. This allows us to allocate more resources in Node to prevent test failures. See #7209 for the previous iteration of this PR. I will close the previous PR when this one merges.This improved version will split Jest TSX tests and Cypress tests to run against React version
[16, 17, 18]
.QA
QA will be done manually by verifying the test runs on Buildkite UI.
(.ts && .js)
unit tests ran.tsx
ran on React 16.tsx
ran on React 17.tsx
ran on React 18