-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Implement numPassingAsserts
of testCaseResult
#13795
Conversation
numPassingAsserts
for custom reporter
numPassingAsserts
for custom reporternumPassingAsserts
of testResult for custom reporter
numPassingAsserts
of testResult for custom reporternumPassingAsserts
of testResult
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.
thanks! can you add a unit or e2e test as well verifying the count is correct for both passing and failing assertions?
973f6a6
to
a423029
Compare
Thank you for your feedback. I will definitely add unit or e2e test to verify the correctness of the numPassingAsserts count for both passing and failing assertions. I will make sure to update the pull request with the new test cases and let you know once it's ready for review.
|
@SimenB Added unit and e2e tests for numPassingAsserts count. Please let me know if there is anything else I can do to improve the implementation. |
], | ||
{ | ||
env: { | ||
JEST_JASMINE: '0', |
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 it feasible to hard code jest-circus as the testRunner here? because numPassingAsserts is not implemented in jest-jasmine.
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.
You could use this helper (search the repo for usage examples): https://github.com/facebook/jest/blob/3d7a096680078854b06c213403d063128386ab1f/packages/test-utils/src/ConditionalTest.ts#L18
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.
Thanks, but it will skip all test of file, it might be better to find a way to skip the specific test.
@@ -46,3 +46,9 @@ export function onNodeVersions( | |||
}); | |||
} | |||
} | |||
|
|||
export function onNotJestJasmine(testBody: () => void): void { |
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.
Perhaps skipTestOnJasmine
would be better name in this case?
export function onNotJestJasmine(testBody: () => void): void { | |
export function skipTestOnJasmine(testBody: () => void): void { |
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.
Thank you for the suggestion, I have updated the function name to 'skipTestOnJasmine'
@@ -46,3 +46,9 @@ export function onNodeVersions( | |||
}); | |||
} | |||
} | |||
|
|||
export function skipTestOnJasmine(testBody: () => void): void { |
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 a new helper needed? shouldn't skipSuiteOnJasmine
be enough? That one also supports snapshots properly
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.
skipSuiteOnJasmine
will skip all test of file, or is it better to create a separate test file to verify the assertion count and call skipSuiteOnJasmine in the jasmine runtime environment?
@@ -238,6 +238,7 @@ const eventHandler = async (event: Circus.Event) => { | |||
break; | |||
} | |||
case 'test_done': { | |||
event.test.numPassingAsserts = jestExpect.getState().numPassingAsserts; |
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.
should we reset it?
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.
numPassingAsserts
is reset to 0 after execution of _addExpectedAssertionErrors
, reset method is called within jestExpect.extractExpectedAssertionsErrors
method.
How does this work with |
Thanks for bringing this to our attention. You're correct that when using |
numPassingAsserts
of testResultnumPassingAsserts
of testCaseResult
Tried to run my own project branch (ymqy:feature/numPassingAsserts) in Circle CI(Avoid pushing too many commit records), not sure why it affects the current PR's CI check, Submitted a request to the Circle support team to resolve this issue |
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.
CI seems happy, so happy to land this. Thanks!
I don't work at Meta or help maintain React, so I'm not much help outside of Jest itself. |
thanks |
Minor version bump to get the fix for `numPassingAsserts`: jestjs/jest#13795 Test Plan: CI
Minor version bump to get the fix for `numPassingAsserts`: jestjs/jest#13795 Test Plan: CI
Minor version bump to get the fix for `numPassingAsserts`: jestjs/jest#13795 Test Plan: CI DiffTrain build for [2de85d7](2de85d7) [View git log for this commit](https://github.com/facebook/react/commits/2de85d7c712ff0f052d9c92f8129ed476f8ce4d8)
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
numPassingAsserts
to track the number of passing asserts in a single testTest plan
ci green