aglinxinyuan opened a new pull request, #8048:
URL: https://github.com/apache/texera/pull/8048
### What changes were proposed in this PR?
A new `TestIcebergDocumentWithMockCatalog` class in
`test_iceberg_document.py`, taking the file from 9 collected items to 22.
| Metric | Before | After |
|---|---|---|
| Codecov fully-covered lines **today** | 118/124 | **124/124** |
| Codecov fully-covered lines **after #8040** | 115/124 | **124/124** |
| Branch arms | 21/26 | **26/26** |
**`iceberg_document.py` reaches 100% on both lines and branches.** +6
fully-covered lines today, +9 after #8040 — lines 90, 108 and 189 are `50%
(1/2)` partials, invisible to the current branch-less report.
The nine lines were exactly {79, 80, 81, 84, 90, 108, 109, 189, 190}:
`get_uri`, `get_count`, `clear`, and the `get_after` skip guard.
### The measurement problem, and how it was handled
The existing `TestIcebergDocument` needs a live postgres Iceberg catalog. CI
supplies one; on this machine the `texera` role cannot read `iceberg_tables`,
so **all 9 of those tests fail locally** and a raw local coverage run
under-reports the file badly — enough to suggest ~50 winnable lines instead of
9.
A throwaway sqlite-backed `SqlCatalog` proxy was used to reproduce the same
code paths, applied identically to both sides and then deleted. The honest gap
is 9.
**The new tests need none of that.** They patch
`IcebergCatalogInstance.get_instance` rather than mutating it, so they touch no
process-global state and need no postgres, filesystem or network. They run in
1.25s.
### Two hazards found in the existing fixture
- **`StorageConfig.initialize` is a process-wide one-shot** that raises on a
second call, and `test_iceberg_document.py:45` calls it **unguarded at module
level**. That works only because this module happens to be the first
module-level initializer imported — two sibling modules guard with `if not
StorageConfig._initialized`. A new test module initializing at module level and
sorting before it would break collection for the whole suite. Appending a class
to the existing file avoids this; a separate file would have been a trap.
- **`IcebergCatalogInstance._catalogs` is a class-level dict** and
`replace_instance` mutates it for the rest of the session, poisoning every
later suite in the shared process. The new tests sidestep it by patching
`get_instance`.
### Honest discounts
- **`get_uri` and `get_count` have zero callers anywhere in
`src/main/python`**, and `clear` has none either — the Scala side has its own
implementation. These are real statements with real logic, not
`@abstractmethod` stubs, but the value is interface-contract conformance rather
than live behaviour.
- **Lines 189–190 need a negative `from_index`.** `num_of_skipped_records`
is provably 0 when the guard first runs, so the only route is `get_after(-1)`.
It is drivable through the public API, but no production caller produces a
negative offset — a defensive-guard test. Counted strictly, the bundle is 7
lines and 4 arms.
- **One mutant survives this class**: `sum(f.file.record_count ...)` to
`sum(1 ...)` at line 110, killed only by the pre-existing postgres-gated
`test_get_counts`. Line 110 is already covered in CI so it is not a coverage
gap, but the kill is not this bundle's and one was not manufactured.
### Verification
Two reviewers returned seven findings; all repaired. **The repair pass added
zero coverage lines and zero arms** — the builder's bundle already reached
124/124. Its entire value was mutation strength: 14 mutants that passed the
delivered spec now fail. Stated plainly rather than sold as more coverage.
Full pyamber suite: `5 failed, 1172 passed, 7 errors` against main's `1159
passed`. Failure sets sorted and diffed — **identical by identity**, 12
entries. `ruff check` and `ruff format --check` pass on CI's scope. The
throwaway proxy is deleted and `git status --porcelain` is clean.
No production file is touched; `git diff -- 'amber/src/main/*'` is empty.
### Any related issues, documentation, discussions?
Closes #8044
### How was this PR tested?
```
cd amber && python -m pytest -m "not integration" -q
src/test/python/core/storage/iceberg/test_iceberg_document.py -k MockCatalog
```
```
13 passed, 1 warning in 1.25s
```
The 9 pre-existing `TestIcebergDocument` tests still require the postgres
catalog that CI provides.
### 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]
