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

Conditional status checks for PRs which do not require testing #6686

Open
philknows opened this issue Apr 18, 2024 · 5 comments
Open

Conditional status checks for PRs which do not require testing #6686

philknows opened this issue Apr 18, 2024 · 5 comments
Labels
meta-feature-request Issues to track feature requests. scope-testing Issues for adding test coverage, fixing existing tests or testing strategies.

Comments

@philknows
Copy link
Member

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 philknows added meta-feature-request Issues to track feature requests. scope-testing Issues for adding test coverage, fixing existing tests or testing strategies. labels Apr 18, 2024
@nflaig
Copy link
Member

nflaig commented Apr 18, 2024

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

@philknows
Copy link
Member Author

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

@ensi321
Copy link
Contributor

ensi321 commented Apr 22, 2024

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

@philknows
Copy link
Member Author

philknows commented Apr 23, 2024

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.

@jeluard
Copy link
Member

jeluard commented May 7, 2024

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-feature-request Issues to track feature requests. scope-testing Issues for adding test coverage, fixing existing tests or testing strategies.
Projects
None yet
Development

No branches or pull requests

4 participants