Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 47 additions & 4 deletions .github/workflows/e2e.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
name: E2E

# End-to-end suite (#334). Step-1 trigger: PR opt-in via the `e2e` label
# only. Step-4 of the rollout flips this to also run on push to main and
# adds the `attn:e2e-failure` issue-opening step. See umbrella issue.
# End-to-end suite (#334). Triggers:
# - PR opt-in via the `e2e` label (label / synchronize / reopened).
# - push to main (post-merge regression catch).
# Failure surfacing (#370):
# - push:main fail → open issue tagged `attn:e2e-failure` referencing the SHA.
# - PR fail → add `attn:e2e-failure` label to the PR.

on:
pull_request:
types: [labeled, synchronize, reopened]
branches: [main]
push:
branches: [main]

permissions:
contents: read
Expand All @@ -18,7 +23,7 @@ concurrency:

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
Comment on lines 25 to 27

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

timeout-minutes: 8
strategy:
Expand Down Expand Up @@ -60,3 +65,41 @@ jobs:
run: |
uv sync --frozen --group dev
uv run pytest tests/e2e/ -q --maxfail=3

surface-failure:
needs: e2e
if: failure()
runs-on: ubuntu-latest
permissions:
issues: write
pull-requests: write
steps:
- uses: step-security/harden-runner@8d3c67de8e2fe68ef647c8db1e6a09f647780f40 # v2.19.0
with:
egress-policy: audit
- name: Label PR on failure
if: github.event_name == 'pull_request'
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
PR_NUMBER: ${{ github.event.pull_request.number }}
REPO: ${{ github.repository }}
run: |
gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "attn:e2e-failure"
- name: Open issue on push:main failure
if: github.event_name == 'push'
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPO: ${{ github.repository }}
SHA: ${{ github.sha }}
RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
run: |
short_sha="${SHA:0:7}"
gh issue create \
--repo "$REPO" \
--title "e2e: failure on main @ ${short_sha}" \
--label "attn:e2e-failure" \
--body "E2E job failed on push to \`main\` at commit \`${SHA}\`.

**Failing run:** ${RUN_URL}

This issue was opened automatically by \`.github/workflows/e2e.yml\` (#370). Triage: investigate the failing matrix leg(s), open a fix PR, then close this issue. If the failure is environmental and not a code regression, label \`attn:e2e-flake\` and quarantine within one business day per #334."
108 changes: 108 additions & 0 deletions docs/testing-strategy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
# Testing strategy

aelfrice ships three layers of automated tests. Each layer answers a different
question; landing a new test in the wrong layer produces either green-but-blind
coverage or slow-and-flaky CI. This page is the contract for where new tests go.

Companion to umbrella issue #334 (E2E job) and follow-up #370 (push:main +
failure surfacing).

## Layer 1 — unit tests (`tests/test_*.py`)

**Question answered:** does this module behave correctly in isolation?

- Run on every PR via the `pytest (3.12)` and `pytest (3.13)` matrix in
`.github/workflows/ci.yml`.
- In-process imports. Mocks are allowed for store init, network, subprocess.
- Should be fast (< 1s per file typical). The full unit suite is the dev
inner-loop signal.
- **Land here when:** you are adding or changing a single module's
behavior and the test can be written against that module's public API
with the rest of the system stubbed.

## Layer 2 — integration tests (`tests/test_*.py`, real-store)

**Question answered:** do these modules agree on a contract when wired through
a real `BeliefStore`?

- Same workflow file as unit tests; same matrix.
- In-process imports, but **no mocks for the store, schema, or migrations**.
Use `tmp_path` for the DB.
- Examples in tree: `test_legacy_migration.py`, `test_v1_to_v1x_migration.py`,
`test_retrieve_v2.py`, `test_replay_full_equality.py`.
- **Land here when:** the test exercises a contract between two or more
modules (e.g. ingest writes a column that retrieval reads), and the
contract is enforceable without crossing a process boundary.

## Layer 3 — end-to-end tests (`tests/e2e/test_*.py`)

**Question answered:** does the installed binary, against a real DB, behave
the way an external user would experience it?

- Run by `.github/workflows/e2e.yml`:
- on PR with the `e2e` label (opt-in for PRs touching cross-module seams),
- on every push to `main` (post-merge regression catch),
- across an install-method matrix: `uv-tool`, `pipx`, `venv-pip`.
- **No in-process imports of `aelfrice.*`.** Tests invoke the installed
`aelf` binary or `python -m aelfrice.mcp_server.serve` via `subprocess.run`
and assert on its observable output.
- No mocks at all. Fixtures: `tmp_path` DB, the public-safe project under
`tests/e2e/fixtures/tiny-project/`, and the synthetic v1.4 snapshot at
`tests/e2e/fixtures/v14-snapshot.db` (rebuildable via
`tests/e2e/build-v14-snapshot.sh`).
- Failure surfacing (#370): a push:main failure opens an
`attn:e2e-failure` issue; a PR failure adds the `attn:e2e-failure` label.
Both surface in `aelf-scan §1`.
- **Land here when:** you are catching a regression class that crossed a
module *seam* and was missed by unit/integration tests, or that depends
on how `aelf` is installed.

### Class of bug each E2E scenario catches

The seed scenarios are deliberately picked to cover the regression classes
that unit tests structurally cannot catch:

| Scenario | File | Class caught |
|---|---|---|
| install → onboard → search | `test_install_onboard_search.py` | install-time wiring; fixture-project loadable |
| hook → inject roundtrip | `test_hook_inject_roundtrip.py` | hook ↔ ingest ↔ rebuild ↔ injection seam |
| source-kind discrimination | `test_source_type_discrimination.py` | the #190 R1 class — constant defined in module A, never recorded by path B |
| v1.4 → current migration | `test_migration_v14_to_current.py` | migration regressions on real DB shapes |

Scenarios 4 (lock-survives-reopen) and 6 (`aelf upgrade-advice`) are listed in
#334 as additive follow-ups, not part of the issue-close gate.

## Choosing a layer — quick decision

```
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

two or more modules, but stays in-process?
→ Layer 2 (integration), in tests/.

Otherwise — single module, mocks acceptable?
→ Layer 1 (unit), in tests/.
```
Comment on lines +77 to +88

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


When in doubt, prefer the cheapest layer that can deterministically catch
the regression you have in mind. Cost climbs sharply: unit (~ms), integration
(~10ms–1s), E2E (~30s–8min).

## Bench / regression gates

- E2E wall time: ≤ 8 min p95 over the first 10 main-branch runs (per #334).
Hard cap is the workflow's `timeout-minutes: 8`.
- Flake budget: zero. A flake is quarantined within one business day and
tagged `attn:e2e-flake`.
- Unit and integration runtime: untouched by E2E work — the install-matrix
cost is paid only on push:main and labeled PRs.

## Out of scope

- Cross-OS matrix (macOS / Windows). Linux-only for v1.
- UI / frontend testing.
- Mutation testing (#325 bundle workflow handles this separately).
- Performance / benchmark regression gates (`bench-gated` v2.0 work).
Loading