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 Buildkite test job, take 2 #7209

Closed
wants to merge 30 commits into from

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Sep 20, 2023

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 using npm instead of yarn. This allows us to allocate more resources in Node to prevent test failures. See #7197 for the previous iteration of this PR.

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.

  • Linter ran successfully
  • Jest (.ts|.js) unit tests ran
  • Jest TSX ran on React 16
  • Jest TSX ran on React 17
  • Jest.tsx ran on React 18
  • Cypress ran on React 16
  • Cypress ran on React 17
  • Cypress ran on React 18

Copy link
Contributor Author

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

Left some inline comments explaining decisions. If we want to run snapshots against React 17 in cases where I've limited them to React 18, that should be easy enough. If the snapshots rewrite for different versions, I'll take that as a sign to refactor to unit tests.

Comment on lines +12 to +18
- command: .buildkite/scripts/pipeline_test.sh
label: ":jest: TS unit tests"
agents:
provider: "gcp"
env:
TEST_TYPE: 'unit:ts'
if: build.branch != "main"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once this PR merges, I want to find a way to build our Docker image once, and use a cached version for each command step. This works, but is downloading the Docker image multiple times so it's not as efficient as it could be.

I'm looking at a couple of options for building a safe cached Docker container that could be possibilities after I reduce the image complexity (Puppeteer user and associated libs).

Comment on lines 76 to 105
testOnReactVersion(['18'])(
'collapses from a push flyout to an overlay flyout once the screen is smaller than 3x the flyout width',
() => {
mockWindowResize(600);
const { baseElement } = render(
<EuiCollapsibleNavBeta>Nav content</EuiCollapsibleNavBeta>
);
expect(baseElement).toMatchSnapshot();
}
);

testOnReactVersion(['18'])(
'makes the overlay flyout full width once the screen is smaller than 1.5x the flyout width',
() => {
mockWindowResize(320);
const { baseElement } = render(
<EuiCollapsibleNavBeta>Nav content</EuiCollapsibleNavBeta>
);
expect(baseElement).toMatchSnapshot();
}
);

it('makes the overlay flyout full width once the screen is smaller than 1.5x the flyout width', () => {
it('dauptes the overlay controls once the screen is smaller than 1.5x the flyout width', () => {
mockWindowResize(320);
const { baseElement, getByTestSubject } = render(
const { baseElement, getByLabelText, getByTestSubject } = render(
<EuiCollapsibleNavBeta>Nav content</EuiCollapsibleNavBeta>
);
expect(getByLabelText('Toggle navigation open')).toBeInTheDocument();
fireEvent.click(getByTestSubject('euiCollapsibleNavButton'));
expect(baseElement).toMatchSnapshot();
expect(getByLabelText('Toggle navigation closed')).toBeInTheDocument();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored this to two React 18 snapshot tests and an actual unit test for mobile adaptive behavior on Cee's recommendation in a Slack chat. The prior snapshot tests were rewriting themselves when I ran them in React 16, so this felt a better approach with respect to test coverage.

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @1Copenut

@1Copenut 1Copenut marked this pull request as ready for review September 25, 2023 14:24
@1Copenut 1Copenut requested a review from a team as a code owner September 25, 2023 14:24
@1Copenut 1Copenut changed the title [WIP][INFRA] Parallelize Buildkite test job, take 2 [INFRA] Parallelize Buildkite test job, take 2 Sep 25, 2023
Comment on lines +16 to +17
} else {
Adapter = require('enzyme-adapter-react-16');
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, what tests were failing when this wasn't being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A large number of unit tests were failing, over a third, spread across multiple suites.

@1Copenut
Copy link
Contributor Author

1Copenut commented Oct 4, 2023

@1Copenut 1Copenut closed this Oct 4, 2023
@1Copenut 1Copenut deleted the infra/parallelize-test-job branch October 4, 2023 13:21
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.

4 participants