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
SimenB
left a comment
There was a problem hiding this comment.
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.
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.
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.
Thanks, but it will skip all test of file, it might be better to find a way to skip the specific test.
| } | ||
| } | ||
|
|
||
| export function onNotJestJasmine(testBody: () => void): void { |
There was a problem hiding this comment.
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.
Thank you for the suggestion, I have updated the function name to 'skipTestOnJasmine'
| } | ||
| } | ||
|
|
||
| export function skipTestOnJasmine(testBody: () => void): void { |
There was a problem hiding this comment.
why is a new helper needed? shouldn't skipSuiteOnJasmine be enough? That one also supports snapshots properly
There was a problem hiding this comment.
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?
| break; | ||
| } | ||
| case 'test_done': { | ||
| event.test.numPassingAsserts = jestExpect.getState().numPassingAsserts; |
There was a problem hiding this comment.
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 |
SimenB
left a comment
There was a problem hiding this comment.
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
numPassingAssertsto track the number of passing asserts in a single testTest plan
ci green