Skip to content

Implement numPassingAsserts of testCaseResult#13795

Merged
SimenB merged 29 commits into
jestjs:mainfrom
ymqy:feature/numPassingAsserts
Jan 26, 2023
Merged

Implement numPassingAsserts of testCaseResult#13795
SimenB merged 29 commits into
jestjs:mainfrom
ymqy:feature/numPassingAsserts

Conversation

@ymqy

@ymqy ymqy commented Jan 21, 2023

Copy link
Copy Markdown
Contributor

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

@SimenB SimenB left a comment

Copy link
Copy Markdown
Member

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
ymqy force-pushed the feature/numPassingAsserts branch from 973f6a6 to a423029 Compare January 23, 2023 12:10
@ymqy

ymqy commented Jan 23, 2023

Copy link
Copy Markdown
Contributor Author

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

ymqy commented Jan 24, 2023

Copy link
Copy Markdown
Contributor Author

@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.

Comment thread e2e/__tests__/customReporters.test.ts Outdated
],
{
env: {
JEST_JASMINE: '0',

@ymqy ymqy Jan 24, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ymqy ymqy Jan 24, 2023

Copy link
Copy Markdown
Contributor Author

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.

}
}

export function onNotJestJasmine(testBody: () => void): void {

Copy link
Copy Markdown
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
Copy Markdown
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'

}
}

export function skipTestOnJasmine(testBody: () => void): void {

Copy link
Copy Markdown
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

@ymqy ymqy Jan 25, 2023

Copy link
Copy Markdown
Contributor Author

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?

break;
}
case 'test_done': {
event.test.numPassingAsserts = jestExpect.getState().numPassingAsserts;

Copy link
Copy Markdown
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?

@ymqy ymqy Jan 25, 2023

Copy link
Copy Markdown
Contributor Author

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

SimenB commented Jan 25, 2023

Copy link
Copy Markdown
Member

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

@ymqy

ymqy commented Jan 25, 2023

Copy link
Copy Markdown
Contributor Author

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

ymqy commented Jan 26, 2023

Copy link
Copy Markdown
Contributor Author

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 SimenB left a comment

Copy link
Copy Markdown
Member

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

SimenB commented Jan 26, 2023

Copy link
Copy Markdown
Member

@SimenB

SimenB commented Jan 31, 2023

Copy link
Copy Markdown
Member

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

@ymqy

ymqy commented Jan 31, 2023

Copy link
Copy Markdown
Contributor Author

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 react/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 react/react that referenced this pull request Feb 10, 2023
@github-actions

github-actions Bot commented Mar 3, 2023

Copy link
Copy Markdown

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