-
Notifications
You must be signed in to change notification settings - Fork 16
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 fork repositories support for reporting regression tests #508
Conversation
.github/workflows/tests_results.yml
Outdated
@@ -0,0 +1,44 @@ | |||
name: Tests results |
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.
Fix grammar. It is customary to use a singular form ("test results").
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.
Done
.github/workflows/main.yml
Outdated
# - name: Test Report | ||
# uses: EnricoMi/publish-unit-test-result-action@v2 | ||
# with: | ||
# files: test/regression/cocotb/results.xml | ||
# check_name: cocotb test results | ||
# comment_mode: off | ||
|
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.
Remove commented out lines.
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.
Done
.github/workflows/tests_results.yml
Outdated
- name: Download and Extract Artifacts | ||
uses: dawidd6/action-download-artifact@246dbf436b23d7c49e21a7ab8204ca9ecd1fe615 | ||
with: | ||
run_id: ${{ github.event.workflow_run.id }} | ||
path: artifacts |
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 should preferably only download only the needed artifacts.
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.
What is the idea behind these changes? CI
jobs are run on fork and Test result
is run on trunk? Or I am wrong?
.github/workflows/tests_results.yml
Outdated
|
||
steps: | ||
- name: Download and Extract Artifacts | ||
uses: dawidd6/action-download-artifact@246dbf436b23d7c49e21a7ab8204ca9ecd1fe615 |
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.
Why not actions/download-artifact
?
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.
Don't know the source of problem with that action, but according to dawidd6/action-download-artifact, official actions/download-artifact does not allow to download an artifact uploaded in other workflow.
.github/workflows/main.yml
Outdated
uses: actions/upload-artifact@v3 | ||
with: | ||
name: Event File | ||
path: ${{ github.event_path }} |
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.
Why you upload the whole directory instead of event.json
?
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.
Why are you claiming that this is directory? ${{ github.event_path }} is pointing to event.json file.
.github/workflows/main.yml
Outdated
check_name: cocotb test results | ||
comment_mode: off | ||
name: Tests results | ||
path: test/regression/cocotb/results.xml |
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.
Maybe you can upload event.json
here to don't create separate step? This will reduce resource usage.
@lekcyjna123: the simple answer to your question is that everything was copied from the document linked in the description, and that document doesn't give any explanations. |
.github/workflows/tests_results.yml
Outdated
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 action is not valid and fails on your fork because of some yaml syntax error: https://github.com/makz00/coreblocks/actions/runs/6825022147
Suggestion: remove the "cocotb test results" thing, parse the results file in "Run regression tests" and fail the workflow if there are any failed test cases there. |
Suggestion: do the failure check in |
In the interest of having this issue solved, I'm merging this without reviews, leaving the suggested change for later. |
Issue: #492
The solution proposed in: support-fork-repositories-and-dependabot-branches has been applied. It is based on dedicated workflow that is only responsible for creating tests report. The report input data are saved as artifact by CI workflow.