-
Notifications
You must be signed in to change notification settings - Fork 592
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
chore(ci): Publish unit-test results for the bazel-workflow #14145
chore(ci): Publish unit-test results for the bazel-workflow #14145
Conversation
Thanks for opening a PR! 💯
Howto
More infoPlease take a moment to read through the Magma project's
If this is your first Magma PR, also consider reading
|
1486f62
to
32fcd9c
Compare
num_all_disabled += int(test_suites_data.attrib['disabled']) | ||
init_time = min(init_time, datetime.fromisoformat(test_suites_data.attrib['timestamp'])) | ||
init_time = datetime.fromisoformat(test_suites_data.attrib.get('timestamp', str(init_time))) |
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 old logic finds the lowest init time of all xml files, while the new logic uses the one from the last xml file in the loop. I guess this was adapted because at one point it encountered a KeyError
when there was no timestamp? In that case we could just
try:
init_time = ...
except KeyError:
pass
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.
Your are right, but wouldn't this be better?:
init_time = min(init_time, datetime.fromisoformat(test_suites_data.attrib.get('timestamp', str(init_time))))
@@ -46,17 +46,24 @@ def merge_all_report(working_dir, list_xml_report_paths, output_path): | |||
xml_file_path = working_dir + "/" + xml_file_path | |||
test_result = ET.parse(xml_file_path) | |||
test_suites_data = test_result.getroot() | |||
# for the integration tests pytest generates the XML files with a slightly different structure |
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.
Was the old name wrong?
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 introduced this conditional when I was working on publishing integration tests results, but now that unit tests require the same conditional I think it is reasonable to rename it.
32fcd9c
to
e42cd0e
Compare
Signed-off-by: Krisztián Varga <krisztian.varga@tngtech.com>
e42cd0e
to
1a21054
Compare
Signed-off-by: Krisztián Varga krisztian.varga@tngtech.com
Summary
The the unit-test results were not published before for the
Bazel build and test
workflow. This PR ensures that the XML files are archived and published for all matrix jobs. This closes #13526.Test Plan
CI.
Test run with failing tests: https://github.com/vktng/magma/actions/runs/3235921237
Test run without failing tests: https://github.com/vktng/magma/actions/runs/3236092774
Additional Information