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

[INFRA] Parallelize the Buildkite test job on PR #7197

Merged
merged 12 commits into from
Sep 20, 2023
Merged

[INFRA] Parallelize the Buildkite test job on PR #7197

merged 12 commits into from
Sep 20, 2023

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Sep 18, 2023

Summary

This job will update our Buildkite test to parallelize linting, unit testing, and Cypress e2e testing against React [16, 17, 18].

QA

QA will be done manually by verifying the test runs on Buildkite UI.

  • Linter ran successfully
  • Jest unit tests ran
  • Cypress ran on React 16
  • Cypress ran on React 17
  • Cypress ran on React 18

Screen Shot 2023-09-19 at 1 14 47 PM

@1Copenut 1Copenut marked this pull request as ready for review September 19, 2023 18:15
@1Copenut 1Copenut requested a review from tkajtoch September 19, 2023 18:16
Comment on lines 4 to 10
- command: .buildkite/scripts/pipeline_test.sh
label: ":typescript: Linting"
agents:
provider: "gcp"
command: .buildkite/scripts/pipeline_test.sh
env:
TEST_TYPE: 'lint'
if: build.branch != "main" # We're skipping testing commits in main for now to maintain parity with previous Jenkins setup
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only conceit on this PR is to make the Docker image smaller, because each command is run by a separate agent with no knowledge of other agents. I'm seeing several opportunities to reduce the size and speed up our Docker image caching. I'll roll that work into removing the Puppeteer step from Dockerfile later this week or next.

docker.elastic.co/eui/ci:5.3
)

case $TEST_TYPE in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I used if / elif / fi blocks but it was getting hard to read. This felt cleaner and more scalable.

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @1Copenut

@1Copenut 1Copenut requested a review from a team September 20, 2023 13:41
Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

I'm no BuildKite expert, but this was very interesting to walk through! And to see all of these run in about 20 minutes is very cool! I'm not sure what it looked like with if/elif/fi, but the structure that you use in pipeline_test.sh feels very clean and easy to read.

@1Copenut 1Copenut merged commit 57ec944 into elastic:main Sep 20, 2023
1 check passed
@1Copenut 1Copenut deleted the infra/parallelize-test-job branch September 20, 2023 15:04
@cee-chen
Copy link
Contributor

  1. Why assign a PR to Tomasz and not wait for his review on it?
  2. Our Jest tests should be on multiple React versions as well, not just our Cypress tests. I believe we discussed this in our meeting yesterday - what happened with that?

@1Copenut
Copy link
Contributor Author

Fair questions. I looked at Tomasz's schedule today and it looked like he was extremely busy, so I widened the PR. I've got bandwidth this week, so I'll work on testing Jest against multiple React versions also.

@cee-chen
Copy link
Contributor

I recall requesting during yesterday's meeting that we update the issue to reflect that all tests, not just Cypress, should be run on different React versions. The issue should have been updated and this PR should not have been merged without fully completing the issue as specified (or if the PR was intended to only partially complete the task, that should have been made clear in the PR description and the issue should remain open).

@cee-chen
Copy link
Contributor

and it looked like he was extremely busy, so I widened the PR

I get that this happens, but in the future I strongly suggest verbally documenting that thought process and pinging the reviewer in question to confirm that that's the case. Or if there's a pressing need to merge sooner rather than later (e.g. release schedules, ongoing merge conflicts, etc.) I would at least add a postscript saying "[person who hasn't had time to review], please feel free to review post-merge and I'll open up a follow-up PR with requested changes". A little extra communication can go a long way to making a PR not feel like it was rushed through before someone had a chance to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants