Skip to content

fix(ci): run the real Python test suite (3 tests → 182) - #195

Merged
beinan merged 3 commits into
lance-format:mainfrom
beinan:fix/ci-runs-real-test-suite
Jul 25, 2026
Merged

fix(ci): run the real Python test suite (3 tests → 182)#195
beinan merged 3 commits into
lance-format:mainfrom
beinan:fix/ci-runs-real-test-suite

Conversation

@beinan

@beinan beinan commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

The problem

python-test.yml ran pytest python/tests/ with working-directory: python, which resolves to python/python/tests/ — a single stale file with 3 tests. The real suite, python/tests/ (24 files, 179 tests), was never collected. pytest exits 0 on a valid-but-nearly-empty directory, so this failed silently.

Git history dates the split: python/python/tests/ last changed in #59; python/tests/ is current through #158. ~100 PRs merged ungated — rollout store, async wrappers, export, ingestion.

Two further layers of silencing, each sufficient on its own:

  • Only bare pytest was installed, never the [tests] extra — so pytest-asyncio was absent.
  • No asyncio_mode anywhere, so the 12 async def tests were collected, never awaited, and passed without executing a line.

The same path bug appeared three more times: the doctest step was a permanent no-op (an if on a path that never exists, plus || echo), and ruff format --check python/ / ruff check python/ never linted the 24 test files. .codex/.../run_ci_checks.sh pointed at a rust/ directory that does not exist, so set -e killed it before any Python check ran.

CI changes

  • Pin collection with testpaths = ["tests"] so the ambiguous relative path cannot resurface.
  • asyncio_mode = "strict" + --strict-markers --strict-config — a mistyped marker now fails instead of silently disarming a test.
  • Install [lance-python,tests]; lint . instead of python/; make the doctest step real.
  • Point the helper script at the real crates/ layout.

Turning the suite on surfaced 19 real failures

1. Embedded RolloutStore silently discarded every write. This is the significant one. #181 made add durable-but-not-visible, with visibility driven by a server-side sweeper. The embedded path has no sweeper, and flush() was never exposed to Python — so:

s = RolloutStore.open(d)
s.add({'id':'a','rollout_id':'t1'})   # -> {'version': 2, 'ids': ['a'], 'count': 1}
s.list()                              # -> []   ... and still [] after reopen

Data acknowledged, then unreadable forever. This PR exposes flush() through the facade, PyO3, and both sync and async Python wrappers.

2. test_search.py's DummyInner fake had drifted from the real binding signature (missing include_binary/include_embedding), so all 55 tests raised TypeError on contact with current code.

3. Remote rollout tests asserted read-your-write against a server whose default flush interval is 30s. They now set ROLLOUT_FLUSH_INTERVAL_SECS=1 and poll — asserting the row arrives rather than pinning to the interval.

The 3 stale tests are moved into python/tests/ rather than dropped (they cover snapshot/fork/memory:// and still pass), and the orphaned directory is removed so nothing can silently collect it again.

Verification

  • 182 passed, 2 skipped (179 real + 3 rescued).
  • ruff format --check ., ruff check ., pyright — all clean.
  • cargo fmt --all -- --check and clippy clean on the changed crates.
  • Confirmed the async tests now genuinely execute by injecting a failing assertion and watching it fail — it would not have before this change.

Note on scope

This is deliberately scoped to making CI honest plus the failures that gating surfaced. It does not address: publish workflows having no needs: on any test job (a release cut from a red main ships to PyPI/crates.io), cargo test covering only 2 of 8 crates and --lib only, or the DummyInner fake asserting hardcoded literals so RRF ranking is still never exercised. Happy to file those separately.

🤖 Generated with Claude Code

beinan and others added 2 commits July 25, 2026 08:50
`python-test.yml` ran `pytest python/tests/` with `working-directory: python`,
which resolves to `python/python/tests/` -- a single stale file with 3 tests.
The real suite, `python/tests/` (24 files, 179 tests), was never collected.
pytest exits 0 on a valid-but-nearly-empty directory, so this failed silently.

Git history dates the split: `python/python/tests/` last changed in lance-format#59, while
`python/tests/` is current through lance-format#158. Roughly 100 PRs merged ungated,
including the rollout store, async wrappers, export, and ingestion.

Two further layers of silencing, each sufficient alone:
  - only bare `pytest` was installed, never the `[tests]` extra, so
    `pytest-asyncio` was absent;
  - no `asyncio_mode` anywhere, so the 12 `async def` tests were collected,
    never awaited, and passed without executing a line.

The same path bug appeared three more times: the doctest step was a permanent
no-op (guarded by an `if` on a path that never exists, plus `|| echo`), and
`ruff format --check python/` / `ruff check python/` never linted the 24 test
files. `.codex/.../run_ci_checks.sh` pointed at a `rust/` directory that does
not exist, so `set -e` killed it before any Python check ran.

CI changes:
  - pin collection with `testpaths = ["tests"]` in `[tool.pytest.ini_options]`
    so the ambiguous relative path cannot resurface;
  - `asyncio_mode = "strict"` + `--strict-markers --strict-config` so a
    mistyped marker fails instead of silently disarming a test;
  - install `[lance-python,tests]`; lint `.` instead of `python/`;
  - point the helper script at the real `crates/` layout, matching CI.

Turning the suite on surfaced 19 real failures, fixed here:

1. Embedded `RolloutStore` silently discarded every write. lance-format#181 made `add`
   durable-but-not-visible, with visibility driven by a server-side sweeper.
   The embedded path has no sweeper and `flush()` was never exposed to Python,
   so a write returned `{"count": 1}` and the row was unreadable -- even after
   reopen. Exposes `flush()` through the facade, PyO3, and both the sync and
   async Python wrappers.

2. `test_search.py`'s hand-written `DummyInner` had drifted from the real
   binding signature (missing `include_binary` / `include_embedding`), so all
   55 tests raised `TypeError` on contact with current code.

3. Remote rollout tests asserted read-your-write against a server whose default
   flush interval is 30s. They now set `ROLLOUT_FLUSH_INTERVAL_SECS=1` and poll,
   asserting the row *arrives* rather than pinning to the interval.

The 3 stale tests are moved into `python/tests/` rather than dropped -- they
cover `snapshot`/`fork`/`memory://` and still pass -- and the orphaned
directory is removed so nothing can silently collect it again.

Verified locally: 182 passed, 2 skipped; ruff format/check and pyright clean;
`cargo fmt --all --check` and clippy clean on the changed crates. Confirmed the
async tests now genuinely execute by injecting a failing assertion and watching
it fail, which it would not have done before this change.

Co-Authored-By: Claude <noreply@anthropic.com>
Enabling the real suite surfaced two genuine failures in `test_persistence.py`
that CI had never run. They are not caused by this PR and cannot be fixed here.

`Context.create` against a custom-endpoint S3 succeeds, but the first `add`
fails with "Bucket '...' not found". Reproduced locally against moto with
request logging: every request that reaches moto returns 200, and the failing
request never arrives at all -- the WAL writer is talking to a different
endpoint.

Root cause is upstream, in lance 7.0.0:

    // lance/src/dataset/mem_wal/api.rs:607
    let (store, base_path) = ObjectStore::from_uri(base_uri).await?;

`mem_wal_writer` constructs a fresh object store from the URI alone, discarding
the dataset's configured `storage_options`. `aws_endpoint_url` is therefore
dropped and the writer targets real AWS. `create` works because it goes through
the options-aware path; `add` is the first call that opens a shard writer.

There is no local workaround: `ShardWriterConfig` has no field to carry storage
options, so the crate cannot thread them in. The fix is upstream, switching to
the `from_uri_and_params` constructor that already exists in lance-io.

Marked `xfail(strict=True)` rather than skipped, so the tests keep running and
turn into a hard failure the moment a lance bump fixes this -- the marker
cannot silently rot.

Worth noting the blast radius beyond the tests: all three stores
(`store.rs:518`, `rollout_store.rs:632`, `datagen_store.rs:186`) open shard
writers this way, so writes are broken against any custom-endpoint S3 --
MinIO, Ceph, R2, or moto. Only AWS-proper works today. This was invisible
because the tests covering it were never executed.

Full suite with all extras installed: 194 passed, 2 skipped, 3 xfailed.

Co-Authored-By: Claude <noreply@anthropic.com>
@beinan

beinan commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

CI surfaced a real bug: MemWAL drops storage_options on custom-endpoint S3

The first CI run came back 190 passed, 2 failed — more tests than my local run, because CI installs the [tests] extra and therefore actually runs the moto-backed S3 tests. Both failures are genuine, pre-existing, and not caused by this PR.

Symptom

Context.create against a custom-endpoint S3 succeeds; the first add fails with Bucket '...' not found.

Diagnosis

Reproduced locally against moto with request logging. Every request that reaches moto returns 200, and the failing request never arrives at all — so the WAL writer is talking to a different endpoint than the dataset.

Root cause is upstream in lance 7.0.0:

// lance/src/dataset/mem_wal/api.rs:607
let (store, base_path) = ObjectStore::from_uri(base_uri).await?;

mem_wal_writer builds a fresh object store from the URI alone, discarding the dataset's configured storage_options. aws_endpoint_url is dropped, so the writer targets real AWS. create works because it goes through the options-aware path; add is the first call that opens a shard writer.

No local workaround exists — ShardWriterConfig has no field to carry storage options. The fix is upstream: use from_uri_and_params, which already exists in lance-io.

Blast radius beyond the tests

All three stores open shard writers this way — store.rs:518, rollout_store.rs:632, datagen_store.rs:186. Writes are broken against any custom-endpoint S3: MinIO, Ceph, R2, moto. Only AWS-proper works today. This was invisible precisely because the tests covering it were never executed.

What I did

Marked both xfail(strict=True) — not skipped. They keep running, and flip to a hard failure the moment a lance bump fixes this, so the marker cannot silently rot. The reason string carries the full diagnosis and the upstream file:line.

Full suite with all extras: 194 passed, 2 skipped, 3 xfailed.

This seems worth an upstream issue against lance; happy to file it if you agree with the read.

The previous commit turned the doctest step from a permanent no-op into a real
check, which immediately exposed that the invocation itself was wrong:

    ImportError: attempted relative import with no known parent package

`python -m doctest <path>` imports the target by filename, so the
package-relative `from .api import ...` in `__init__.py` has no parent package
and fails before any doctest runs. This is why the original step could only
ever "pass" while wrapped in `|| echo "No doctests found"`.

Import the installed module instead and propagate the result, so the step
fails on a failing doctest rather than on its own invocation.

Note the module currently reports `attempted=0` -- there are genuinely no
doctests today. The step is now wired correctly so that any doctest added
later is actually executed and enforced.

The pytest step in the same job passed: 190 passed, 6 skipped, 3 xfailed.

Co-Authored-By: Claude <noreply@anthropic.com>
@beinan
beinan merged commit 42dac5d into lance-format:main Jul 25, 2026
9 checks passed
beinan added a commit that referenced this pull request Jul 26, 2026
```diff
-  cargo test -p lance-context-core -p lance-context-master --lib
+  cargo test --workspace --all-targets
```

`--lib` runs **unit tests only**. The preceding `--no-run` step compiled
the `crates/lance-context-core/tests/*.rs` integration tests and then
**discarded them** — they never executed.

## Evidence

The CI log from **#199**:

```
test result: ok. 167 passed   ← core unit tests
test result: ok. 14 passed    ← master unit tests
test result: ok. 1 passed     ← the one etcd test named explicitly
```

No line for the **5 WAL-merge concurrency tests that PR added**. They
merged into `main` having never run in CI — as had
`wal_merge_generation_cleanup.rs` before them.

`--workspace` additionally covers `lance-context-api`, `-server`,
`-client`, `-metrics` and the facade crate, **none of which were tested
at all**.

## Impact

**250 tests pass** under the new command, against **182** under `--lib`.
Runtime ~7 min against the existing 30 min timeout.

Same class of failure as #195 (Python CI collecting the wrong
directory), in a different mechanism: tests that exist, compile, and are
silently not run. I flagged `--lib` in an earlier review and then failed
to re-check it when adding integration tests in #199 — a test that never
executes is the same as no test.

## Note

This PR is **only** the CI change, deliberately. It was originally
bundled with a `ContextStore` concurrency fix in #201; on review that
fix addresses a scenario that is not currently reachable in
single-process deployments, so it should be argued on its own merits
rather than riding along with an unambiguous CI repair. #201 will be
rescoped.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant