Ensure "complete" CI steps always run

[marcotc]

This PR ensures the last step of our CI workflows (normally called WORKFLOW NAME (complete)) always run, even when its dependency jobs have failed or been skipped.

This ensures that our mandatory branch protection checks will never be skipped by GitHub. Today when the "complete" job is skipped, the branch protection check considers it successful ‽

Change log entry
No

Additional Notes:

How to test the change?

[p-datadog]

LGTM but I don't understand the GH stuff enough to checkmark

[pr-commenter]

Benchmarks

Benchmark execution time: 2025-03-18 08:32:12

Comparing candidate commit 4db2cc1 in PR branch fix-checks with baseline commit 874e602 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 31 metrics, 2 unstable metrics.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.69%. Comparing base (874e602) to head (4db2cc1).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4506      +/-   ##
==========================================
- Coverage   97.70%   97.69%   -0.02%     
==========================================
  Files        1381     1381              
  Lines       83973    83973              
  Branches     4251     4251              
==========================================
- Hits        82046    82036      -10     
- Misses       1927     1937      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[datadog-datadog-prod-us1]

Datadog Report

Branch report: fix-checks
Commit report: 4db2cc1
Test service: dd-trace-rb

✅ 0 Failed, 20632 Passed, 1370 Skipped, 3m 17.78s Total Time

[lloeki]

Today when the "complete" job is skipped, the branch protection check considers it successful ‽

This is normal (and similar to rspec skipping tests?)

In GHA terms, "skipped" means "this step/workflow is not relevant".

e.g running system tests for doc changes makes no sense => skipped, ergo "successful".

This PR would transform a "skipped" UI display materialising the fact that the tests did not run (intentionally so) into a checkmark that looks like as if the tests did run when they actually didn't.

(Again, a skipped test is displayed differently than a passing one in a rspec suite)

even when its dependency jobs have failed

Also "(complete)" should not run when any dependent fail and thus properly result in the PR tests not passing / the PR being unmergeable. Is that the initial reason for this PR? That you'd like it to be red?

Marking this "request changes" to leave the opportunity to discuss, but I don't believe the change should be merged.

See:

• #4408
• #4418

[p-datadog]

@lloeki My reading of the diff is that a skipped status would be changed to failing, not successful, thus there will not actually be a case when a skipped test would be marked as having been run and succeeded?

[lloeki]

a skipped status would be changed to failing,

That would be a problem! It's like you have this rspec test:

it 'tests something' do
  skip "don't run this test in this case" if some_condition?
  
  # something only conditionally relevant...
end

You don't want that to fail the test suite, you want the test suite to pass (and tell you that bit has been skipped)

If you have a doubt, imagine that some_condition? is jruby? and the test case is something that is not relevant to JRuby.

In our case the system tests have no business running when there are only changes to, say, documentation files, ergo the running of it is skipped based on a path criteria, because they're simply irrelevant to run in that case, and so the overall result of the skip is to satisfy the branch protection status requirement check.

If "skip" implied "fail" then merges would be blocked by irrelevant checks.

[p-datadog]

@lloeki According to PR description only the "summary" jobs would be changed to be always required, not e.g. the system tests ones. @marcotc please correct me if I misunderstood.

[lloeki]

only the "summary" jobs would be changed to be always required

They are already required. The desc says that they are made to always run and never skipped:

This ensures that our mandatory branch protection checks will never be skipped by GitHub. Today when the "complete" job is skipped, the branch protection check considers it successful ‽

When system tests should run they must run.

Yet there are cases where they are useless to run: doc change, code change with no impact on system tests (e.g spec change, appraisal...), etc... Running them would just delay PR merging and waste workers.

That's the point of the path-based filter:

• When systests are relevant to run they are required to run and pass.
• When systests don't need to run they are irrelevant, hence skip.

A skip status on a "completed" synthetic job satisfies the required status check for merging, but most importantly a skip on this "completed" job is visually materialised to indicate it is expected to be skipped.

The PR comment says:
Today when the "complete" job is skipped, the branch protection check considers it successful ‽
And aims to address that by making it a green checkmark instead, when having it be skipped is the correct and intended behaviour. "The branch protection check consider[ing] it successful" is exactly what is desired: the result of system tests simply does not matter.

Again if you had a spec like:

it 'forks' do
  skip 'no fork in jruby' if RUBY_ENGINE == 'jruby'

  # ... whatever
end

And ran it under JRuby, you wouldn't mark this spec example as "green", "completed", or whatever: it would be a) tallied as "skipped" and b) ignored as far as the whole spec suite result is concerned (and so logically equivalent to "success" WRT that overall test suite result)

So I am wondering why do we want them green when they did not run? Maybe I missed something?

[lloeki]

X-linking this one which is somewhat related: #4629 (comment)

[p-datadog]

I think it's fine to either always run "complete" jobs or present them as "successful" when all of their dependencies are skipped (but none failed).

Do you have another suggestion for how to avoid the scenario of a PR having failed required jobs that don't prevent merging (via an intermediate job that is skipped)? Changing the ~5 "compete" jobs from "skipped" to "successful", seems like a very reasonable compromise if there aren't any better suggestions.

[TonyCTHsu]

Obsolete with #4629