Skip to content
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

CPP-733 Jest plugin #160

Merged
merged 7 commits into from
Jan 18, 2022
Merged

CPP-733 Jest plugin #160

merged 7 commits into from
Jan 18, 2022

Conversation

serena97
Copy link
Contributor

There is no well documented way run jest programmatically (issue) so I have used the cli to run jest

@serena97 serena97 requested a review from a team as a code owner January 13, 2022 09:10
Copy link
Contributor

@ivomurrell ivomurrell left a comment

Choose a reason for hiding this comment

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

A few nitpicks but this all looks good imo!

@@ -34,7 +34,10 @@
"@dotcom-tool-kit/node": "file:../../plugins/node"
},
"husky": {
"pre-commit": "dotcom-tool-kit git:precommit"
"pre-commit": "dotcom-tool-kit git:precommit",
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: this line can now be safely deleted after #157

Suggested change
"pre-commit": "dotcom-tool-kit git:precommit",

"license": "ISC",
"dependencies": {
"@dotcom-tool-kit/types": "file:../../lib/types",
"jest-cli": "^27.4.7"
Copy link
Contributor

Choose a reason for hiding this comment

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

question: would it be better to have jest-cli as a peer dependency so that the consumer can use their own copy? we aren't consistent with this at the moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great suggestion 👍

if (code === 0) {
resolve()
} else {
reject(new Error(`Jest returned an error`))
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: we should only throw ToolKitError from our code

Suggested change
reject(new Error(`Jest returned an error`))
reject(new ToolKitError(`Jest returned an error`))


const jestCLIPath = require.resolve('jest-cli/bin/jest')

export default function runJest(mode: string, options: JestOptions) : Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: we should limit the mode type to the two possible string literals it is allowed to be

Suggested change
export default function runJest(mode: string, options: JestOptions) : Promise<void> {
type JestMode = "ci" | "local"
export default function runJest(mode: JestMode, options: JestOptions) : Promise<void> {

@@ -17,7 +17,7 @@ console.log('📦 initialising package')
execSync('npm init -y --scope @dotcom-tool-kit')

console.log('📥 installing dependencies')
execSync('npm install ../task')
execSync('npm install ../../lib/types')
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: nice catch!


jest.mock('child_process', () => ({
fork: jest.fn(() => {
// return a fake emitter that immediately sends an "exit" event, so the webpack task resolves
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: copy and paste error 😄

Suggested change
// return a fake emitter that immediately sends an "exit" event, so the webpack task resolves
// return a fake emitter that immediately sends an "exit" event, so the jest task resolves

@serena97 serena97 merged commit b953d4b into main Jan 18, 2022
@serena97 serena97 deleted the jest-plugin branch January 18, 2022 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants