Update TAP information for jck tests #504
Conversation
Update to add all test output information if diagnostic is 'all' Filter and summarize failure tests in a similar way to openjdk tests Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
|
Grinder: |
|
Update won't affect jdk tap files Grinder for jdk_custom still works fine https://ci.adoptium.net/job/Grinder/8992/ |
| @@ -159,10 +159,31 @@ sub resultReporter { | |||
| $numOfTotal++; | |||
| $tapString .= "not ok " . $numOfTotal . " - " . $testName . "\n"; | |||
| $tapString .= " ---\n"; | |||
| if (($diagnostic eq 'failure') || ($diagnostic eq 'all')) { | |||
There was a problem hiding this comment.
What is diagnostic used for? And why remove it?
There was a problem hiding this comment.
Can we change the logic to the following?
if (($diagnostic eq 'failure') || ($diagnostic eq 'all')) {
if ($buildList =~ /jck/) {...}
elsif ($buildList =~ /openjdk/) {...}
else {..}
}
There was a problem hiding this comment.
This is to parse, filter and record the failed test case name to a formatted tap file. Only make sense for diagnotic=failure. No need to do for diagnostic=noDetail and dianostic=all
There was a problem hiding this comment.
sure, we do not need to check for dianostic=all. The current if else block at line 181 is confusing.
There was a problem hiding this comment.
Yes, the negation is normally difficult to understand. However, the way you mentioned would make the majority of code (current else block) confusing because extra if else would be needed, which is not straightforward.
There was a problem hiding this comment.
I can update to
if (($diagnostic eq 'failure') && ($buildList =~ /jck/ || $buildList =~ /openjdk/)) {
} else {
}
The difference would be the longer code block under the if branch and the one-liners of an exception move to else branch. Initially I think it may be more readable to keep the longer code block under the else branch. But I'm ok with the change.
|
Seems like this review is being held up over differing preferences for code style and readability. Given the related aqa-tests PR is merged (before this PR which it depends on), can we proceed to bring this in and update style later? I am worried to hit a bunch of test jobs failing tonight otherwise. ? I am also, waiting on this to merge before creating tags and release branch. |
|
sigh, I did not notice adoptium/aqa-tests#5106 was merged despite there being the I merge this in to unblock other activities. |
|
thanks @llxia ! I will proceed with adoptium/aqa-tests#5549 now. |
Update failed test target output parse in tap file to avoid the information cut and missing
Update jck test cases failed information to be the same way as openjdk tests for diagnostic=failure
Close adoptium/aqa-tests#439