-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[APM] Script static checkers + precommit #77900
Conversation
Pinging @elastic/apm-ui (Team:apm) |
task: () => | ||
execa( | ||
'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.
Could we just do require.resolve('jest/bin/jest')
here? Not sure what the jest script written here does other than run the CLI.
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.
Is that the same as running yarn jest --config x-pack/plugins/apm/jest.config.js
? Because looks like the latter is a little slower to pick up files.
resolve(__dirname, './jest.js'), | ||
'--reporters', | ||
resolve(__dirname, './node_modules/jest-silent-reporter'), | ||
'--collect-coverage', |
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.
If this is just for precommit we might want to turn off coverage
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
@elasticmachine merge upstream |
💚 Build SucceededBuild metricsdistributable file count
History
To update your PR or re-run it, just comment with: |
fix, | ||
cache: true, | ||
extensions: ['.js', '.jsx', '.ts', '.tsx'], | ||
}); |
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.
Does this automatically find the right eslintrc?
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.
Yeah, looks like it.
|
||
const config = require('../jest.config.js'); | ||
|
||
const argv = [...process.argv.slice(2), '--config', JSON.stringify(config)]; |
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 this could/should be a path
const argv = [...process.argv.slice(2), '--config', JSON.stringify(config)]; | |
const argv = [...process.argv.slice(2), '--config', '../jest.config.js']; |
(might have to do something with path.resolve('..', 'jest.config.js')
to get the correct path)
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's the same approach as the top-level jest test runner uses.
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Adds a
precommit
script that can be used in.git/hooks/pre-commit.local
. Also scripts Jest and ESLint runs. Those scripts are able to gather files more quickly, resulting in a performance boost.To run the scripts:
./x-pack/plugins/apm/scripts/precommit
./x-pack/plugins/apm/scripts/jest
./x-pack/plugins/apm/scripts/eslint