aglinxinyuan opened a new pull request, #8042: URL: https://github.com/apache/texera/pull/8042
### What changes were proposed in this PR? `test_main_loop.py` goes from 34 tests to 44, covering the console-message RPC path, the end-channel completion rules, the per-element exception backstop, and the ECM/loop-counter forwarding paths. | Metric | Before | After | |---|---|---| | Codecov fully-covered lines **today** | 270/285 | **284/285** | | Codecov fully-covered lines **after #8040** | 259/285 | **280/285** | | Branch arms newly taken | — | **+11** | **+14 fully-covered lines move on Codecov today; the full +21 only shows up once #8040 lands.** Lines 176, 397, 658, 661, 706, 714 and 837 are line-hit with one arm untaken, so today's branch-less report already scores them covered — closing their second arm is real and mutation-proven but invisible until branch coverage is on. I would rather state both figures than quote the larger one and have it not materialise. The `[tool.coverage.run] branch = true` setting is deliberately **not** added here; it belongs to #8040 and adding it would conflict. Newly fully covered: 176, 177, 397, 400, 658, 661, 662, 706, 714, 715, 718, 719, 725, 813, 814, 817, 837, 838, 839, 849, 850. Nothing lost. Line 795 remains uncovered and four lines (349, 650, 740, 794) remain branch-partial. ### What the reviewers found Two adversarial reviewers reported eight surviving mutants against the first draft. Every one was reproduced before being fixed — seven gave a clean `43 passed` on the delivered spec and one produced no summary at all. **Eight of the claimed lines were covered vacuously.** The ones worth naming: - **The console-message test blocked on `output_queue.get()` instead of failing** — a hang, not an assertion failure, which is the worst way for a test to be wrong. - **The two-input-port end-channel test could not distinguish the port lookup from a constant**, so the rule it exists to pin was unconstrained. - **The no-output-ports test could not see an `EndChannel` broadcast at all.** - **`assert switched == [True]` claimed a statement *order* it cannot observe.** - A `emitted_counter == 0` assertion was a degenerate literal, and a `reset_calls == []` assertion was call-graph-guaranteed to hold. All eight now die. **33 mutations applied one at a time, 32 killed, 1 judged equivalent** (a flush/pause order exchange, with the reasoning recorded). Four of the 32 kills are honest about their conditions: the line-706, line-661, line-837 negations and the 393–394 order swap die only when the run is scoped to a single test, because they make **pre-existing** thread-driven tests block, and pytest-timeout's Windows `thread` method then kills the session before the killing test runs. They are stated as scoped kills rather than credited as clean whole-file kills. One further mutant was discarded rather than reported as surviving: negating line 661 makes the pre-existing `test_main_loop_thread_can_process_messages` block forever, yielding no per-test signal. The same statement is covered by a `return`→`pass` mutant that dies exclusively against a new test, so nothing is left unproven. ### A defect found and deliberately not pinned `_process_data_element`'s per-element backstop (813–814) is `except Exception as err: logger.exception(err)` — it logs and continues **without calling `context.report_exception`**. A runtime failure on one element therefore never reaches the coordinator, and the workflow can report SUCCESS on a short result. The new test asserts only that iteration continues and that nothing propagates. It does **not** assert that nothing is reported, because that half is arguably a silent-wrong-results bug and pinning it would cement it. The test carries a comment saying so; anyone strengthening it should keep that boundary. ### Verification Measured with the full CI-shaped suite from `amber/`, `--cov-branch` passed explicitly on both sides (this branch is on `main` and so lacks #8040's config), `LOGURU_LEVEL=WARNING`, per-line `hits` and `condition-coverage` read out of `coverage.xml` by script rather than from the summary percentage. The before-state was obtained by copying the original spec back from a scratch snapshot — never via `git restore` — with the production tree verified pristine throughout. **No regression.** Full pyamber suite: `5 failed, 1169 passed, 7 errors` against a baseline of `5 failed, 1159 passed, 7 errors`. The `FAILED`/`ERROR` node-id sets were extracted from both runs, sorted and diffed: **identical by identity**, 12 entries (7 `test_iceberg_document` errors, 2 iceberg failures, `test_tuple::test_hash`, 2 `test_expression_evaluator` repr-formatting failures). +10 passing. The scoped spec passes `44 passed` on five consecutive runs. `ruff check` and `ruff format --check` pass on CI's exact scope (`src/main/python src/test/python`). `git diff --numstat` shows `666 0` — 666 added, **zero deleted**. **A flake seen once and reported rather than buried:** one no-branch after-run came back 282/285 with lines 324–325 at zero hits; three consecutive re-runs all gave 284/285 with both covered. Those two lines are in `_check_and_process_control`'s while body, reached from a daemon thread, so a scheduling-dependent miss is plausible. The published figure is the reproducible one. ### On overlap with open PRs This file is not untouched work. **#7624** (open) and **#7388** (draft) both modify `main_loop.py` *and* `test_main_loop.py`. I checked both diffs: none of the 21 lines here falls inside either PR's edited hunks, and the new tests are appended at the end of `TestMainLoop`, away from the test-file hunks. The collision risk is a routine rebase rather than a semantic conflict — but it is not zero, and the earlier claim that this target was free was wrong. ### Deliberately not included Line 795 and the four remaining partials (349, 650, 740, 794) are left. Reaching 813/814 already required monkeypatching a `MainLoop` private — established practice in this file but white-box, and flagged as the one such test in the bundle. No production file is touched; `git diff -- 'amber/src/main/*'` is empty, checked after every mutation revert and as the final action. ### Any related issues, documentation, discussions? Closes #8041 ### How was this PR tested? ``` cd amber && python -m pytest -m "not integration" -q src/test/python/core/runnables/test_main_loop.py ``` ``` 44 passed, 1 warning ``` ``` cd amber && ruff check src/main/python src/test/python && ruff format --check src/main/python src/test/python ``` `amber/junit.xml`, `amber/coverage.xml` and `amber/.coverage` are regenerated by every run and are not committed. ### 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]
