Skip to content
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 github workflow to upload test failure markdown summary report as a comment to PRs when tests fail #54906

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

nyalldawson
Copy link
Collaborator

Relates to qgis/QGIS-Enhancement-Proposals#268

This PR expands the current mechanism which generates HTML summary reports of test failures to also generate a (concise!) markdown summary report. The markdown report is then added as a comment to the PR, providing a much more submitter-friendly way of determining why a PR is failing the test suites.

A few notes:

  • In Improve test result handling on QGIS CI QGIS-Enhancement-Proposals#268 I wrote: "The goal is that if a rendering test fails, a comment will be automatically added to the Pull Request containing a descriptive message and a link to download the rendered images for debugging". This is unfortunately still NOT possible given the available Github APIs.
  • Specifically, I was originally hoping to attach the full test summary artifact (html report + rendered images) as a zip file to the autogenerated comment. However, there's no public API to upload assets to Github. (I did find https://github.com/j178/github-s3, but that relies on a private/non-stable API, and I'm dead against relying on private API in our CI setup... there's enough regular ongoing maintainence required here without intentionally heading into unstable territory 🙀 )
  • For the same reason, I can't directly embed the rendered images into the auto-created comment -- there's no way we can grab the images from the test report artifact and upload them as issue assets. => The test failure comment is PLAIN TEXT ONLY 😞
  • Failing this, I was hoping to at least put a link in the comment to the test results artifact zip, to make it easy to locate this artifact. But again, there's no Github support for this 🙀 🙀 . (See Support artifact URL output actions/upload-artifact#50 , List artifacts doesn't work during workflow run actions/upload-artifact#53 )
  • The comment only relates to test failures relating to rendered image checks -- other general test failures are excluded. The rendered image check is our most unusual/trickiest barrier to new contributors wrt failing tests, so I'm exclusively targeting that here.
  • Tests MUST use the base class QgsTest / QgisTestCase methods for render/layout rendering checks. If they have ad-hoc implementations then the markdown report won't include the failures from those ad-hoc methods. Updating all tests to use the base class methods is a WIP, being done on demand (see eg 117f508 )
  • In the QEP I've mentioned: "If possible, the workflow improvements will be backported to apply to all maintained stable release branches (including LTR).". This ISN'T suitable for backporting... there's been many many changes to the test infrastructure since LTR and the delta is just too large to make a backport workable.

Also note: this workflow won't actually DO anything from this PR -- it will only kick in when the workflow is present in the master branch.

@nyalldawson nyalldawson added the Chore GitHub and other CI infrastructure changes label Oct 11, 2023
@github-actions github-actions bot added this to the 3.34.0 milestone Oct 11, 2023
@nyalldawson
Copy link
Collaborator Author

Ping @3nids

@3nids
Copy link
Member

3nids commented Oct 11, 2023

Not sure this would work but what about using a dedicated repo and Gihtub pages?

  • from the testing workflow, you fork the gh-pages repo, add your test data in a folder named date-PR-commit, you can comment with the link
  • on the gh-pages repo:
    • an auto-merge workflow which checks the structure of the PR (no file deletion, repsect the folder structure, only html + images, etc.)
    • a cron workflow which automatically deletes the content older than a week or 2

@nyalldawson
Copy link
Collaborator Author

@3nids

Not sure this would work but what about using a dedicated repo and Gihtub pages?

Novel approach! My reservation would be that it's adding considerable complexity and the accompanying maintenance burden to our already complex CI setup.

My gut feeling is that we should stick within the the more well-trodden, "supported" realm of github workflows. I think there's a good chance that the missing functionality we need is added to github api in future (there's certainly a LOT of demand for it on the various tickets), and at that stage it will be relatively simple to safely upgrade the workflow to handle artifact links/embedded images.

@nyalldawson nyalldawson merged commit 872901a into qgis:master Oct 11, 2023
@nyalldawson nyalldawson deleted the pr_test_report branch October 11, 2023 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chore GitHub and other CI infrastructure changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants