Skip to content

Avoid pushing screenshots on non-main PRs - #14264

Merged
berland merged 1 commit into
equinor:mainfrom
berland:skip_screenshots_for_non_main
Aug 25, 2026
Merged

Avoid pushing screenshots on non-main PRs#14264
berland merged 1 commit into
equinor:mainfrom
berland:skip_screenshots_for_non_main

Conversation

@berland

@berland berland commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

ert-testdata is only able to track screenshots belonging to main of ert and merges to any other version branch must therefore be ignored.

Issue
Resolves #14229

Approach
Skip it

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

  • When screenshots are changed: Review screenshot-PR in ert-testdata,
    merge screenshot-PR in ert-testdata before merging this PR.
  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Add backport label to latest release (format: 'backport release-branch-name')

@berland berland added the release-notes:skip If there should be no mention of this in release notes label Aug 24, 2026
@berland berland self-assigned this Aug 24, 2026
@codecov-commenter

codecov-commenter commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.94%. Comparing base (162ccfe) to head (eaf8227).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14264   +/-   ##
=======================================
  Coverage   91.94%   91.94%           
=======================================
  Files         484      484           
  Lines       33688    33688           
=======================================
+ Hits        30973    30974    +1     
+ Misses       2715     2714    -1     
Flag Coverage Δ
cli-tests 36.52% <ø> (-0.01%) ⬇️
fuzz 44.37% <ø> (ø)
gui-tests 58.67% <ø> (-0.01%) ⬇️
performance-and-unit-tests 80.97% <ø> (+0.07%) ⬆️
test 45.97% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 2 files with indirect coverage changes

@codspeed-hq

codspeed-hq Bot commented Aug 24, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 36 untouched benchmarks


Comparing berland:skip_screenshots_for_non_main (eaf8227) with main (162ccfe)

Open in CodSpeed

Comment thread .github/workflows/push_screenshots.yml Outdated

- name: Get ert PR title
if: steps.metadata.outputs.outcome == 'failure' && steps.screenshots.outputs.has_new_screenshots == 'true' && steps.metadata.outputs.pr_number != '0'
if: steps.metadata.outputs.outcome == 'failure' && steps.screenshots.outputs.has_new_screenshots == 'true' && steps.metadata.outputs.pr_number != '0' && steps.target-branch.outputs.base_ref == 'main'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for running this differs from the other changes. That might be intentional to allow printing, but none of the variables are picked up in other steps not guarded differently. 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you run this manually, with 0 as the input?

@berland

berland commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

I will rewrite this PR to not even test screenshots on merge to non-main, testing screenshots can give false negatives.

The screenshots stored in ert-testdata only targets the
main branch of ert. Thus it is meaningless to run this test
workflow on other than merges to main, it will likely produce
false negatives.
@berland
berland force-pushed the skip_screenshots_for_non_main branch from 14221fb to eaf8227 Compare August 25, 2026 07:50

@andreas-el andreas-el left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@berland
berland merged commit 4ff378d into equinor:main Aug 25, 2026
35 checks passed
@berland
berland deleted the skip_screenshots_for_non_main branch August 28, 2026 09:44
@scout-team-app

scout-team-app Bot commented Sep 4, 2026

Copy link
Copy Markdown

Successfully created backport PR for version-25.0:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport version-25.0 release-notes:skip If there should be no mention of this in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Screenshot tests must be skipped on backports

3 participants