This is an archive of the discontinued LLVM Phabricator instance.

[lit] Improve naming of test result categories
ClosedPublic

Authored by yln on Apr 7 2020, 11:21 PM.

Details

Summary

Improve consistency when printing test results:
Previously we were using different labels for group names (the header
for the list of, e.g., failing tests) and summary count lines. For
example, "Failing Tests"/"Unexpected Failures". This commit changes lit
to label things consistently.

Improve wording of labels:
When talking about individual test results, the first word in
"Unexpected Failures", "Expected Passes", and "Individual Timeouts" is
superfluous. Some labels contain the word "Tests" and some don't.
Let's simplify the names.

Before:

Failing Tests (1):
  ...

Expected Passes    : 3
Unexpected Failures: 1

After:

Failed Tests (1):
  ...

Passed: 3
Failed: 1

Diff Detail

Event Timeline

yln created this revision.Apr 7 2020, 11:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2020, 11:21 PM
yln added a comment.Apr 7 2020, 11:24 PM

Happy to bikeshed the names here.

ldionne accepted this revision.Apr 8 2020, 6:58 AM

I like this.

Did you check whether other parts of LLVM had tests relying on this output? Otherwise, LGTM.

This revision is now accepted and ready to land.Apr 8 2020, 6:58 AM
rnk added a comment.Apr 8 2020, 8:30 AM

I kind of like "Passes", "Failures", "Expected Failures" better than Pass-*ing* Fail-*ing* etc. But I do think we can drop "Expected" from "Expected Passes".

To be consistent, you could change the text "Failing Tests (1):" to "Test Failures (1):"

jdenny added a comment.Apr 8 2020, 8:55 AM
In D77708#1969638, @rnk wrote:

I kind of like "Passes", "Failures", "Expected Failures" better than Pass-*ing* Fail-*ing* etc. But I do think we can drop "Expected" from "Expected Passes".

I like "Expected Passes" to clarify it does not include unexpected passes. This would mostly be helpful for developers used to test harnesses without an XFAIL concept.

To be consistent, you could change the text "Failing Tests (1):" to "Test Failures (1):"

I do like this kind of improved consistency.

By the way, are there any bots or other runners that grep for the current keywords when presenting results?

yln added a comment.Apr 8 2020, 3:19 PM

I am happy to hear that other people also value improving consistency! :)

In D77708#1969638, @rnk wrote:

I kind of like "Passes", "Failures", "Expected Failures" better than Pass-*ing* Fail-*ing* etc. But I do think we can drop "Expected" from "Expected Passes".

To be consistent, you could change the text "Failing Tests (1):" to "Test Failures (1):"

I feel the same about "Passes"/"Failures" vs. "Passing"/"Failing". I can't even give a good reason other than that to me it sounds better. Maybe because I am just used to it by now.
The issue with the old naming is that some labels are nouns and some are adjectives. If everything follows the same scheme we can print the label in the summary and use "<label> Tests" as the group headings. How about "Passed"/"Failed"?

In D77708#1969638, @rnk wrote:

I kind of like "Passes", "Failures", "Expected Failures" better than Pass-*ing* Fail-*ing* etc. But I do think we can drop "Expected" from "Expected Passes".

I like "Expected Passes" to clarify it does not include unexpected passes. This would mostly be helpful for developers used to test harnesses without an XFAIL concept.

I think for new developers and people who can be blissfully ignorant of lit's XFAIL/XPASS the following is better

Passing: 3
Failing: 1

than this:

Expected Passes    : 3
Unexpected Failures: 1

For devs who need to care the "normal" is short and the "odd" is long and ugly to draw attention:

Expectedly Failing:   1
Unexpectedly Passing: 1

Did you check whether other parts of LLVM had tests relying on this output? Otherwise, LGTM.

I will run ninja check-all and grep for the old names before landing this.

jdenny added a comment.Apr 8 2020, 4:32 PM
In D77708#1970510, @yln wrote:

For devs who need to care the "normal" is short and the "odd" is long and ugly to draw attention:

Expectedly Failing:   1
Unexpectedly Passing: 1

I'm not sure what you're saying here. Could you explain a bit more?

jdenny added a comment.Apr 8 2020, 5:07 PM
In D77708#1970510, @yln wrote:

For devs who need to care the "normal" is short and the "odd" is long and ugly to draw attention:

Expectedly Failing:   1
Unexpectedly Passing: 1

I'm not sure what you're saying here. Could you explain a bit more?

Ah, nevermind. After a second look, I got your meaning.

People can certainly learn that, by Passes, lit doesn't really mean all passes. I'd prefer less ambiguous terminology, but I'm in the minority so far, and it's not a big deal.

Are we moving forward with some version of this patch?

yln updated this revision to Diff 264307.May 15 2020, 12:04 PM

Rebase onto master. Rename Passing -> Passed and Failing -> Failed.

yln added a comment.EditedMay 15 2020, 12:19 PM

Are we moving forward with some version of this patch?

Yes, I would still like to if we can settle on a naming scheme. I think that most of the preference here comes from familiarity/what sounds best, which of course is a bit subjective.

I renamed "Passing -> Passed" and "Failing -> Failed". All category headers (the header printed before the list of tests in that group) are now adjective + "Test". For summary counts we use just the adjective describing the category. Currently, @rnk still prefers to old names. @rnk: is the updated scheme acceptable?

ninja check-all shows no failing tests due to this change for me.

After we settle on a final name, I will do a search for the changed strings in the entire repository.

yln edited the summary of this revision. (Show Details)May 27 2020, 2:23 PM
yln added a comment.May 28 2020, 11:12 AM

I have noticed that Reid's profile says that he will be away "between June 2020 and October 2020", so I don't think we want to wait on him. I will give people another week to chime in on the current naming scheme and then land this.

yln added a comment.Jun 5 2020, 8:17 AM

Landing now; standing by the next few hours for any fallout.

This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 5 2020, 8:22 AM