-
Notifications
You must be signed in to change notification settings - Fork 9
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
add collapse in junit as a table, rename metrics in readout #50
add collapse in junit as a table, rename metrics in readout #50
Conversation
Signed-off-by: Shashank Reddy Boyapally <sboyapal@redhat.com>
Signed-off-by: Shashank Reddy Boyapally <sboyapal@redhat.com>
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.
amazing work @shashank-boyapally
pkg/utils.py
Outdated
testsuite.set("failures", str(failures_count)) | ||
testsuite.set("tests", str(test_count)) | ||
xml_str = ET.tostring(testsuites, encoding="utf8", method="xml").decode() | ||
dom = xml.dom.minidom.parseString(xml_str) | ||
pretty_xml_as_string = dom.toprettyxml() | ||
return pretty_xml_as_string | ||
|
||
def generate_tabular_output(data: dict, metric_name: str) -> str: |
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 we should just present in the XML a csv -- uuid, buildURL, metric, value, change-point[true|false] not sure how the XML file will read in the table view.
testcase = ET.SubElement(testsuite, "testcase", name=f"{label_string} {metric} regression detection", timestamp=str(int(datetime.now().timestamp()))) | ||
if [run for run in data_json if not run["metrics"][metric]["percentage_change"] == 0]: | ||
failures_count +=1 | ||
failure = ET.SubElement(testcase,"failure") |
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 wonder if we can reduce the output here, to be where the change-point was seen, with 1 value after and 1 before. wdyt? would cleanup the output quite a bit.
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.
Yeah, I felt the same that the output was too verbose, we can just have one before and after.
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.
yeah, quite honestly, with how we plan to run this in CPT I doubt it will be as verbose. but, just to be careful ;)
made shorter.
|
Signed-off-by: Shashank Reddy Boyapally <sboyapal@redhat.com>
Ok, I like this! Lets ask the TRT team about the format, i wonder if they will want something more like : Might be super nitpicky, but lets ask! Nice work!! |
Nice changes @shashank-boyapally . Can you keep the visuals in description updated as well? |
Signed-off-by: Shashank Reddy Boyapally <sboyapal@redhat.com>
I have made some changes on how the table should render based on the comments provided by the trt team and updated 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.
lgtm, nice work @shashank-boyapally
Type of change
Description
The trt team requested to have the metric test in a single testcase and I renamed the metrics in the readout config. Please ignore linting.
Related Tickets & Documents
Checklist before requesting a review
Testing