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

[Tech debt] Fix Jest unit tests on broken on React 17 #7227

Merged
merged 10 commits into from
Sep 29, 2023

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Sep 26, 2023

Summary

PR provides script commands to run .ts and .tsx tests against React [17, 18] with no errors. This PR will be combined with a few others to split our Buildkite test job to run in parallel.

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

Best way to QA will be to run the following commands after pulling the branch and yarn to ensure latest dependencies:

  • yarn test-unit --testMatch=non-react
  • yarn test-unit --react-version=17 --testMatch=react
  • yarn test-unit --react-version=16 --testMatch=react
  • yarn test-unit --testMatch=react
  • yarn test-unit for a final no-regressions check

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.

Adde a couple of notes for code change context.

package.json Outdated
Comment on lines 35 to 38
"test-unit:ts": "cross-env NODE_ENV=test jest --config ./scripts/jest/config.js --testMatch **/*.test.ts",
"test-unit:ts:17": "cross-env NODE_ENV=test REACT_VERSION=17 jest --config ./scripts/jest/config.js --testMatch **/*.test.ts",
"test-unit:tsx": "cross-env NODE_ENV=test jest --config ./scripts/jest/config.js --testMatch **/*.test.tsx",
"test-unit:tsx:17": "cross-env NODE_ENV=test REACT_VERSION=17 jest --config ./scripts/jest/config.js --testMatch **/*.test.tsx",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd previously discussed reviewers entering these commands into the CLI, but then I remembered how much of a pain they were to get running. I opted to add the short commands into package.json so they would be quicker to test and give myself the hooks I want for when I split the Buildkite job.

Copy link
Member

Choose a reason for hiding this comment

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

I would strongly prefer writing this as a script that accepts args instead of adding a bunch of extremely repetitive scripts to our package.json, so I went ahead and did so in c3072d8 and 9a7212d. Let me know if you have any questions - I've updated your PR description with the new corresponding flags, as well as updated our wiki docs. I'll (eventually) add comment to the original PR's docker commands as well with the new flags

Comment on lines 98 to 105
it('updates 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 added a second snapshot test for rendering at mobile size, and added this interaction test to verify the mobile behavior. That's why the snapshot shows reduced and changed lines.

- instead of creating scripts that devs aren't going to use
CLI will use this to test react vs non react files
- this is an `it renders` type test, so no getting around a snapshot for each version
…ement

- should only be passed to resize observer
@cee-chen cee-chen changed the title [Infra] Run Jest unit tests on React [17, 18] [Tech debt] Fix Jest unit tests on broken on React 17 Sep 28, 2023
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

LMK if you have any question about these changes Trevor, happy to walk through them! Feel free to merge if no other changes. yarn test-unit --react-version=17 passed with no failures for me locally.

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @1Copenut

@1Copenut
Copy link
Contributor Author

This all looks good to me. After talking with @tkajtoch && @cee-chen about the refactor and some particulars of shell variables vs. environment variables, I see this approach being a strong way forward and ways to use this refactor in other processes.

@1Copenut 1Copenut merged commit 01f23ab into elastic:main Sep 29, 2023
1 check passed
@1Copenut 1Copenut deleted the infra/react-17-jest branch September 29, 2023 16:26
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