Skip to content

Tolerate gated screenshot test checks#2822

Draft
ittaigolde wants to merge 2 commits into
jMonkeyEngine:masterfrom
ittaigolde:tolerate-gated-screenshot-checks
Draft

Tolerate gated screenshot test checks#2822
ittaigolde wants to merge 2 commits into
jMonkeyEngine:masterfrom
ittaigolde:tolerate-gated-screenshot-checks

Conversation

@ittaigolde
Copy link
Copy Markdown
Contributor

Summary

  • keep the screenshot comment workflow from failing when the screenshot check was never created
  • report the missing check as a notice, which can happen when the pull request build workflow is waiting for maintainer approval

Reasoning

Fork pull requests can leave the Build jMonkeyEngine workflow in action_required before any jobs are created. In that case the Run Screenshot Tests check never exists, but the pull_request_target comment workflow still runs and currently fails while waiting for that check.

This keeps real screenshot failures working the same way, while avoiding a misleading failed check when there was no screenshot test run to inspect.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

@riccardobl
Copy link
Copy Markdown
Member

riccardobl commented May 28, 2026

I think this will make the screenshot test fail silently if it breaks for whatever reason, what do you think @richardTingle ?

Maybe we need to check for an outcome or something to make sure it is failing due to missing approval.

@richardTingle
Copy link
Copy Markdown
Member

Not posting the comment isn't a huge deal. The pipeline would still fail, it just wouldn't produce a comment explaining why.

But if this is going to happen regularly now pipelines require approval it might be necessary to revisit the auto comment (it may just not be possible anymore).

Really the comment was important when the tests were new to explain them, now they're embedded it isn't quite so essential

@riccardobl
Copy link
Copy Markdown
Member

Not posting the comment isn't a huge deal. The pipeline would still fail, it just wouldn't produce a comment explaining why.

But if this is going to happen regularly now pipelines require approval it might be necessary to revisit the auto comment (it may just not be possible anymore).

Really the comment was important when the tests were new to explain them, now they're embedded it isn't quite so essential

makes sense, i think it would be enough to generate an annotation with the same text using https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#setting-an-error-message

@riccardobl riccardobl marked this pull request as draft May 28, 2026 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants