Fix test performance #169
Fix test performance #169
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| @@ -12,8 +10,13 @@ const config: InitialOptionsTsJest = { | |||
|
|
|||
| collectCoverage: true, | |||
| coverageDirectory: 'coverage', | |||
| coverageReporters: ['json', 'lcov', 'text'], | |||
| coverageReporters: ['lcov'], | |||
There was a problem hiding this comment.
We may eventually need the other reporters, but I think this is fine for now.
| "test-print-config": "jest --showConfig", | ||
| "test-e2e": "yarn workspace @monorepo/e2e-sample-app run test-e2e", | ||
| "test-e2e-open": "yarn workspace @monorepo/e2e-sample-app run test-e2e-open", | ||
| "test-e2e-ci": "start-server-and-test 'yarn run-samples' '9000|9001' 'yarn test-e2e'", | ||
| "eslint": "eslint --max-warnings 0 --ext .js,.jsx,.ts,.tsx", | ||
| "eslint-print-config": "eslint --print-config", | ||
| "jest": "TZ=UTC jest", |
There was a problem hiding this comment.
I think it's generally better to set env. variables when calling scripts, rather than inside scripts or programs.
Side note, patternfly-react also sets the timezone this way, see here.
|
/hold |
|
Some more updates incoming. |
openshift-ci
bot
added
the
do-not-merge/hold
vojtechszocs
force-pushed
the
fix-test-perf
branch
from
7925c66
to
3b19a89
Compare
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
| @@ -5,7 +5,7 @@ const config: InitialOptionsTsJest = { | |||
| preset: 'ts-jest/presets/js-with-ts', | |||
|
|
|||
| testMatch: ['**/*.test.(js|jsx|ts|tsx)'], | |||
| transformIgnorePatterns: ['/node_modules/(?!(@patternfly)/)'], | |||
| transformIgnorePatterns: ['/node_modules/'], | |||
There was a problem hiding this comment.
FYI this was one of the reasons of the slowdowns, since we instructed ts-jest to transform all PatternFly packages by default.
|
/retest |
|
@vojtechszocs: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/lgtm |
optimize test config for better performance (*)
maxWorkersto1when in CI environmentisolatedModulestotrueregardless of the environmentkeep using Jest
27.xbut updatetesting-librarydependencies@testing-library/reactmust be12.xbecause we're using React17.x@testing-library/domwas removed since it's pulled in via@testing-library/jest-domremove
import '@testing-library/jest-dom'in test filespackages/common/test-configs/setup-react.tsupdate
collectCoverageFromto exclude tests and Storybook storiesAccording to kulshekhar/ts-jest#259 (comment) Jest workers run independently from each other, therefore each worker runs TypeScript compilation on its own, which results in slower times (due to multiple parallel TS compiler executions) which are offset by workers running in parallel.
Local testing via
rm -rf /tmp/jest_rs ; yarn testshows that running all tests (includes code coverage collection) withisolatedModules: trueis ~3.5 times faster. Since we don't really need full type checking when running tests (already covered by our "build" scripts which fail upon any TS type errors), it makes sense to always useisolatedModules: true.