-
-
Notifications
You must be signed in to change notification settings - Fork 17
Prettify output and warning to not be deceived by summary #40
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
Conversation
utils/github_reporting.py
Outdated
output_string = "{}\n" \ | ||
"> [!WARNING]\n" \ | ||
"> Failed tests may be incomplete if they failed to " \ | ||
"upload, always check the complete test suite link.\n\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.
What is missing is listing upload failures - which this script does know about...
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.
Specifically https://github.com/QubesOS/openqa-tests-qubesos/blob/main/utils/lib/openqa_api.py#L229 and https://github.com/QubesOS/openqa-tests-qubesos/blob/main/utils/lib/openqa_api.py#L622-L629 - failure in "system_tests" job should be reported as upload failure if there are no detailed results in that job.
utils/github_reporting.py
Outdated
|
||
if degradation: | ||
s += (f" :small_red_triangle: ( previous " | ||
green_circle = "🔻" |
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'm pretty sure this is not green circle ;)
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.
ahhhhhhhhhhhhhhhhhhhhhhh
Sometimes Github doesn't render the emojis, unicode delegates rendering to the browser.
c4ddd54
to
54ba5a1
Compare
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025082411-4.3&flavor=pull-requests Test run included the following:
Upload failures
New failuresCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025081011-4.3&flavor=update
Failed tests11 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/149225#dependencies 80 fixed
Performance TestsPerformance degradation:19 performance degradations
Remaining performance tests:155 tests
Would label it with: ['openqa-failed'] |
utils/github_reporting.py
Outdated
failed_tests_details += ' * ' + str(fail) + '\n' | ||
upload_failures.extend(results[k]) | ||
if not upload_failures: | ||
output_string += "No upload failures!\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.
I'd say simply skip the section if there are no upload failures.
utils/lib/openqa_api.py
Outdated
for test_group in json_data['job']['testresults']: | ||
if test_group['result'] == 'passed': | ||
continue | ||
failure_per_group[test_group['name']] = [] |
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.
Do you actually reference failures from different group in a loop iteration? If not, maybe a simple list (not a dict of lists) would be enough? Same for delayed list below.
But also see the next comment.
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.
Not needed indeed.
utils/lib/openqa_api.py
Outdated
elif failure.name == "system_tests": | ||
delayed_failure_per_group[failure.name].append(failure) | ||
|
||
if not failure_per_group[test_group['name']]: |
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.
Does this work at all? If upload failed, you won't see those other groups at all...
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.
See the example here, where it worked: #40 (comment)
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.
Did it? I don't see audio failures listed for example (https://openqa.qubes-os.org/tests/150378)
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 guess this is another issue but unrelated to this PR?
See the original comment: QubesOS/qubes-core-admin#718 (comment)
I don't find the audio failures there also.
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.
Wait, I am checking the build numbers....
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.
The error occurs because of the f in upload_failures
test...
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025082411-4.3&flavor=pull-requests Test run included the following:
Upload failures
New failuresCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025081011-4.3&flavor=update
Failed tests13 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/149225#dependencies 80 fixed
Performance TestsPerformance degradation:19 performance degradations
Remaining performance tests:155 tests
Would label it with: ['openqa-failed'] |
The result appears to be in line with the |
diff /tmp/report.old /tmp/report
70d69
< * system_tests_backup
103d101
< * system_tests_basic_vm_qrexec_gui_ext4 These were header without failures under them. |
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.
One minor thing remained and otherwise it's good.
utils/lib/openqa_api.py
Outdated
failure_per_group = {} | ||
delayed_failure_per_group = {} |
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.
Those two are not needed anymore
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'll merge it as is for now, as it improves several things, but there is a minor regression:
system_tests_pvgrub_salt_storage
system_tests: [Fail](https://openqa.qubes-os.org/tests/153694#step/system_tests/91) (unknown)
Tests qubes.tests.integ.storage failed (exit code 1), details repor...
system_tests: [Failed](https://openqa.qubes-os.org/tests/153694#step/system_tests/128) (test died)
# Test died: Some tests failed at qubesos/tests/system_tests.pm lin...
StorageReflinkOnBtrfs: [test_003_snapshot](https://openqa.qubes-os.org/tests/153694#step/StorageReflinkOnBtrfs/4) (error + timeout + cleanup)
qubes.exc.QubesVMShutdownTimeoutError: Domain shutdown timed out: '...
If detailed results are uploaded, the generic system_tests
points should be skipped.
Didn't test the warning message rendering.