ci(e2e): step 4+5 — push:main trigger + attn:e2e-failure surfacing + testing-strategy doc (#370) - #371
Merged
Conversation
Closes #334 step 4. Workflow now runs on every push to main as well as labeled PRs. A new surface-failure job opens an attn:e2e-failure issue on push:main failure (referencing the failing SHA + run URL) and adds attn:e2e-failure to the PR on PR-level failure. Both routes feed aelf-scan §1 so a downstream session picks up the regression.
Closes #334 step 5. Companion doc for the e2e workflow. Tells future PR authors which layer a new test belongs in (decision tree at the bottom), what regression class each seed E2E scenario catches, and where the bench / flake budgets live.
Reviewer's GuideExtends the E2E GitHub Actions workflow to run on pushes to main and adds a dedicated failure-surfacing job that labels failing PRs or opens issues for failing main runs, and introduces a testing strategy document that formalizes the unit/integration/E2E split and when to use each layer. Sequence diagram for E2E workflow triggers and failure surfacingsequenceDiagram
actor Developer
participant GitHub as GitHub_Actions
participant e2e as e2e_job
participant surf as surface_failure_job
participant GHAPI as GitHub_CLI_API
Developer->>GitHub: push_to_main
GitHub->>e2e: start_job (event_name push)
e2e-->>GitHub: job_status (success_or_failure)
alt e2e_success_on_push
GitHub-->>Developer: status_check_success
else e2e_failure_on_push
GitHub->>surf: start_job (needs e2e, if failure())
surf->>GHAPI: gh_issue_create attn:e2e-failure with SHA and run_url
GHAPI-->>surf: issue_created
surf-->>Developer: new_issue_linked_to_failing_run
end
Developer->>GitHub: open_or_update_PR_with_e2e_label
GitHub->>e2e: start_job (event_name pull_request)
e2e-->>GitHub: job_status (success_or_failure)
alt e2e_success_on_PR
GitHub-->>Developer: status_check_success
else e2e_failure_on_PR
GitHub->>surf: start_job (needs e2e, if failure())
surf->>GHAPI: gh_pr_edit add_label attn:e2e-failure
GHAPI-->>surf: pr_labeled
surf-->>Developer: PR_labeled_attn_e2e_failure
end
Flow diagram for choosing test layer (unit, integration, E2E)flowchart TD
Start([New_test_needed])
Q_binary{Behavior only observable\nwith installed_binary\nor install_method?}
Q_db_contract{Requires_real_DB_and\ncontract_between_multiple_modules\nwhile staying_in_process?}
Unit_layer[Layer 1 unit tests\nlocation: tests/test_*.py\nfast, module_isolation, mocks_ok]
Integration_layer[Layer 2 integration tests\nlocation: tests/test_*.py real_store\nreal BeliefStore, no store/schema/migration mocks]
E2E_layer[Layer 3 E2E tests\nlocation: tests/e2e/test_*.py\ninstalled aelf binary via subprocess,
no mocks, install_matrix]
Start --> Q_binary
Q_binary -->|yes| E2E_layer
Q_binary -->|no| Q_db_contract
Q_db_contract -->|yes| Integration_layer
Q_db_contract -->|no| Unit_layer
Unit_layer --> End([Test_layer_selected])
Integration_layer --> End
E2E_layer --> End
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe PR extends the E2E workflow to run on every ChangesE2E Workflow Trigger and Failure Surfacing
Testing Strategy Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 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)
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: 0/1 reviews remaining, refill in 60 minutes.Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the
e2ejobif:expression, consider guarding the label check by event type (e.g.github.event_name == 'push' || (github.event_name == 'pull_request' && contains(...))) so thatgithub.event.pull_request.labelsis never accessed on non-PR events. - For the
surface-failurejob, using an explicit needs check likeif: needs.e2e.result == 'failure'is more robust and idiomatic than relying onif: failure()at the job level to detect the failinge2ejob.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `e2e` job `if:` expression, consider guarding the label check by event type (e.g. `github.event_name == 'push' || (github.event_name == 'pull_request' && contains(...))`) so that `github.event.pull_request.labels` is never accessed on non-PR events.
- For the `surface-failure` job, using an explicit needs check like `if: needs.e2e.result == 'failure'` is more robust and idiomatic than relying on `if: failure()` at the job level to detect the failing `e2e` job.
## Individual Comments
### Comment 1
<location path=".github/workflows/e2e.yml" line_range="25-27" />
<code_context>
jobs:
e2e:
- if: contains(github.event.pull_request.labels.*.name, 'e2e')
+ if: github.event_name == 'push' || contains(github.event.pull_request.labels.*.name, 'e2e')
runs-on: ubuntu-latest
timeout-minutes: 8
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard PR-specific context usage to avoid accessing pull_request fields on push events.
On `push` events, `github.event.pull_request` is undefined, and GitHub’s expression handling can be brittle when accessing missing properties. To avoid any chance of evaluation errors, scope the label check to PR events, e.g.
```yaml
if: github.event_name == 'push' || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'e2e'))
```
```suggestion
e2e:
if: github.event_name == 'push' || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'e2e'))
runs-on: ubuntu-latest
```
</issue_to_address>
### Comment 2
<location path="docs/testing-strategy.md" line_range="82" />
<code_context>
+(install-time wiring, install-method-specific behavior, real subprocess)?
+ → Layer 3 (E2E).
+
+Does the test require a real DB and exercises a contract between
+two or more modules, but stays in-process?
+ → Layer 2 (integration), in tests/.
</code_context>
<issue_to_address>
**issue (typo):** Fix verb agreement in this question sentence.
For example, you could rewrite this as: "Does the test require a real DB and exercise a contract between ..." or "Does the test require a real DB, and does it exercise a contract between ..." to fix the verb agreement.
```suggestion
Does the test require a real DB and exercise a contract between
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
25
to
27
| e2e: | ||
| if: contains(github.event.pull_request.labels.*.name, 'e2e') | ||
| if: github.event_name == 'push' || contains(github.event.pull_request.labels.*.name, 'e2e') | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
suggestion (bug_risk): Guard PR-specific context usage to avoid accessing pull_request fields on push events.
On push events, github.event.pull_request is undefined, and GitHub’s expression handling can be brittle when accessing missing properties. To avoid any chance of evaluation errors, scope the label check to PR events, e.g.
if: github.event_name == 'push' || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'e2e'))
Suggested change
| e2e: | |
| if: contains(github.event.pull_request.labels.*.name, 'e2e') | |
| if: github.event_name == 'push' || contains(github.event.pull_request.labels.*.name, 'e2e') | |
| runs-on: ubuntu-latest | |
| e2e: | |
| if: github.event_name == 'push' || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'e2e')) | |
| runs-on: ubuntu-latest |
| (install-time wiring, install-method-specific behavior, real subprocess)? | ||
| → Layer 3 (E2E). | ||
|
|
||
| Does the test require a real DB and exercises a contract between |
There was a problem hiding this comment.
issue (typo): Fix verb agreement in this question sentence.
For example, you could rewrite this as: "Does the test require a real DB and exercise a contract between ..." or "Does the test require a real DB, and does it exercise a contract between ..." to fix the verb agreement.
Suggested change
| Does the test require a real DB and exercises a contract between | |
| Does the test require a real DB and exercise a contract between |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/e2e.yml (3)
69-72: ⚡ Quick winAdd a short
timeout-minutesto thesurface-failurejob.Without it, a hung
ghCLI call (rate-limit backoff, transient API outage) burns the default 6-hour job timeout for what should be a sub-minute operation.⏱ Proposed fix
surface-failure: needs: e2e if: failure() + timeout-minutes: 5 runs-on: ubuntu-latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e.yml around lines 69 - 72, The surface-failure job lacks a timeout and can hang; update the GitHub Actions job definition for "surface-failure" to add a short timeout-minutes setting (e.g., timeout-minutes: 5) under the job block so the job will abort quickly on a hung gh CLI call; locate the "surface-failure" job in the workflow and add the timeout-minutes key alongside runs-on and needs.
14-15:cancel-in-progress: truesilently drops failure surfacing for cancelled push:main runs.The concurrency group for push events collapses to a single key (
e2e-E2E-refs/heads/main). If commit A breaks e2e and commit B is pushed before A's run finishes, A's run is cancelled.surface-failureonly fires on afailureresult — acancelledresult is notfailure(), so no issue is opened for commit A. The regression on A is silently lost if B's run passes.For push:main regression detection you may want to set
cancel-in-progress: falsefor push events, or use a per-SHA concurrency group:⚙️ Option: per-SHA group for push:main, cancellable for PRs
concurrency: - group: e2e-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} - cancel-in-progress: true + group: e2e-${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e.yml around lines 14 - 15, The workflow's concurrency setting causes in-flight push: main jobs to be cancelled (cancel-in-progress: true) which turns failures into cancelled results and prevents surface-failure from opening issues; to fix, change the concurrency behavior for push: main by either setting cancel-in-progress: false for the push concurrency group or switch the group's key to a per-SHA value (e.g., use github.sha in the concurrency.group) so only identical-SHA runs collide; update the concurrency block where cancel-in-progress is defined and ensure surface-failure still triggers on actual failure() results.
97-105: Each push:main failure opens a new issue — no deduplication guard.If main stays broken across several commits (or while a fix PR is being reviewed), every push triggers another
gh issue create, accumulating duplicate open issues. Consider searching for an existing open issue with theattn:e2e-failurelabel before creating a new one:🔁 Option: skip creation if an open tracking issue already exists
run: | short_sha="${SHA:0:7}" + existing=$(gh issue list \ + --repo "$REPO" \ + --label "attn:e2e-failure" \ + --state open \ + --json number \ + --jq 'length') + if [ "$existing" -gt 0 ]; then + echo "Open attn:e2e-failure issue already exists; skipping creation." + exit 0 + fi gh issue create \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e.yml around lines 97 - 105, Before running the gh issue create step, query GitHub for an existing open issue with the attn:e2e-failure label and only create a new issue if none exists; specifically, call the GitHub CLI to list open issues for the same "$REPO" filtered by label "attn:e2e-failure" (e.g., gh issue list --repo "$REPO" --label "attn:e2e-failure" --state open) and conditionally execute the existing gh issue create command when that list is empty, ensuring you preserve the same title/body/labels and environ vars (short_sha, SHA, RUN_URL) used in the current gh issue create invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/testing-strategy.md`:
- Around line 77-88: The fenced code block that starts with "Is the test
behavior observable only when the binary is installed" is missing a language
specifier (MD040); update that triple-backtick fence to include a language token
(use "text") so the block becomes ```text to satisfy markdownlint-cli2 without
changing rendered output, targeting the fenced block containing the three Q/A
lines.
---
Nitpick comments:
In @.github/workflows/e2e.yml:
- Around line 69-72: The surface-failure job lacks a timeout and can hang;
update the GitHub Actions job definition for "surface-failure" to add a short
timeout-minutes setting (e.g., timeout-minutes: 5) under the job block so the
job will abort quickly on a hung gh CLI call; locate the "surface-failure" job
in the workflow and add the timeout-minutes key alongside runs-on and needs.
- Around line 14-15: The workflow's concurrency setting causes in-flight push:
main jobs to be cancelled (cancel-in-progress: true) which turns failures into
cancelled results and prevents surface-failure from opening issues; to fix,
change the concurrency behavior for push: main by either setting
cancel-in-progress: false for the push concurrency group or switch the group's
key to a per-SHA value (e.g., use github.sha in the concurrency.group) so only
identical-SHA runs collide; update the concurrency block where
cancel-in-progress is defined and ensure surface-failure still triggers on
actual failure() results.
- Around line 97-105: Before running the gh issue create step, query GitHub for
an existing open issue with the attn:e2e-failure label and only create a new
issue if none exists; specifically, call the GitHub CLI to list open issues for
the same "$REPO" filtered by label "attn:e2e-failure" (e.g., gh issue list
--repo "$REPO" --label "attn:e2e-failure" --state open) and conditionally
execute the existing gh issue create command when that list is empty, ensuring
you preserve the same title/body/labels and environ vars (short_sha, SHA,
RUN_URL) used in the current gh issue create invocation.
🪄 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: b1aaca68-a44f-4afd-8416-4f74d2fee824
📒 Files selected for processing (2)
.github/workflows/e2e.ymldocs/testing-strategy.md
Comment on lines
+77
to
+88
| ``` | ||
| Is the test behavior observable only when the binary is installed | ||
| (install-time wiring, install-method-specific behavior, real subprocess)? | ||
| → Layer 3 (E2E). | ||
|
|
||
| Does the test require a real DB and exercises a contract between | ||
| two or more modules, but stays in-process? | ||
| → Layer 2 (integration), in tests/. | ||
|
|
||
| Otherwise — single module, mocks acceptable? | ||
| → Layer 1 (unit), in tests/. | ||
| ``` |
There was a problem hiding this comment.
Fenced code block is missing a language specifier (MD040).
markdownlint-cli2 flags this block. Adding text satisfies the rule with no rendering change.
📝 Proposed fix
-```
+```text
Is the test behavior observable only when the binary is installed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| Is the test behavior observable only when the binary is installed | |
| (install-time wiring, install-method-specific behavior, real subprocess)? | |
| → Layer 3 (E2E). | |
| Does the test require a real DB and exercises a contract between | |
| two or more modules, but stays in-process? | |
| → Layer 2 (integration), in tests/. | |
| Otherwise — single module, mocks acceptable? | |
| → Layer 1 (unit), in tests/. | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/testing-strategy.md` around lines 77 - 88, The fenced code block that
starts with "Is the test behavior observable only when the binary is installed"
is missing a language specifier (MD040); update that triple-backtick fence to
include a language token (use "text") so the block becomes ```text to satisfy
markdownlint-cli2 without changing rendered output, targeting the fenced block
containing the three Q/A lines.
Collaborator
Author
|
[claim:review:Setr:2026-05-03T15:00:04Z] |
Collaborator
Author
|
[claim:review:Gylf:2026-05-03T15:00:35Z] |
Collaborator
Author
|
[release:review:Gylf:2026-05-03T15:00:40Z] |
Collaborator
Author
|
[release:review:Setr:2026-05-03T15:01:13Z] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #370. Lands the two acceptance items left over from umbrella #334 after it closed on the seed-scenario gate.
What changes
Commit 1 —
ci(e2e): trigger on push:main + open attn:e2e-failure on failure.github/workflows/e2e.yml: addpush: { branches: [main] }trigger.e2ejob'sif:now matchespushOR labeled PR (was: labeled-PR-only).surface-failurejob (needs: e2e,if: failure()):gh pr edit --add-label attn:e2e-failure.gh issue createwithattn:e2e-failure, referencing the failing SHA + run URL.issues: write,pull-requests: writeon the surface-failure job only; the e2e job stayscontents: read.attn:e2e-failurelabel created in the repo (was missing).Commit 2 —
docs(testing-strategy): document unit / integration / E2E splitdocs/testing-strategy.md. Decision-tree at the bottom for "which layer does my new test go in", regression-class table for each seed E2E scenario, references to the bench / flake budgets in ci: end-to-end integration test job — catch system-level regressions per-merge #334.Verification
python -c "import yaml; yaml.safe_load(open('.github/workflows/e2e.yml'))"— clean.uvx zizmor .github/workflows/e2e.yml— no findings (5 suppressed, all pre-existing).Out of scope (explicitly deferred per #334)
aelf upgrade-advice) — additive follow-ups.Summary by Sourcery
Extend the E2E workflow to run on main-branch pushes and surface failures via labels/issues, and document the testing-layer strategy for the project.
CI:
Documentation:
Summary by CodeRabbit
Chores
Documentation