-
Notifications
You must be signed in to change notification settings - Fork 336
test: test infra #2085
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
test: test infra #2085
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! I had some comments
packages/core/jest.config.js
Outdated
// slowTestThreshold: 5, | ||
|
||
// A list of paths to snapshot serializer modules Jest should use for snapshot testing | ||
setupFilesAfterEnv: ["<rootDir>/setupTests.js"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure it should be running for every test? can it run globally in the global setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you can also do:
testEnvironment: 'jest-environment-jsdom',
https://testing-library.com/docs/react-testing-library/setup/#jest-27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of having this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like setupTest is still needed, but changed to jest-environment-jsdom anyway
packages/core/TESTING_README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is a bit outdated, I'll open a task to revisit it
"ts-loader": "^9.3.1", | ||
"ts-node": "^10.9.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To use TS config with Jest, (i.e. in node)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe upgrade the version here to be exactly like core, so it would be a shared dep between the two packages, just for convenience
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about it, I'll check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -2,6 +2,8 @@ const path = require("path"); | |||
|
|||
module.exports = { | |||
process(src, filename) { | |||
return `module.exports = ${JSON.stringify(path.basename(filename))};`; | |||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to Jest breaking chnages
packages/core/.eslintrc.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we even need the override here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lemme check, probably not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same - do we even need the override here?
@@ -340,7 +340,7 @@ export const TransitionAnimation = { | |||
]); | |||
|
|||
setNumActions(numActions + 1); | |||
}, [steps, setSteps, numActions, setNumActions]); | |||
}, [numActions, steps, getStepKeyForTransition, STEP_TRANSITIONS, clearSteps]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was missing
@@ -95,8 +95,7 @@ describe("useDebounceEvent", () => { | |||
const additionalDelay = 200; | |||
|
|||
beforeEach(() => { | |||
// @ts-ignore | |||
jest.useFakeTimers("modern"); | |||
jest.useFakeTimers(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API changed, this is the same
@@ -39,8 +39,7 @@ | |||
"lint:fix": "eslint \"./src/**/*.{js,jsx,ts,tsx}\" --fix", | |||
"prettier:fix": "prettier --write \"{,!(node_modules)/**/}*.{js,jsx,ts,tsx}\"", | |||
"test": "jest --passWithNoTests", | |||
"test:update": "yarn test -- -u", | |||
"test:stories": "testing=storybook TEST_END_FILES=jest-story jest --passWithNoTests", | |||
"test:update": "yarn test -u", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the --
due to yarn warning that it's not needed
This reverts commit 4cd0549.
packages/core/.eslintrc.js
Outdated
@@ -59,7 +59,7 @@ const commonExtends = ["plugin:react/recommended", "plugin:react-hooks/recommend | |||
module.exports = { | |||
overrides: [ | |||
{ | |||
files: ["*.jest.js", "jest.setup.js", "jest.init.js"], | |||
files: ["jest.init.js"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out it's needed for eslint, keeping it for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! we really needed this one!!
https://monday.monday.com/boards/3532714909/pulses/5657864174
https://monday.monday.com/boards/3532714909/pulses/6426652552