Switch `pre-commit` hook to `pre-push`; fix Jest `--findRelatedTests` behavior #7163

cee-chen · 2023-09-06T22:02:46Z

Summary

After a recent meeting, the EUI team discussed the friction point of the current pre-commit hook and that we feel it currently hinders developers more than it helps. We agreed to move it to a pre-push hook instead, ~~and additionally switch it to only linting and not running unit tests.~~ (see below git thread)

Note - if absolutely necessary, this hook can be skipped via git push --no-verify.

QA

NOTE: Once this PR merges, devs will need to manually run rm -rf .git/hooks/pre-commit from the EUI project root

General checklist

N/A, tech debt/dev-only

cee-chen · 2023-09-06T22:14:08Z

scripts/test-staged.js

Couple quick notes - I played around with trying to keep the idea of the "test only files in the commit(s) as opposed to every single test" with the following commands:

git diff main...HEAD

yarn test-unit --changedSince main (docs)

Unfortunately, when I tested both by adding a small comment to, e.g. collapsible_nav_beta.tsx which no other component in the EUI library imports, Jest's logic for determining related tests seemed to go haywire and it started running thousands of other unrelated tests.

I'm not totally sure why the above was happening, but I don't think it's super worth investigating why (unless someone else can figure it out in under 30 mins here). CI will catch any test failures in any case and we're at least on-par with Kibana in that regard.

@tkajtoch has discovered that Jest's --findRelatedTests and --changedSince are failing because of several instances where we import from the top level components/index.ts, which causes the jest dependency tree to always resolve the whole src/ directory.

I'll look into fixing that and linting for it separately from this PR, but out of curiosity, how do you all feel about running test locally on push as opposed to in CI? Do we definitely still want that, or stick to only linting?

Actually, on second thought, I do think we do have to run tests on some hook before it reaches remote, regardless of EUI friction. The use case I'm thinking of here is open source/community non-Elastic developers who contribute to EUI - CI doesn't run for them automatically, so they lose the test feedback if we drop this from our pre-PR hooks.

Actually, on second thought, I do think we do have to run tests on some hook before it reaches remote

If it has to be done, I think pre-push is the right place to have it. If the friction becomes too much, we can still use -n to skip the tests and allow CI to take care of it.

I concur that pre-push strikes a good balance. If I'm moving fast and making a lot of atomic commits, I tend to skip the test run pre-commit, then forget to run tests before pushing and end up spending more time if the CI fails. Having that scoped test run before code makes it to the full job may take an extra 5-10 minutes, but end up saving 30 or more.

… staged files

which messes up Jest's logic for finding related tests

…ated tests bug?? - the `require`->`import` changes are syntactical sugar only, the main fix is line 44 (and no I don't understand why Jest is looking at strings lol)

- which again, greatly messes up Jest's find related tests logic

- a lint rule might be better long-term, but this will at least warn us if our hook starts running extra tests again

kibanamachine · 2023-09-12T01:05:06Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7163/

elasticmachine · 2023-09-12T01:11:38Z

💚 Build Succeeded

Buildkite Build
Commit: 9b7f276

History

💔 Build #592 failed 28476ad
💔 Build #591 failed 66ce161
💔 Build #589 failed 977862c
💚 Build #534 succeeded eea7a33

cc @cee-chen

cee-chen · 2023-09-12T14:05:03Z

@elastic/eui-team This is ready for re-review whenever!

1Copenut

👍 LGTM! I ran through your test case to see the console output. Very cool way to use the Jest CLI to confirm behavior.

This is going to be a net improvement by failing fast locally instead of failing slower on remote CI jobs.

1Copenut · 2023-09-14T17:21:29Z

scripts/tests/test-staged.test.js

+    const outputString = execSync(
+      'yarn test-unit --findRelatedTests --listTests src/components/beacon/beacon.tsx',
+      { cwd: euiRoot }
+    ).toString();
+    const output = outputString
+      .split('\n')
+      .filter((item) => item.endsWith('test.tsx'));
+
+    expect(
+      output[0].endsWith('src/components/tour/tour_step.test.tsx')
+    ).toBeTruthy();
+    expect(
+      output[1].endsWith('src/components/beacon/beacon.test.tsx')
+    ).toBeTruthy();


This is an awesome way to test local infrastructure!

1Copenut · 2023-09-14T17:25:08Z

scripts/test-staged.js

I concur that pre-push strikes a good balance. If I'm moving fast and making a lot of atomic commits, I tend to skip the test run pre-commit, then forget to run tests before pushing and end up spending more time if the CI fails. Having that scoped test run before code makes it to the full job may take an extra 5-10 minutes, but end up saving 30 or more.

cee-chen · 2023-09-14T17:56:14Z

Thanks Trevor! I'm going to merge this in because honestly I'd really like this change for myself locally (will speed up my dev experience quite a bit, and save my wrist on extra clicks in vscode to commit w/ no verify 😅). But as always please feel free to leave post-merge comments and I can follow-up in another PR, or feel free to or open your own follow-up PR (mostly @tkajtoch, I do see your 👀 reaction).

cee-chen added tech debt skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) labels Sep 6, 2023

cee-chen self-assigned this Sep 6, 2023

cee-chen requested a review from a team September 6, 2023 22:11

cee-chen marked this pull request as ready for review September 6, 2023 22:11

cee-chen commented Sep 6, 2023

View reviewed changes

cee-chen added 7 commits September 11, 2023 14:57

Switch pre-commit hook to pre-push

d90a70b

Update test-staged to look at all commits from main, not just local…

c278544

… staged files

Use simpler --changedSince jest flag

4fb17b8

[EuiCard] Fix imports pointed directly at src/components/

a18b6e3

which messes up Jest's logic for finding related tests

[EuiPopover] Fix test import pointed at src/components

5d7a24d

which messes up Jest's logic for finding related tests

[EuiProvider] Fix imports pointed at src/components

1063a24

[src-docs] Fix import string somehow triggering bizarre Jest find rel…

f80952b

…ated tests bug?? - the `require`->`import` changes are syntactical sugar only, the main fix is line 44 (and no I don't understand why Jest is looking at strings lol)

cee-chen force-pushed the git-pre-commit-push branch from eea7a33 to f80952b Compare September 11, 2023 22:03

Fix src/test imports pointed at src/components/

e4b32fe

- which again, greatly messes up Jest's find related tests logic

cee-chen changed the title ~~Switch pre-commit hook to pre-push~~ Switch pre-commit hook to pre-push; fix Jest --findRelatedTests behavior Sep 11, 2023

cee-chen force-pushed the git-pre-commit-push branch from 977862c to 66ce161 Compare September 11, 2023 23:05

Add regression test for Jest --findRelatedTests behavior

9b7f276

- a lint rule might be better long-term, but this will at least warn us if our hook starts running extra tests again

cee-chen force-pushed the git-pre-commit-push branch from 28476ad to 9b7f276 Compare September 12, 2023 00:40

1Copenut approved these changes Sep 14, 2023

View reviewed changes

cee-chen merged commit d456136 into elastic:main Sep 14, 2023

cee-chen deleted the git-pre-commit-push branch September 14, 2023 17:56

Switch pre-commit hook to pre-push; fix Jest --findRelatedTests behavior #7163

Switch pre-commit hook to pre-push; fix Jest --findRelatedTests behavior #7163

Uh oh!

Conversation

cee-chen commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Uh oh!

cee-chen Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

cee-chen Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breehall Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

1Copenut Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Sep 12, 2023

Uh oh!

elasticmachine commented Sep 12, 2023

💚 Build Succeeded

History

Uh oh!

cee-chen commented Sep 12, 2023

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

1Copenut Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

1Copenut Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Sep 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Switch `pre-commit` hook to `pre-push`; fix Jest `--findRelatedTests` behavior #7163

Switch `pre-commit` hook to `pre-push`; fix Jest `--findRelatedTests` behavior #7163

cee-chen commented Sep 6, 2023 •

edited

Loading

cee-chen Sep 6, 2023 •

edited

Loading

cee-chen Sep 7, 2023 •

edited

Loading