aglinxinyuan opened a new pull request, #8040:
URL: https://github.com/apache/texera/pull/8040

   ### What changes were proposed in this PR?
   
   Enables branch coverage for pyamber. Two files, no production or test code:
   
   - **`amber/pyproject.toml`** — adds `[tool.coverage.run] branch = true`.
   - **`.github/workflows/build.yml`** — a comment on the pytest step recording 
that branch coverage is on and where it is configured. The `run:` command 
itself is unchanged.
   
   ### Why the config rather than `--cov-branch`
   
   `--cov-branch` on the CI command would work, but it would put CI and a 
developer's laptop on **different metrics**: a local `pytest --cov` would keep 
reporting statement-only coverage, so a half-taken `if` would read as covered 
locally and partial on Codecov. Setting it in `pyproject.toml` gives both the 
same metric from one switch.
   
   The comment in `build.yml` exists so the pipeline is not silent about the 
change, and it says explicitly not to add the flag back — two switches for one 
boolean invites someone to remove one and believe it is off.
   
   ### What this surfaces
   
   Measured on `97e3585a6a`, with `codecov.yml`'s ignore rules applied 
(`proto/**`, `test_*.py`):
   
   | | Before | After |
   |---|---|---|
   | Fully covered lines | 4352 | 4290 |
   | **Partial lines** | **0** | **62** |
   | Missed lines | 122 | 122 |
   | Branch arms (taken/total) | not measured | 814/894 |
   
   **62 partial lines and 80 untaken branch arms become visible.** No line's 
actual execution changed — the same 1159 tests run and the same statements 
execute. This only stops the report from rounding a half-taken branch up to 
"covered".
   
   ### The reported number will go down, and that is the point
   
   **pyamber's flag figure drops 97.27% → 95.89%, about 1.4 points.** That is 
expected: partial lines stop counting as hits.
   
   `codecov.yml` sets `coverage.status.project` to `target: auto, threshold: 
1%`, so it is worth being precise about whether this trips the check. It does 
not:
   
   - The project status is computed **repo-wide**, and `flag_management` in 
`codecov.yml` sets only `carryforward` — no per-flag statuses are configured, 
so pyamber's own 1.4-point drop is not itself gated.
   - Repo-wide, 62 lines out of 47,713 moves coverage **93.32% → 93.19%, a 
0.13-point drop** — comfortably inside the 1% slack.
   - The `patch` status is unaffected: this PR changes no coverable lines.
   
   If a per-flag pyamber status is ever added, it should be added after this 
lands so its baseline is the branch-aware figure.
   
   ### Verification
   
   Run locally on Python 3.12.10 with coverage 7.13.5 and pytest 7.4.0 — the 
same coverage version CI resolves.
   
   - **The config alone does the job.** Ran the CI command **verbatim, without 
any `--cov-branch` flag**, and `coverage.xml` came back with 
`branch-rate="0.9105"` and 447 lines carrying `condition-coverage` data. Before 
the change the same command gives `branch-rate="0"` and zero such lines. 
coverage.py finds `amber/pyproject.toml` because CI does `cd amber` first, 
which is also how the repo documents running pytest.
   - **No behavioural change.** 1159 passed on both sides. The pre-existing 
local failures (5 failed, 7 errors — Iceberg/Windows environment, present on 
`main`) are **identical in identity**, not merely in count: the two 
`FAILED`/`ERROR` sets diff clean at 12 entries.
   - **`build.yml` still parses** as YAML (9 jobs) and the pyamber step's 
`run:` command is byte-identical to `main`.
   - **`amber/pyproject.toml` still parses** via `tomllib`, with `[tool.ruff]` 
and `[tool.pytest.ini_options]` intact.
   - **Lint clean** using CI's exact scoping — `ruff check src/main/python 
src/test/python && ruff format --check src/main/python src/test/python` → `All 
checks passed!` / `211 files already formatted`.
   
   One note in case someone reproduces this: running `ruff format --check .` 
from `amber/` reports two files needing reformatting, both `.py` files living 
under `src/main/scala/.../aiassistant/`. They are pre-existing (identical 
output with this change reverted) and outside CI's ruff scope, so they are 
unrelated to this PR — mentioned only so the broader command's output is not 
mistaken for a regression.
   
   ### Any related issues, documentation, discussions?
   
   Closes #8039
   
   ### How was this PR tested?
   
   ```
   cd amber && pytest -m "not integration" --cov=src/main/python 
--cov-report=xml -q
   ```
   
   ```
   1159 passed, 1 deselected, 1 xfailed  (plus the pre-existing local 
Iceberg/Windows failures, unchanged from main)
   branch-rate = 0.9105   (was 0.0)
   ```
   
   CI's own lint scope:
   
   ```
   cd amber && ruff check src/main/python src/test/python && ruff format 
--check src/main/python src/test/python
   ```
   
   ### Was this PR authored or co-authored using generative AI tooling?
   
   Generated-by: Claude Code (Opus 5)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to