Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Sep 14, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d90a70b
Switch pre-commit hook to pre-push
cee-chen Sep 6, 2023
c278544
Update `test-staged` to look at all commits from main, not just local…
cee-chen Sep 6, 2023
4fb17b8
Use simpler `--changedSince` jest flag
cee-chen Sep 11, 2023
a18b6e3
[EuiCard] Fix imports pointed directly at `src/components/`
cee-chen Sep 11, 2023
5d7a24d
[EuiPopover] Fix test import pointed at `src/components`
cee-chen Sep 11, 2023
1063a24
[EuiProvider] Fix imports pointed at `src/components`
cee-chen Sep 11, 2023
f80952b
[src-docs] Fix import string somehow triggering bizarre Jest find rel…
cee-chen Sep 11, 2023
e4b32fe
Fix `src/test` imports pointed at `src/components/`
cee-chen Sep 11, 2023
9b7f276
Add regression test for Jest `--findRelatedTests` behavior
cee-chen Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
"**/@types/react": "^18",
"react-focus-lock": "^2.9.5"
},
"pre-commit": [
"pre-push": [
"test-staged"
],
"dependencies": {
Expand Down Expand Up @@ -220,7 +220,7 @@
"postcss-inline-svg": "^4.1.0",
"postcss-loader": "^7.0.1",
"postcss-styled-syntax": "^0.4.0",
"pre-commit": "^1.2.2",
"pre-push": "^0.1.4",
"prettier": "^2.8.8",
"process": "^0.11.10",
"prop-types": "^15.6.0",
Expand Down
9 changes: 2 additions & 7 deletions scripts/test-staged.js
View file
Open in desktop
Copy link
Contributor Author

@cee-chen cee-chen Sep 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@cee-chen cee-chen Sep 7, 2023

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

@cee-chen cee-chen Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@breehall breehall Sep 7, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@1Copenut 1Copenut Sep 14, 2023

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
const { execSync } = require('child_process');

// find names of staged files
const stagedFiles = execSync('git diff --cached --name-only --diff-filter=ACMR')
.toString()
.split(/[\r\n]+/g);

// execute tests related to the staged files
execSync(`yarn test-unit --findRelatedTests ${stagedFiles.join(' ')}`, {
// https://jestjs.io/docs/cli#--changedsince
execSync(`yarn test-unit --changedSince main`, {
stdio: 'inherit',
});
24 changes: 24 additions & 0 deletions scripts/tests/test-staged.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
const { execSync } = require('child_process');
const path = require('path');
const euiRoot = path.resolve(__dirname, '../../'); // NOTE: CI lists the EUI project root as app/ instead of eui/

describe('test-staged.js', () => {
it('does not have --findRelatedTests regressions due to imports from src/components', () => {
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();
Comment on lines +7 to +20
Copy link
Contributor

@1Copenut 1Copenut Sep 14, 2023

Choose a reason for hiding this comment

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

This is an awesome way to test local infrastructure!


expect(output[2]).toBeUndefined();
});
});
8 changes: 4 additions & 4 deletions src-docs/src/components/guide_section/_utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const dedent = require('dedent');
const { renderJsSourceCode } = require('./_utils.js');
import dedent from 'dedent';
import { renderJsSourceCode } from './_utils';

describe('renderJsSourceCode', () => {
describe('EUI imports', () => {
Expand Down Expand Up @@ -41,8 +41,8 @@ describe('renderJsSourceCode', () => {
expect(
renderJsSourceCode({
default: dedent(`
import { EuiButton, EuiFlexGroup, EuiFlexItem } from '../../../src/components';
import { useGeneratedHtmlId } from '../../../src/services';
import { EuiButton, EuiFlexGroup, EuiFlexItem } from '../../src/components';
import { useGeneratedHtmlId } from '../../src/services';

export default () => <EuiCode>{useGeneratedHtmlId()}</EuiCode>;`),
})
Expand Down
3 changes: 2 additions & 1 deletion src/components/card/card.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { css } from '@emotion/react';
import { EuiCardProps } from '..';

import {
euiPaddingSize,
euiSupportsHas,
Expand All @@ -18,6 +18,7 @@ import {
import { UseEuiTheme } from '../../services';
import { euiButtonColor } from '../../themes/amsterdam/global_styling/mixins';

import { EuiCardProps } from './card';
const paddingKey = 'm';
const halfPaddingKey = 's';

Expand Down
8 changes: 5 additions & 3 deletions src/components/card/card.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ import { requiredProps } from '../../test';
import { shouldRenderCustomStyles } from '../../test/internal';
import { render } from '../../test/rtl';

import { EuiCard, ALIGNMENTS } from './card';

import { EuiIcon, EuiAvatar, EuiI18n } from '../../components';
import { EuiIcon } from '../icon';
import { EuiAvatar } from '../avatar';
import { EuiI18n } from '../i18n';
import { COLORS, SIZES } from '../panel/panel';

import { EuiCard, ALIGNMENTS } from './card';

describe('EuiCard', () => {
test('is rendered', () => {
const { container } = render(
Expand Down
10 changes: 6 additions & 4 deletions src/components/card/card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,23 @@ import React, {
} from 'react';
import classNames from 'classnames';

import { CommonProps, ExclusiveUnion } from '../common';
import {
getSecureRelForTarget,
useEuiTheme,
cloneElementWithCss,
} from '../../services';
import { useGeneratedHtmlId } from '../../services/accessibility';
import { validateHref } from '../../services/security/href_validator';

import { CommonProps, ExclusiveUnion } from '../common';
import { EuiText } from '../text';
import { EuiTitle } from '../title';
import { EuiBetaBadge, EuiBetaBadgeProps } from '../badge/beta_badge';
import { EuiIconProps } from '../icon';
import { EuiCardSelect, EuiCardSelectProps } from './card_select';
import { useGeneratedHtmlId } from '../../services/accessibility';
import { validateHref } from '../../services/security/href_validator';
import { EuiPanel, EuiPanelProps } from '../panel';
import { EuiSpacer } from '../spacer';

import { EuiCardSelect, EuiCardSelectProps } from './card_select';
import {
euiCardBetaBadgeStyles,
euiCardStyles,
Expand Down
6 changes: 3 additions & 3 deletions src/components/popover/popover.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import { act } from '@testing-library/react';
import { shouldRenderCustomStyles } from '../../test/internal';
import { requiredProps } from '../../test/required_props';
import { render } from '../../test/rtl';
import { EuiFocusTrap } from '../';

import { keys } from '../../services';
import { EuiFocusTrap } from '../focus_trap';

import {
EuiPopover,
Expand All @@ -21,8 +23,6 @@ import {
PopoverAnchorPosition,
} from './popover';

import { keys } from '../../services';

const actAdvanceTimersByTime = (time: number) =>
act(() => jest.advanceTimersByTime(time));

Expand Down
2 changes: 1 addition & 1 deletion src/global_styling/functions/typography.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import React, { FunctionComponent, PropsWithChildren } from 'react';
import { renderHook } from '@testing-library/react';

import { EuiProvider } from '../../components';
import { EuiProvider } from '../../components/provider';
import { useEuiTheme } from '../../services';
import { EuiThemeFontScales, EuiThemeFontUnits } from '../variables/typography';

Expand Down
2 changes: 1 addition & 1 deletion src/services/theme/hooks.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import React from 'react';
import { render, act } from '@testing-library/react';
import { renderHook } from '../../test/rtl';

import { EuiProvider } from '../../components';
import { EuiProvider } from '../../components/provider';

import { setEuiDevProviderWarning } from './warning';
import {
Expand Down
2 changes: 1 addition & 1 deletion src/test/internal/render_custom_styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { get, set } from 'lodash';

import { render } from '../rtl';
import { cloneElementWithCss } from '../../services';
import { keysOf } from '../../components';
import { keysOf } from '../../components/common';

export const customStyles = {
className: 'hello',
Expand Down
2 changes: 1 addition & 1 deletion src/test/rtl/custom_render.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
within,
} from '@testing-library/react';

import { EuiProvider } from '../../components';
import { EuiProvider } from '../../components/provider';

import * as dataTestSubjQueries from './data_test_subj_queries';

Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17532,10 +17532,10 @@ postcss@^8.4.24:
picocolors "^1.0.0"
source-map-js "^1.0.2"

pre-commit@^1.2.2:
version "1.2.2"
resolved "https://registry.yarnpkg.com/pre-commit/-/pre-commit-1.2.2.tgz#dbcee0ee9de7235e57f79c56d7ce94641a69eec6"
integrity sha1-287g7p3nI15X95xW186UZBpp7sY=
pre-push@^0.1.4:
version "0.1.4"
resolved "https://registry.yarnpkg.com/pre-push/-/pre-push-0.1.4.tgz#48e7a29e15d3d7a83b62d5a02e10ba5dc40ab3aa"
integrity sha512-bIdVuDQR3r5AWV7bM6OMHD3mCXA53Ql0LXmW5UfcSmJZq+J+TytqZ5YJcTmMLcojJysN65vcFIeCqRn6YidA+Q==
dependencies:
cross-spawn "^5.0.1"
spawn-sync "^1.0.15"
Expand Down