Add live-server integration coverage for the #40/#41 query-helper relaxations

[craigmcchesney]

Closes the gap left by #47: both relaxations were verified by reading the dp-service source, but shipped with unit tests only.

Why unit tests were not enough

A unit test asserts the request the client builds — values == [] for #40, criteria == [] for #41. That is exactly the assertion that cannot distinguish a working server from a broken one:

• Criterion helpers reject the key-only attributes search the protos document #40: an attributes criterion with no values must become Filters.exists("attributes.<key>"). The plausible failure is an $in: [], which matches nothing while producing a request that looks identical on the wire.
• Make criteria optional on the annotation-service query/iter methods (empty list now matches all) #41: an omitted criteria list must be match-all. The plausible failure is a business error.

What's covered

Four tests, ten subtests:

Path	#40 key-only	#41 browse-all
PV metadata	✅	✅
Configurations	✅	✅
Configuration activations	✅	✅
v2 PvQuery.attr selector	✅	n/a — deliberately out of scope

Each test asserts a pair: the key-only query must find the record, and a query naming a value the record does not have must not. Alone, the first assertion cannot tell an existence filter from a match-all — which is the whole thing under test.

Verified non-vacuous

I did not assume a passing test proves anything. I temporarily reintroduced the old behavior (empty values sent as a non-matching $in) and confirmed the tests fail with the empty-result symptom:

AssertionError: 'ITEST:RELAX:PV:...' not found in [] :
  key-only search (values empty list) must match on key existence alone

Then reverted and re-ran green.

Mechanics

The v2 class ingests its own samples — a query selector needs archived data to select — and polls for bucket visibility rather than sleeping, since ingestData() acks before the bucket is queryable. It goes through the generated stub because IngestionClient wraps only registerProvider() until #17, matching test_datasets_annotations_integration.py.

Every record is namespaced by a per-run id and removed via addCleanup, so repeat runs do not accumulate state and a mid-test failure still tears down; I confirmed the annotation catalogue is empty again after a run. Tests self-skip with an actionable message when the services are absent.

No CI impact: -m "not integration" deselects all 47 integration tests. 771 pass with the ecosystem up (725 unit + 46 integration); 725 pass and 47 deselect without it. Ruff clean, 107 cookbook snippets checked.

Both plans get a dated note recording the added coverage, per plan/README.md's rule that merged plans take correction notes rather than rewrites.

Relates to #40, #41.

🤖 Generated with Claude Code

https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

[Copilot]

🟡 Changes recommended

Multiple unresolved test reliability, isolation, cleanup, and error-handling issues remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds live-server integration coverage for the #40/#41 query-helper relaxations.

Changes:

• Tests key-only searches and browse-all behavior.
• Adds v2 selector ingestion and visibility polling.
• Updates ticket plans and CLAUDE.md.

File summaries

File	Summary
tests/integration/test_query_helper_relaxations_integration.py	Adds integration tests and fixtures; requires fixes for service checks, isolation, cleanup, setup failures, catalogue handling, and short-circuiting.
plan/tickets/41/plan.md	Records #41 integration coverage.
plan/tickets/40/plan.md	Records #40 integration coverage.
CLAUDE.md	Documents the integration coverage.

Review details

Suppressed comments (8)

tests/integration/test_query_helper_relaxations_integration.py:318

• This second-level run id has the same collision window for the ingested PV and provider. Concurrent test processes can select or clean up each other's data, so the v2 selector test is not isolated despite the surrounding comments; use a UUID or at least time.time_ns() here as well.

        cls.run_id = int(time.time())

tests/integration/test_query_helper_relaxations_integration.py:335

• unittest does not call tearDownClass when setUpClass raises, including the SkipTest paths in _ingest_samples() and _catalogue_pv(). Therefore a setup failure after opening the ingestion channel can leave the channel and test-side state behind; register addClassCleanup as each resource is created instead of relying only on this teardown method.

    def tearDownClass(cls):
        # Guarded: an early skip can leave these unset.
        if getattr(cls, "client", None) is not None:
            cls.client.annotation.pv_metadata.delete_pv_metadata(cls.pv_name)
        if getattr(cls, "_ingestion_channel", None) is not None:
            cls._ingestion_channel.close()

tests/integration/test_query_helper_relaxations_integration.py:348

• The service-reachability check has already passed, so a registration business error is a test setup failure, not an unavailable-service condition. Raising SkipTest here skips the entire class and can make broken ingestion coverage appear green; reserve skips for a known unsupported backend and fail the setup instead.

        if registration.HasField("exceptionalResult"):
            raise unittest.SkipTest(
                f"could not register an ingestion provider: {registration.exceptionalResult.message}"

tests/integration/test_query_helper_relaxations_integration.py:367

• At this point the ingestion service was reachable, so an ingestData business error indicates that the fixture or server is broken. Converting it to SkipTest silently removes the v2 selector test from the run instead of reporting a failure; this should fail setup rather than skip.

        response = stub.ingestData(request, timeout=15)
        if response.HasField("exceptionalResult"):
            raise unittest.SkipTest(f"could not ingest test data: {response.exceptionalResult.message}")

tests/integration/test_query_helper_relaxations_integration.py:381

• A reachable annotation service returning an error from save_pv_metadata is not an absent-service condition. Skipping the class here can hide a real catalogue/API regression and report no v2 coverage; treat this fixture setup failure as a test failure, reserving SkipTest for a deliberately detected incompatible backend.

        if result.result_status.is_error:
            raise unittest.SkipTest(f"could not catalogue the test PV: {result.result_status.message}")

tests/integration/test_query_helper_relaxations_integration.py:156

• When the catalogue exceeds 5,001 records, this path calls skipTest instead of checking whether the saved PV is reachable. That makes the key #41 integration assertion disappear in larger environments, so a browse-all regression can go unnoticed; iterate until the unique record is found and fail if it is absent rather than skipping.

            if count >= 5000:
                self.skipTest("catalogue too large to confirm browse-all reachability within the scan bound")

tests/integration/test_query_helper_relaxations_integration.py:213

• This materializes every configuration returned by the unfiltered iterator just to test for one run-unique record. On a large live catalogue it needlessly increases runtime and memory; use a short-circuiting any(...) or an explicit loop that stops once the target is found.

        self.assertIn(
            config_name,
            [c.configurationName for c in mc.iter_configurations()],
            "iter_configurations() with no criteria should reach the saved configuration",
        )

tests/integration/test_query_helper_relaxations_integration.py:292

• This also materializes the complete unfiltered activation catalogue even though the test needs only the run-unique activation id. A short-circuiting any(...) or early-break loop avoids unnecessary runtime and memory on a populated live service.

        self.assertIn(
            activation_id,
            [a.clientActivationId for a in mc.iter_configuration_activations()],
            "iter_configuration_activations() with no criteria should reach the saved activation",
        )

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.