-
Notifications
You must be signed in to change notification settings - Fork 38
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
Comments are only sent when GNU/BFS test compatibility changes. #407
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #407 +/- ##
=======================================
Coverage 65.31% 65.31%
=======================================
Files 34 34
Lines 3941 3941
Branches 903 903
=======================================
Hits 2574 2574
Misses 1001 1001
Partials 366 366 ☔ View full report in Codecov by Sentry. |
.github/workflows/comment.yml
Outdated
@@ -58,7 +58,14 @@ jobs: | |||
.map(annotation => `${annotation.run}: ${annotation.annotation.message}`) | |||
.join('\n'); | |||
|
|||
console.log(annotationContent); | |||
// check if no changes | |||
let gnuTestReport = annotationContent.includes('Run GNU findutils tests: Changes from main: PASS +0 / FAIL +0 / ERROR +0 / SKIP +0'); |
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.
this seems a bit fragile. In the json, we don't have the number to do some if > 0 ?
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.
That might mean getting the results for the master branch in comment.yml and comparing them to bfs-result.json and gnu-result.json? I think this might be a bit repetitive and complicated.
How about changing compare_bfs_result.py to send a Github Actions Annotations using print(f"::warning :: BFS tests no change.")
and catch annotation in comment.yml
when compatibility is unchanged?
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, probably better and easier
GNU testsuite comparison:
|
GNU testsuite comparison:
|
GNU testsuite comparison:
|
i guess it isn't ready yet, right ? :) |
Yeah, I haven't tested this code on my fork yet. |
@sylvestre done. |
GNU testsuite comparison:
|
sorry, looks like i wasn't cleary. What I would like is to have nothing to display when we should only display something when we noticed changes in bfs or gnu tests |
No, actually it does not send comments when it contains |
ok, let's see :) |
Tested in fork, this change may still only take effect after being merged.