-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Get rid of succ & fail marks on rate summary #3765
base: master
Are you sure you want to change the base?
Conversation
@@ -23,9 +23,9 @@ const ( | |||
" ✓ check1\n" + | |||
" ✗ check3\n ↳ 66% — ✓ 10 / ✗ 5\n" + | |||
" ✗ check2\n ↳ 33% — ✓ 5 / ✗ 10\n\n" + | |||
" ✓ checks......: 75.00% ✓ 45 ✗ 15 \n" | |||
" ✓ checks......: 75.00% 45 out of 60\n" |
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.
It seems inconsistent with the check1
, check2
, and check3
since they still use icons. Do you think so?
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 think the reason is because one is stored as a Rate
metric, so it is handled as such, while the other falls under the summarizeCheck
path.
That said, as I mentioned in the description, I didn't modify summarizeCheck
because I feel that the use of icons for that may still be useful (because there the use of ✓ always has a positive meaning).
However, it's true that once all together, it looks a bit inconsistent.
So, would you suggest to use out of
everywhere?
cc/ @oleiade @codebien @mstoykov (as you already shared some feedback before around this discussion) 🙇🏻
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.
Isn't still dependent from what is the meaning of the check? For example if check3
would be something like whatever_metric_failed
wouldn't be still confusing?
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.
Well... yes and no 😅
Technically, a check that succeeds is always ✓, while a rate metric is just a number.
Think analogously in programming terms, if the code execution enters into an if
statement, it's always because the condition is satisfied, even if the condition reflects a negative event (e.g. if failed { ... }
), but a rate is just a variable that holds a number, where the number might reflect succeeds or fails. So, the ambiguity is different/smaller.
That said, I agree that could make sense to change it everywhere. So, let's open the discussion widely and see what other people prefers, honestly I don't have strong preferences.
This PR aligns with our goal and designs to refactor the end-of-test summary. As a matter of fact, we integrated its approach in our designs. From my perspective, this is good to go and merge, in principle 👍🏻 |
What?
It replaces the use of succ & fail marks (✓/✗) for rate metrics on summary, in favor of the text:
{success} out of {total}
.Note: the check summary remains the same, as for checks there's no ambiguity on the use of ✓ and ✗.
Why?
Because as discussed in #2306 and #3656, the current form is sometimes confusing, because the metric may have either a "positive" meaning (like successful requests) or a "negative" meaning (like failed requests), for which the use of ✓ may result in confusions, especially for the latter.
Checklist
make lint
) and all checks pass.make tests
) and all tests pass.Related PR(s)/Issue(s)
Closes #2306
Second iteration of #3656, based on what was discussed there.