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

Implement numPassingAsserts of testCaseResult #13795

Merged
merged 29 commits into from
Jan 26, 2023

Conversation

ymqy
Copy link
Contributor

@ymqy ymqy commented Jan 21, 2023

Summary

  • implement numPassingAsserts to track the number of passing asserts in a single test
  • refactor the implementation of numPassingAsserts in testResult object
  • make numPassingAsserts actual value available in custom reporter

Test plan

ci green

@ymqy ymqy changed the title Implement numPassingAsserts for custom reporter Implement numPassingAsserts for custom reporter Jan 21, 2023
@ymqy ymqy changed the title Implement numPassingAsserts for custom reporter Implement numPassingAsserts of testResult for custom reporter Jan 21, 2023
@ymqy ymqy changed the title Implement numPassingAsserts of testResult for custom reporter Implement numPassingAsserts of testResult Jan 22, 2023
Copy link
Member

@SimenB SimenB left a 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?

@ymqy
Copy link
Contributor Author

ymqy commented Jan 23, 2023

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.

thanks! can you add a unit or e2e test as well verifying the count is correct for both passing and failing assertions?

@ymqy
Copy link
Contributor Author

ymqy commented Jan 24, 2023

@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',
Copy link
Contributor Author

@ymqy ymqy Jan 24, 2023

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@ymqy ymqy Jan 24, 2023

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 {
Copy link
Contributor

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?

Suggested change
export function onNotJestJasmine(testBody: () => void): void {
export function skipTestOnJasmine(testBody: () => void): void {

Copy link
Contributor Author

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 {
Copy link
Member

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

Copy link
Contributor Author

@ymqy ymqy Jan 25, 2023

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;
Copy link
Member

Choose a reason for hiding this comment

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

should we reset it?

Copy link
Contributor Author

@ymqy ymqy Jan 25, 2023

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.

https://github.com/facebook/jest/blob/d683aafde24f2cf6f6cb5a0a71069fd6a0a55c36/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts#L249-L253

@SimenB
Copy link
Member

SimenB commented Jan 25, 2023

How does this work with jest.retryTimes? I assume it'd be confused. Not sure how to deal with it either, so...

@ymqy
Copy link
Contributor Author

ymqy commented Jan 25, 2023

How does this work with jest.retryTimes? I assume it'd be confused. Not sure how to deal with it either, so...

Thanks for bringing this to our attention. You're correct that when using jest.retryTimes, numPassingAsserts will only save the assertion count of the last test run. To access the numPassingAsserts of each retried test in a custom reporter, you can access it from the onTestCaseResult event. If you're only interested in the last retried test, you can access it from the onTestFileResult event. This event only saves the testCaseResult of the last retried test.

@ymqy ymqy changed the title Implement numPassingAsserts of testResult Implement numPassingAsserts of testCaseResult Jan 26, 2023
@ymqy
Copy link
Contributor Author

ymqy commented Jan 26, 2023

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

Copy link
Member

@SimenB SimenB left a 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!

@SimenB SimenB merged commit c78905c into jestjs:main Jan 26, 2023
@ymqy ymqy deleted the feature/numPassingAsserts branch January 26, 2023 14:41
@SimenB
Copy link
Member

SimenB commented Jan 26, 2023

@SimenB
Copy link
Member

SimenB commented Jan 31, 2023

I don't work at Meta or help maintain React, so I'm not much help outside of Jest itself.

@ymqy
Copy link
Contributor Author

ymqy commented Jan 31, 2023

I don't work at Meta or help maintain React, so I'm not much help outside of Jest itself.

thanks

kassens added a commit to kassens/react that referenced this pull request Feb 10, 2023
Minor version bump to get the fix for `numPassingAsserts`: jestjs/jest#13795

Test Plan:
CI
kassens added a commit to facebook/react that referenced this pull request Feb 10, 2023
Minor version bump to get the fix for `numPassingAsserts`:
jestjs/jest#13795

Test Plan:
CI
github-actions bot pushed a commit to facebook/react that referenced this pull request Feb 10, 2023
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)
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants