feat(ci): test-impact analysis E2E (default-on) + release E2E workflow - #2559
Conversation
Three changes that compose into a single E2E policy:
1. **Test-impact analysis selector** (`hack/select-e2e.sh`).
Reads `packages/core/platform/sources/*.yaml` (the PackageSource
dependency graph also rendered by `cozypkg dot`) and emits the bats
files affected by a PR diff. Library/core/networking/CI changes,
shared `hack/*.sh` helpers, the install bats, and the
`pull-requests` / `release-e2e` workflows trigger the full suite.
Doc/dashboard/`*.md` diffs select nothing. Per-app bats edits
select only that app. Anything inside `packages/` with no graph
entry, or a system source whose graph has no `*-application`
descendants, falls back to the full suite (conservative).
2. **Default-on TIA in `pull-requests.yaml`** with `full-e2e`
opt-out. Drops the previous `e2e-tia` opt-in shape — the
selector's behaviour has been validated, so making it the default
gives PR runtime savings out of the box. PRs that need the full
suite (e.g. cross-cutting refactors that the selector can't
reason about) carry the `full-e2e` label and get the unchanged
pre-TIA flow. Docs-only PRs (selector emits nothing) skip the
`Run E2E tests` step entirely.
3. **`release-e2e.yaml`** runs the full suite on every tag push
(`v*`). Independent of `tags.yaml`'s release publish flow — a
failure here is an alarm, not a release blocker. Closes the
coverage gap left by making PR runs use TIA: the full suite is
now exercised against shipped code at tag-cut time.
Bug fixes vs. the original `feat/select-e2e-script` draft:
- `dashboard/` -> `dashboards/` (path is plural in this repo).
- `hack/e2e-apps/<name>.sh` and `hack/<name>.bats` were silently
ignored; they now trigger the full suite (test infrastructure
affecting all apps).
- A path matching the graph but yielding zero `*-application`
descendants now falls back to the full suite instead of silently
emitting nothing (system-only changes are no longer silently
skipped).
- Per-app bats edits are matched before the full-suite pattern, so
editing one bats file no longer escalates to the full suite.
- Removed the dead `skip_pattern` variable (PCRE syntax that
`grep -qE` doesn't support; it was never matched).
Tests: `hack/select-e2e_test.bats` covers all 11 cases (the original
6 plus 5 for the bug fixes above).
Workflow inputs are passed via env vars rather than direct `${{ }}`
expansion into shell commands, per workflow injection guidance.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Assisted-By: Claude <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes the CI pipeline by implementing intelligent test-impact analysis. By analyzing the dependency graph of the project, the system now selectively runs only the necessary E2E tests for a given set of changes, significantly reducing CI time. Additionally, it establishes a robust release-time testing gate to ensure full suite validation on all tagged releases, balancing development velocity with production stability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughAdds a Test Impact Analysis step to PR E2E that diffs against the base ref and selects affected bats apps via a new ChangesSmart E2E Test Selection
Release E2E Workflow
Sequence DiagramssequenceDiagram
actor Developer
participant GitHub as "GitHub Actions (PR)"
participant GitCmd as "git diff"
participant Selector as "hack/select-e2e.sh"
participant YQ as "yq (sources index)"
participant E2E as "E2E Test Suite (bats)"
Developer->>GitHub: Push PR
GitHub->>GitCmd: Compute diff vs origin/BASE_REF
GitCmd-->>Selector: List of changed files
Selector->>YQ: Build owners & reverse-deps indexes
YQ-->>Selector: Indexes
Selector->>Selector: Resolve files → PackageSources → transitive deps → map to bats
Selector-->>GitHub: outputs `apps=<list>` or `skip=true`
alt skip == 'true'
GitHub->>GitHub: Skip E2E
else
alt full-e2e label present
GitHub->>E2E: Run all `hack/e2e-apps/*.bats`
else
GitHub->>E2E: Run selected bats files (env SELECTED_APPS)
end
E2E-->>GitHub: Test results
end
GitHub-->>Developer: Workflow result
sequenceDiagram
actor Maintainer
participant GitHub as "GitHub Actions (Release)"
participant Build as "build job"
participant Registry as "OCI Registry"
participant E2E as "e2e job"
participant Artifacts as "Artifact Storage"
participant Sandbox as "Sandbox (prepare-env)"
Maintainer->>GitHub: Push tag v*.*.*
GitHub->>Build: Start build job
Build->>Registry: Log in
Build->>Build: make build PUSH=1
Build->>Build: Build Talos image
Build->>Artifacts: Upload Talos raw artifact
Build-->>GitHub: build complete
GitHub->>E2E: Start e2e job (needs: build)
E2E->>Artifacts: Download Talos artifact
E2E->>Sandbox: Retry prepare-env (up to 3×)
Sandbox-->>E2E: Sandbox ready
E2E->>E2E: Install Cozystack & run OpenAPI tests
E2E->>E2E: For each `hack/e2e-apps/*.bats`: run `test-apps-$app`
E2E->>Artifacts: Collect cozyreport.tgz
E2E->>Sandbox: Teardown
E2E->>E2E: Remove workspace
E2E-->>GitHub: Exit 0 or 1
GitHub-->>Maintainer: Workflow result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces hack/select-e2e.sh, a script designed to optimize CI runs by identifying and executing only the E2E tests affected by specific code changes, along with a comprehensive test suite in hack/select-e2e_test.bats. The review feedback highlights several opportunities for optimization and robustness: reducing process overhead by batching yq calls and streamlining the all_apps calculation, using exact matching in grep to prevent false positives with hyphenated package names, and ensuring clean output formatting by avoiding trailing spaces.
|
|
||
| # Deduplicate; intersect with available bats files. | ||
| final_apps=$(echo "$final" | tr ' ' '\n' | sort -u | grep -v '^$' | while read -r app; do | ||
| if echo "$all_apps" | grep -qw "$app"; then |
There was a problem hiding this comment.
Using grep -w can lead to false positives because characters like - are often treated as word boundaries in many grep implementations. For example, grep -w "postgres" would match postgres-operator. Since all_apps is newline-separated, grep -Fxq is safer as it matches the entire line exactly.
| if echo "$all_apps" | grep -qw "$app"; then | |
| if echo "$all_apps" | grep -Fxq "$app"; then |
There was a problem hiding this comment.
Good catch — switched to grep -Fxq so apps with hyphenated names can never prefix-match each other. Fixed in 6e5379e.
| build_owners_index() { | ||
| for f in "$SOURCES_DIR"/*.yaml; do | ||
| src=$(yq -r '.metadata.name' "$f") | ||
| yq -r '.spec.variants[]?.components[]?.path // ""' "$f" | while read -r path; do | ||
| if [ -n "$path" ]; then echo "$path $src"; fi | ||
| done | ||
| done | ||
| } |
There was a problem hiding this comment.
This function can be significantly optimized by passing all files to a single yq process instead of calling yq twice per file in a loop. This reduces process overhead and improves performance in CI environments.
| build_owners_index() { | |
| for f in "$SOURCES_DIR"/*.yaml; do | |
| src=$(yq -r '.metadata.name' "$f") | |
| yq -r '.spec.variants[]?.components[]?.path // ""' "$f" | while read -r path; do | |
| if [ -n "$path" ]; then echo "$path $src"; fi | |
| done | |
| done | |
| } | |
| build_owners_index() { | |
| yq -r '.metadata.name as $n | .spec.variants[]?.components[]?.path | select(. != null and . != "") | . + "\t" + $n' "$SOURCES_DIR"/*.yaml | |
| } |
There was a problem hiding this comment.
Folded into a single yq invocation with -N to suppress the document separator (otherwise yq emits --- between docs and the output picks up stray blank lines). About 30× faster on 91 source files; output is byte-identical to the loop. Fixed in 6e5379e.
| build_reverse_deps() { | ||
| for f in "$SOURCES_DIR"/*.yaml; do | ||
| src=$(yq -r '.metadata.name' "$f") | ||
| yq -r '.spec.variants[]?.dependsOn[]? // ""' "$f" | while read -r dep; do | ||
| if [ -n "$dep" ]; then echo "$dep $src"; fi | ||
| done | ||
| done | ||
| } |
There was a problem hiding this comment.
Similar to build_owners_index, this function can be optimized by using a single yq call for all files to avoid the overhead of spawning a process for every YAML file.
| build_reverse_deps() { | |
| for f in "$SOURCES_DIR"/*.yaml; do | |
| src=$(yq -r '.metadata.name' "$f") | |
| yq -r '.spec.variants[]?.dependsOn[]? // ""' "$f" | while read -r dep; do | |
| if [ -n "$dep" ]; then echo "$dep $src"; fi | |
| done | |
| done | |
| } | |
| build_reverse_deps() { | |
| yq -r '.metadata.name as $n | .spec.variants[]?.dependsOn[]? | select(. != null and . != "") | . + "\t" + $n' "$SOURCES_DIR"/*.yaml | |
| } |
There was a problem hiding this comment.
Same treatment as build_owners_index — single yq with -N. Fixed in 6e5379e.
| full_suite_pattern='^(packages/library/|packages/core/|api/|cmd/|internal/|hack/[^/]+\.sh$|hack/[^/]+\.bats$|hack/e2e-apps/[^/]+\.sh$|Makefile$|\.github/workflows/(pull-requests|release-e2e)\.yaml$)' | ||
|
|
||
| # All known per-app bats files | ||
| all_apps=$(ls hack/e2e-apps/*.bats 2>/dev/null | xargs -n1 basename | sed 's/\.bats$//') |
There was a problem hiding this comment.
The calculation of all_apps can be optimized by avoiding multiple basename processes via xargs. Using sed to strip the path and extension in a single pipeline is more efficient.
| all_apps=$(ls hack/e2e-apps/*.bats 2>/dev/null | xargs -n1 basename | sed 's/\.bats$//') | |
| all_apps=$(ls hack/e2e-apps/*.bats 2>/dev/null | sed 's,.*/,,; s/\.bats$//') |
There was a problem hiding this comment.
Keeping ls hack/e2e-apps/*.bats | xargs -n1 basename | sed 's/\.bats$//' — the intent (extract filename, strip extension) reads more obviously than a chained sed, and the cost on ~16 files is negligible.
| done < "$CHANGED" | ||
|
|
||
| if [ "$trigger_full" = 1 ]; then | ||
| echo "$all_apps" | tr '\n' ' ' |
There was a problem hiding this comment.
Using tr '\n' ' ' leaves a trailing space in the output. Using paste -sd ' ' - is a cleaner way to join lines with spaces and is consistent with how it's used later in the script.
| echo "$all_apps" | tr '\n' ' ' | |
| echo "$all_apps" | paste -sd ' ' - |
There was a problem hiding this comment.
Switched to paste -sd ' ' - for consistency with the existing call at line 147. Fixed in 6e5379e.
| # silently skip E2E. Fall back to full suite so a path inside the graph is | ||
| # never silently dropped. | ||
| if [ -z "$final_apps" ]; then | ||
| echo "$all_apps" | tr '\n' ' ' |
There was a problem hiding this comment.
Use paste to avoid a trailing space in the output for the full suite fallback.
| echo "$all_apps" | tr '\n' ' ' | |
| echo "$all_apps" | paste -sd ' ' - |
There was a problem hiding this comment.
Same — paste -sd ' ' -. Fixed in 6e5379e.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/select-e2e.sh (1)
25-25: 💤 Low valueConsider using
findinstead ofls | xargsfor robustness.While bats filenames in this project are controlled and unlikely to contain special characters, using
findwould be more robust against edge cases.♻️ Optional fix
-all_apps=$(ls hack/e2e-apps/*.bats 2>/dev/null | xargs -n1 basename | sed 's/\.bats$//') +all_apps=$(find hack/e2e-apps -maxdepth 1 -name '*.bats' -exec basename {} .bats \; 2>/dev/null | sort)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/select-e2e.sh` at line 25, Replace the fragile pipeline that builds all_apps using "ls | xargs | sed" with a robust find-based command: update the assignment to all_apps (the variable defined in the script) so it uses find to locate files under hack/e2e-apps with a .bats suffix, prints only the basename without the extension, and handles missing files without errors; keep the resulting value format identical (space-separated names) so downstream references to all_apps continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/select-e2e_test.bats`:
- Around line 75-80: The test "per-app bats edit selects only that app, never
escalates" uses an exact string equality check ([ "$output" = "redis" ]) which
can fail due to trailing newlines/whitespace; update the assertion in
hack/select-e2e_test.bats to mirror the postgres fix by comparing a trimmed
output or using a pattern match (e.g., checking that $output contains or matches
"redis") so trailing whitespace/newlines don't cause flakiness, and ensure the
same approach is used in the parallel postgres test for consistency; modify the
assertion in this test (the block starting with `@test` "per-app bats edit selects
only that app, never escalates") accordingly.
---
Nitpick comments:
In `@hack/select-e2e.sh`:
- Line 25: Replace the fragile pipeline that builds all_apps using "ls | xargs |
sed" with a robust find-based command: update the assignment to all_apps (the
variable defined in the script) so it uses find to locate files under
hack/e2e-apps with a .bats suffix, prints only the basename without the
extension, and handles missing files without errors; keep the resulting value
format identical (space-separated names) so downstream references to all_apps
continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65c506ce-adba-4b35-8924-97b3c1e54601
📒 Files selected for processing (4)
.github/workflows/pull-requests.yaml.github/workflows/release-e2e.yamlhack/select-e2e.shhack/select-e2e_test.bats
- Use grep -Fxq instead of -w when intersecting with all_apps so apps with hyphenated names cannot prefix-match each other. - Collapse build_owners_index and build_reverse_deps into single yq invocations with -N. Output is identical to the per-file loop and about 30x faster on the current 91 PackageSource files. - Use paste -sd ' ' - on the two full-suite emit paths for consistency with the post-walk join at line 147. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/select-e2e.sh`:
- Around line 80-94: The current logic only recognizes
packages/(apps|system|extra) when computing rel and silently ignores other
packages/* paths; change the script so any path matching ^packages/[^/]+/ that
does not produce a mapped rel triggers a full-suite run: after computing rel and
before the final else branch, detect if "$file" matches the packages/* pattern
and if so set trigger_full=1 (instead of ignoring), preserving existing behavior
for when src is found (selected_sources addition using selected_sources and src)
and for non-packages paths.
- Line 25: Replace the fragile ls|xargs pipeline that populates all_apps with a
safe shell glob loop: iterate over the hack/e2e-apps/*.bats pattern, skip
non-matches, extract the base name via parameter expansion (e.g.,
name="${f##*/}" and strip the .bats suffix with "${name%.bats}"), and append
each safe filename to the all_apps variable; update the assignment that
currently uses all_apps=$(ls ... | xargs ... | sed ...) to this loop in
hack/select-e2e.sh so filenames with spaces or leading dashes are handled
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a576c098-29cc-475d-92e7-d17854014d5b
📒 Files selected for processing (1)
hack/select-e2e.sh
| full_suite_pattern='^(packages/library/|packages/core/|api/|cmd/|internal/|hack/[^/]+\.sh$|hack/[^/]+\.bats$|hack/e2e-apps/[^/]+\.sh$|Makefile$|\.github/workflows/(pull-requests|release-e2e)\.yaml$)' | ||
|
|
||
| # All known per-app bats files | ||
| all_apps=$(ls hack/e2e-apps/*.bats 2>/dev/null | xargs -n1 basename | sed 's/\.bats$//') |
There was a problem hiding this comment.
Avoid parsing ls here.
ls | xargs basename is brittle in sh: filenames with spaces/leading dashes will be mangled, and an empty glob can make the command behave unexpectedly under set -e. Please verify this path with a simple filename that contains whitespace.
🔎 Read-only verification script
#!/bin/sh
set -eu
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/hack/e2e-apps"
: > "$tmp/hack/e2e-apps/foo bar.bats"
: > "$tmp/hack/e2e-apps/baz.bats"
cd "$tmp"
ls hack/e2e-apps/*.bats 2>/dev/null | xargs -n1 basename | sed 's/\.bats$//'🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 25-25: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2011)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/select-e2e.sh` at line 25, Replace the fragile ls|xargs pipeline that
populates all_apps with a safe shell glob loop: iterate over the
hack/e2e-apps/*.bats pattern, skip non-matches, extract the base name via
parameter expansion (e.g., name="${f##*/}" and strip the .bats suffix with
"${name%.bats}"), and append each safe filename to the all_apps variable; update
the assignment that currently uses all_apps=$(ls ... | xargs ... | sed ...) to
this loop in hack/select-e2e.sh so filenames with spaces or leading dashes are
handled correctly.
| # 4. Component change: lookup in PackageSource graph | ||
| rel=$(echo "$file" | sed -nE 's,^packages/(apps|system|extra)/([^/]+)/.*,\1/\2,p') | ||
| if [ -n "$rel" ]; then | ||
| src=$(echo "$OWNERS" | awk -v p="$rel" -F'\t' '$1==p {print $2}') | ||
| if [ -n "$src" ]; then | ||
| selected_sources="$selected_sources $src" | ||
| trigger_any=1 | ||
| else | ||
| # Inside packages/ but no graph entry — be conservative. | ||
| trigger_full=1 | ||
| fi | ||
| fi | ||
| # Anything else (e.g. unrelated workflow files, top-level configs) is | ||
| # silently ignored. | ||
| done < "$CHANGED" |
There was a problem hiding this comment.
Treat unknown packages/* paths as full-suite triggers.
Right now only packages/apps, packages/system, and packages/extra are mapped into rel. Any future or renamed subtree under packages/ that does not match those prefixes is silently ignored, even though the comment and PR objective say unrecognized packages/* paths should run the full suite.
♻️ Suggested fix
rel=$(echo "$file" | sed -nE 's,^packages/(apps|system|extra)/([^/]+)/.*,\1/\2,p')
if [ -n "$rel" ]; then
src=$(echo "$OWNERS" | awk -v p="$rel" -F'\t' '$1==p {print $2}')
if [ -n "$src" ]; then
selected_sources="$selected_sources $src"
trigger_any=1
else
# Inside packages/ but no graph entry — be conservative.
trigger_full=1
fi
+ elif echo "$file" | grep -qE '^packages/'; then
+ trigger_full=1
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/select-e2e.sh` around lines 80 - 94, The current logic only recognizes
packages/(apps|system|extra) when computing rel and silently ignores other
packages/* paths; change the script so any path matching ^packages/[^/]+/ that
does not produce a mapped rel triggers a full-suite run: after computing rel and
before the final else branch, detect if "$file" matches the packages/* pattern
and if so set trigger_full=1 (instead of ignoring), preserving existing behavior
for when src is found (selected_sources addition using selected_sources and src)
and for non-packages paths.
CI invokes hack/cozytest.sh, a project-local pure-shell harness, not bats. cozytest does not honor setup()/teardown() and does not provide bats' run / $status / $output, so the original tests failed with "TMPDIR: parameter not set" under set -u. Inline tmpdir creation and an EXIT trap for cleanup in each test. Replace `run` + `$output` with direct command substitution, and replace `! grep` (which set -e ignores) with an explicit if-then-exit. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The "Select E2E tests" step runs `git diff --name-only
origin/${BASE_REF}...HEAD` inside the sandbox copy of the workspace.
Without fetch-depth: 0 the e2e job's checkout only resolves a single
commit, so origin/main is missing and the diff aborts with
"ambiguous argument 'origin/main...HEAD': unknown revision".
Mirror the Build job's setup and request full history.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pull-requests.yaml:
- Around line 256-288: The E2E selector step (name: "Select E2E tests", id:
select) runs after sandbox provisioning and OpenAPI work, so move this
lightweight selector earlier in the workflow (before provisioning/CozyStack
install) and then gate the heavy steps (the "Run OpenAPI tests" step and any
sandbox/provisioning steps) on its output (use steps.select.outputs.skip !=
'true') so those expensive actions are skipped when skip=true; update the
conditions for the provisioning/OpenAPI/E2E steps to reference
steps.select.outputs.skip and ensure the selector still emits "skip" and "apps"
to $GITHUB_OUTPUT.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ea8f411-21f8-4c23-a462-e5ac39629e14
📒 Files selected for processing (1)
.github/workflows/pull-requests.yaml
| # Test Impact Analysis (TIA): walk the PackageSource graph for the PR | ||
| # diff and select only the bats files affected. Runs by default; opt | ||
| # out via the `full-e2e` label to run the full suite. | ||
| # `skip=true` means nothing to test (docs-only PR). | ||
| - name: Select E2E tests | ||
| id: select | ||
| if: ${{ !contains(github.event.pull_request.labels.*.name, 'full-e2e') }} | ||
| env: | ||
| BASE_REF: ${{ github.base_ref }} | ||
| run: | | ||
| cd /tmp/$SANDBOX_NAME | ||
| git diff --name-only "origin/${BASE_REF}...HEAD" > /tmp/changed.txt | ||
| apps=$(./hack/select-e2e.sh /tmp/changed.txt) | ||
| echo "Selected apps: ${apps:-<none>}" | ||
| echo "apps=${apps}" >> $GITHUB_OUTPUT | ||
| if [ -z "${apps}" ]; then | ||
| echo "skip=true" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "skip=false" >> $GITHUB_OUTPUT | ||
| fi | ||
|
|
||
| - name: Run OpenAPI tests | ||
| run: | | ||
| cd /tmp/$SANDBOX_NAME | ||
| make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME test-openapi | ||
|
|
||
| # ▸ Run E2E tests | ||
| # Run E2E tests. With `full-e2e` label the selector is skipped, its | ||
| # outputs are empty, the != 'true' guard lets this step run, and the | ||
| # full bats list is used. Without the label, the selector decides; | ||
| # an empty selection (docs-only) skips this step entirely. | ||
| - name: Run E2E tests | ||
| id: e2e_tests | ||
| if: ${{ steps.select.outputs.skip != 'true' }} |
There was a problem hiding this comment.
Gate the expensive E2E path before provisioning.
skip=true is only computed after sandbox prep, Cozystack install, and OpenAPI have already run, so PRs with no affected bats still consume almost the full E2E job. To get the intended CI savings, move the selector into a lightweight upstream job (or at least before provisioning) and gate the heavy steps on its outputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pull-requests.yaml around lines 256 - 288, The E2E
selector step (name: "Select E2E tests", id: select) runs after sandbox
provisioning and OpenAPI work, so move this lightweight selector earlier in the
workflow (before provisioning/CozyStack install) and then gate the heavy steps
(the "Run OpenAPI tests" step and any sandbox/provisioning steps) on its output
(use steps.select.outputs.skip != 'true') so those expensive actions are skipped
when skip=true; update the conditions for the provisioning/OpenAPI/E2E steps to
reference steps.select.outputs.skip and ensure the selector still emits "skip"
and "apps" to $GITHUB_OUTPUT.
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — fail-safe test-impact selector (over-selects on any uncertainty, never silently drops an app) backed by a release-tag full-suite safety net; matching is precise and the 11-case bats suite covers the key paths.
Business context: PR E2E ran the full suite on every change; this adds diff-based test-impact analysis so a PR only runs the bats files its diff affects, with the full suite preserved at release-tag time so shipped code keeps full coverage.
Verified: the selector is fail-safe in every uncertain case — full_suite_pattern escalates library/core/api/cmd/internal/shared-hack/Makefile/workflow changes to the full suite, an app resolved from the dependency graph that is not a real bats app falls back to the full suite (the explicit "never silently skip" guard), and matching uses anchored grep -qE '^...' and fixed-string whole-line grep -Fxq, not word-boundary matching — so the grep -w concern raised on the thread does not apply to the current code. Per-app bats edits select only that app (checked before the full-suite trigger), which is correct since a bats file is that app's own test. hack/select-e2e_test.bats passes all 11 cases (single app, transitive operator, networking/library full-suite, docs/dashboards skip, two-bats kubernetes-application, shared helper, install bats, per-app no-escalation, release-workflow change). The release-e2e workflow runs independently of the publish flow, so a failure alarms without blocking the release.
Non-blocking: the build_owners_index / all_apps helpers shell out per file; batching into a single yq / basename pass (as suggested on the thread) would speed the selector up, though it runs once per PR so the cost is negligible. The residual gap — app-to-app runtime coupling that has no PackageSource edge — is the one case TIA can miss, and it is exactly what the release-tag full suite is there to catch.
Reconcile the parallel-matrix PR build (#2983) with main, which since this branch diverged landed a competing single-VM build restructure (#2939) and test-impact-analysis E2E (#2559). Per maintainer decision, #2983's matrix build supersedes #2939: - .github/workflows/pull-requests.yaml: keep #2983's plan/checks/build(matrix)/build-talos/finalize jobs; keep main's newer resolve_assets/e2e (carrying #2559 test-impact E2E). The build->e2e artifact interface (pr-patch + talos-image) is identical on both sides, so e2e depends on finalize/build-talos with no input rewiring. main's top-level permissions and SHA-pinned action refs applied across all jobs. - All other files merged cleanly (only pull-requests.yaml conflicted). Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
What this PR does
Adds test-impact analysis (TIA) to the PR E2E workflow so that only bats files affected by the diff are exercised, and a release-time E2E workflow that runs the full suite on every release tag to keep coverage on the shipped code.
1. Test-impact analysis selector —
hack/select-e2e.shReads
packages/core/platform/sources/*.yaml(the PackageSource dependency graph) and emits the bats files affected by a PR diff. The walk is the same onecozypkg dotrenders.packages/apps/postgres/values.yamlpostgrespackages/system/postgres-operator/...postgres harbor(transitive)packages/system/cilium/...*-applicationdependents)packages/library/cozy-lib/...hack/e2e-install-cozystack.batshack/e2e-apps/redis.batsredis(per-app edit, never escalates)docs/agents/contributing.mddashboards/gpu/gpu-fleet.jsonConservative fallbacks:
packages/*path or a system source with no*-applicationdescendants escalates to the full suite, so a path inside the graph is never silently dropped.2. Default-on TIA +
full-e2eopt-out —pull-requests.yamlSelect E2E testsstep runs by default and producesapps+skipoutputs.Run E2E testsruns unlessskip=true(docs / dashboards /*.mdonly).full-e2elabel skips the selector step (output empty) and falls through to the unchanged full-bats-list path — the safety hatch when reviewers want the full suite on a PR.3.
release-e2e.yaml— full E2E on every release tagTriggered on
v*.*.*,v*.*.*-rc.*,v*.*.*-beta.*,v*.*.*-alpha.*. Builds images + Talos image, then runs the full suite. Independent oftags.yaml's release publish flow — failure here is an alarm, not a release blocker.This closes the coverage gap left by making PR runs default to TIA: the full suite is now exercised at tag-cut time against the shipped code.
Tests
hack/select-e2e_test.batscovers 11 cases — single app, transitive operator, networking/library full-suite, docs/dashboards skip,kubernetes-applicationtwo-bats mapping, shared E2E helper, install bats, per-app bats no-escalation, release-e2e workflow change.YAML parse verified for both new workflow files.
Workflow injection safety
All
${{ }}contexts that could carry untrusted data (github.base_ref, label arrays) flow through env vars, never directly into shell text.github.workspaceis also indirected throughWORKSPACEenv in the release workflow.Release note
Summary by CodeRabbit
Chores
Tests