-
Notifications
You must be signed in to change notification settings - Fork 8
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
Compares current with previous test-status.json #196
Conversation
2befe48
to
8699168
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
a024810
to
9ae929f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1a98755
to
f80f463
Compare
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.
Thank you for working on this! I left some remarks
tools/generate_report.py
Outdated
with open(REPORT_PATH, 'r') as f: | ||
REPORT = json.load(f) |
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 add a function like this to utils.py
, as we have similar code in soooo many places
def read_json(path):
with open(path, 'rb') as f:
return json.load(f)
Note that I used 'rb
', to read as binary, with the hope that we can ignore encodings this way; but it may not work... Of course in the four places we call json.load
right now, we do...
open(fname, 'r')
open(fname, "rb")
open(fname, "r", encoding="utf-8")
open(fname, 'r', encoding='utf-8', errors='ignore')
So actually unifying this may be difficult sigh. But at least within this file, we could do it...
tools/generate_report.py
Outdated
with open(DIR_REPORT+'/test-status-diff.json', 'w') as f: | ||
json.dump(REPORT_DIFF, f, ensure_ascii=False, indent=2) |
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.
... aaaand also a write_json
helper might be useful as well
2d3dd60
to
ada7dbf
Compare
There is no such thing as an "empty" branch, but I've created dummy |
ada7dbf
to
0887f8c
Compare
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.
It is not quite clear to me what needs to be done / what the status of this is. Perhaps we can discuss it a bit tomorrow?
8077caa
to
01c55bf
Compare
@fingolfin, also after you merged some new package updates, the Edit: Sometimes it seems to randomly work, and sometimes it fails. Really weird. Edit 2: It seems, that sometimes the first run on a commit fails. The next workflow that runs on the same commit succeeds after that. Edit 3: The problem seems to have disappeared |
3a3eb0d
to
d348402
Compare
@fingolfin, could you configure our GAP bot somehow, so that it has a picture and we could click on its name in the commit history? Currently this is not possible, but it was possible with the Github Actions Bot. Edit: For the pull-request this seems to work, but once you merge, the name seems to change? At least in the commit history the name (Gap Package Distribution Bot) is not clickable, but in the pull-request tab the name (gap-package-distribution-bot) is clickable. Edit 2: Yeah, this is quite weird. The author/committer in our actions is maybe set wrong. For example in this pull-request you can click on our bot. But if you click on the first force-push commit, you cannot click on the author (which should be our bot, something is fishy here). Edit 3: Ok, the commiter is definitely set wrong. I maybe have found a solution here Edit 4: Going here shows the id of our bot Edit 5: Fixed wrong commiter for our bot in all actions. |
cca4adc
to
7f5d770
Compare
@fingolfin, the pull-request is finished. For now, I scheduled the daily tests for 2 am (I hope). I am not sure if splitting the daily tests into different YML workflows for different GAP versions (like Wilf did in gap-infra/integration) is somehow more efficient than calling workflows in parallel as I did now. Here is a dynamic dashboard that we could use in the README.md later:
Here is the report for Package Evaluation ReportJob PropertiesTesting: master/2022-03-30-19:10:47-84bdcfd8 vs master/2022-03-30-18:22:10-3ff817cf Generated by Workflow: https://github.com/FriedrichRober/PackageDistro/actions/runs/2066703509 In total, 152 packages were tested, out of which 114 succeeded, 17 failed and 21 were skipped. ✔️ ✔️ Packages now succeeding1 package(s) succeeded tests only on the current version. Click to show package(s)!
❗ Packages still failing17 package(s) failed tests also on the previous version. Click to show package(s)!
✔️ Packages still succeeding113 package(s) succeeded tests also on the previous version. Click to show package(s)!
➖ Packages that still skipped21 package(s) skipped tests also on the previous version. Click to show package(s)!
|
Awesome! Let's see how it fares :-) |
High-level status dashboard
Transfer this into the README.md after this pull-request is merged.
Replace
FriedrichRober
withgap-system
.Python Scripts
tools/generate-report.py
creates areport.md
that compares the test results with the previous run.It also generates a
test-status-diff.json
which we could use for writing the comment as suggested in Automatically add (resp. update) PR comments with test results #194.We could also directly use
report.md
as the comment. We could usetest-status-diff.json
in order to figure out, if we should write notifacations to slack or if we can automerge pull-request.tools/update-latest-report.py
updates thelatest
report symlinkand the corresponding html-redirects and badges.