-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: Add report section summarizing failing assertions from all tests in a plan #1288
Conversation
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 @stalgiag! Very impressive getting all this added and in a pretty maintainable way as well given where it has to be referenced.
The main issue I've come across is the handling of the priority 0 assertions but I left some inline suggestions on that. Seems this is all very close!
return ( | ||
<> | ||
<p> | ||
{metrics.assertionsFailedCount} assertions failed for{' '} |
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.
{metrics.assertionsFailedCount} assertions failed for{' '} | |
{failingAssertions.length} assertions failed for{' '} // or metrics.mustAssertionsFailedCount + metrics.shouldAssertionsFailedCount |
metrics.assertionsFailedCount
would also cover the MAY
assertions which isn't wanted here.
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.
I believe this is addressed by 79adb6d
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.
I believe this is addressed by 79adb6d
@stalgiag the underlying calculation is correct but the result of it isn't being used here. This suggestion should still be done because the MAY
assertions from the overall getMetrics
calculation is still being counted in it.
It makes it confusing when the result table only has 14 rows but the text notes 24 assertions failed for x commands in y tests
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.
Ah yes! Apologies. Brain fog must've been in charge that day. Thanks for sticking to your point 😅
Co-authored-by: Howard Edwards <[email protected]>
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.
@stalgiag the changes look good! I still have one pressing concern I left inline: #1288 (comment)
Looks good to go for me after.
Also, I'm realizing that my previous concern in our discussion from this thread may just have to do with the structure of the sentence. x assertions failed ACROSS y commands in z tests
feels better to me. But once again, maybe it's just my understanding being slightly off. But something to think on if it's raised in the future
I agree with "across" feeling better. I took the liberty of changing it 😄 72ba042 |
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.
@stalgiag one more inline change suggestion because of some confusion I introduced! 😅
Should be good to go afterwards
Co-authored-by: Howard Edwards <[email protected]>
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.
Changes look good and very exciting! Thanks for working through all the feedback items!
see #1253
This PR adds a report table summarizing all failing assertions to the Candidate Review and on Report pages.
There were a number of design and user experience decisions made here that I especially welcome feedback on. There were also a few decisions for which I was on the fence. Namely: