lodestar icon indicating copy to clipboard operation
lodestar copied to clipboard

Conditional status checks for PRs which do not require testing

Open philknows opened this issue 10 months ago • 5 comments

Problem description

Currently, all PRs will go through all status checks. Whether it is an actual feature code change or just a documentation change, we should have a way to signal skipping checks for PRs that don't need to go through the entire testing process. Document change PRs don't really need to run unit/spec/sim tests.

Solution description

We should create a label such as status-skip-tests we can use to signal the CI to skip certain status checks if tagged with this label.

Additional context

No response

philknows avatar Apr 18 '24 16:04 philknows

Maybe we can do it based on modified files, e.g. if only edits in docs/** then just run linting, spellcheck, .. and no test suites

nflaig avatar Apr 18 '24 17:04 nflaig

Is there anything else we foresee not requiring status check tests to run?

philknows avatar Apr 19 '24 18:04 philknows

Is there anything else we foresee not requiring status check tests to run?

Not requiring any status check tests you mean? Probably some typo fixing in code comments. Even a small change in markdown file is covered by docs-check.

I personally would love to see spec-tests being skippable for unrelated PRs

ensi321 avatar Apr 22 '24 04:04 ensi321

I personally would love to see spec-tests being skippable for unrelated PRs

Yeah me too. There are some PRs that shouldn't need to go through these when they obviously don't modify for the functionality of Lodestar in any way.

Currently, for unstable merge, the tests that must pass are:

  • Spec tests (20)
  • Unit Tests (20)
  • Lint (20) (Note, it shouldn't even be this minimal, but due to ongoing work for other tests, this is currently the setting to bypass inconsistencies)

All tests for PRs:

  • Benchmark / run (pull_request)
  • CodeQL / Analyze (javascript) (pull_request)
  • Check docs / Docs spellcheck (pull_request)
  • Lint PR title / Validate PR title (pull_request_target)
  • Sim merge execution/builder tests / Sim merge tests (pull_request) S
  • Sim tests / Build (pull_request)
  • Tests / Build (20) (pull_request)
  • Tests / Lint (20) (pull_request)
  • Sim tests / Multifork sim test (pull_request)
  • Sim tests / Endpoint sim tests (pull_request)
  • Tests / Type Checks (20) (pull_request)
  • Sim tests / Deneb sim tests (pull_request)
  • Tests / E2E Tests (20) (pull_request)
  • Sim tests / Eth backup provider sim tests
  • Tests / Browser Tests (20) (pull_request)
  • Sim tests / Mixed clients sim tests (pull_request)
  • Tests / Spec tests (20) (pull_request)
  • Tests / Unit Tests (20) (pull_request)
  • license/cla

For things that don't require extensive testing (docs changes, some repo maintenance chores, etc.), we should just have it run:

  • Check docs / Docs spellcheck (pull_request)
  • Lint PR title / Validate PR title (pull_request_target)
  • license/cla

Maybe we can do it based on modified files, e.g. if only edits in docs/** then just run linting, spellcheck, .. and no test suites

There are some things that don't necessarily touch only docs/**. For example, some website maintenance to our docs are labeled as chore: and perhaps those could be excluded too? This is why I thought of the label method as a conditional for the CI to skip certain tests and can be appended by the PR author based on their judgement and the reviewers.

philknows avatar Apr 23 '24 02:04 philknows

Here is a tentative strategy to implement this:

  • some jobs are filtered based on the presence of a label on the PR (whole workflows cannot be filtered)
  • this filtering doesn't prevent required workflows to pass
  • label can be removed/added during the lifetime of a PR; jobs should be triggered accordingly
  • in a second step, labels could be added automatically based on some conditions (e.g. docs only PR could get those labels applied)

This proposed solution allows to merge even if some jobs are skipped, as long as the required workflows pass.

Some of the jobs that could be skippable:

  • all of test workflow, via ci-skip-test
  • all of benchmark workflow, via ci-skip-benchmark
  • all of sim workflow, via ci-skip-sim
  • all of sim-merge workflow, via ci-skip-sim-merge
  • a meta label to skip all of the above: ci-skip-all-tests

Jobs can be filtered by label using this syntax:

jobs:
  build:
    if: contains(github.event.pull_request.labels.*.name, '<label_name>')

Workflows can be re-triggered when label are updated:

on:
  pull_request:
    types: [ opened, synchronize, reopened, labeled, unlabeled ]
    # defaults to "opened", "synchronize" and "reopened"

Some actions that could be useful for auto-labeling:

  • https://github.com/tj-actions/changed-files
  • https://github.com/actions/labeler

jeluard avatar May 07 '24 10:05 jeluard