Refactor veristat-compare.py script #237
Conversation
yurinnick
commented
Oct 4, 2023
•
yurinnick
commented
- Use csv.DictReader for veristat results parsing
- Better column name handling with Enum class
- Fix the issue when comparison reports 100% change when both old and new values are zeros
yurinnick
force-pushed
the
veristat-compare-refactor
branch
from
a429ba9 to
73d63b2
Compare
There was a problem hiding this comment.
Skimmed over and looks fine.
It would be useful to capture some fixtures and run tests against those to make sure we don't break the code in the future as it is hard to really validate without going through a full cycle.
The veristat summary of this diff is:
# No /tmp/work/vmtest/vmtest/x86_64-gcc-baseline-veristat-kernel available
# No /tmp/work/vmtest/vmtest/x86_64-gcc-baseline-veristat-meta available
is this expected?
.github/scripts/veristat-compare.py
Outdated
| def get_results_title(self) -> str: | ||
| if self.new_failures: | ||
| return "There are new veristat failures" | ||
|
|
||
| if self.changes: | ||
| return "There are changes in verification performance" | ||
|
|
||
| return "No changes in verification performance" | ||
|
|
||
| def get_results_summary(self, html: bool = False) -> str: | ||
| title = self.get_results_title() | ||
| if not self.table: | ||
| return f"# {title}\n" |
There was a problem hiding this comment.
as a follow-up, it would be good to report failure/warning with color codes so it is made more obvious in GH.
There was a problem hiding this comment.
Github markup doesn't support colors: github/markup#1440
There was a problem hiding this comment.
The text content can be "colorized" by mean of special tags, see https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-a-warning-message
The markup one can use simple to signal success/failure: https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/
There was a problem hiding this comment.
It's not exactly "colors", but I got what you mean. I will cover it in a separate PR.
.github/scripts/veristat-compare.py
Outdated
| verdict = f"{verdict_old} -> {verdict_new}" | ||
| if verdict_new == "failure": | ||
| new_failures = True | ||
| verdict += " (!!)" |
There was a problem hiding this comment.
I feel this (!!) should be a constant.
Yes, since we don't store any baseline metrics for in this repository`s cache, we don't have anything to compare to |
- Use csv.DictReader for veristat results parsing - Better column name handling with Enum class - Fix the issue when comparison reports 100% change when both old and new values are zeros Signed-off-by: Nikolay Yurin <[email protected]>
yurinnick
force-pushed
the
veristat-compare-refactor
branch
from
73d63b2 to
ac22b32
Compare
ok, I guess there is still value to run it in vmtest even without the baseline. So, let's leave it like this. Did you test this in kernel-patches/bpf? Could you make a PR where vmtest is modified to reflect the changes you are making here so we can have a proper e2e test? |
|
Created test PR in |
