From fda61cf5edc2d499ba3c26ebf24a8e2f829fc939 Mon Sep 17 00:00:00 2001 From: Craig McChesney Date: Thu, 10 Sep 2026 15:57:37 -0600 Subject: [PATCH 1/2] test: add live-server coverage for the #40/#41 relaxations PR #47 verified both server behaviors by reading the dp-service source, but shipped unit tests only. A unit test asserts the request the client builds -- `values == []`, `criteria == []` -- which is precisely the assertion that cannot tell a working server from a broken one: - #40: an attributes criterion with no values must become Filters.exists("attributes."). The plausible failure is an `$in: []`, which matches NOTHING while producing a request that looks identical on the wire. - #41: an omitted criteria list must be match-all. The plausible failure is a business error. Four tests, ten subtests, covering PV metadata, configurations, activations, and the v2 PvQuery.attr selector -- every helper #40 touched that a live server can reach, plus #41's browse-all on all three annotation families. The load-bearing shape is a PAIR of assertions: the key-only query must find the record, AND a query naming a value the record does not have must not. Alone, the first cannot distinguish an existence filter from a match-all, which is the whole thing being tested. Verified non-vacuous by temporarily reintroducing the old behavior and confirming the tests fail with the empty-result symptom, not by assuming a passing test proves anything. The v2 class ingests its own samples, since a query selector needs archived data to select, and polls for bucket visibility rather than sleeping a fixed interval -- 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; the annotation catalogue is empty again after a run. Tests self-skip with an actionable message when the services are absent, and CI's `-m "not integration"` deselects all 47 integration tests, so this cannot affect the CI signal. 771 pass with the ecosystem up (725 unit + 46 integration); 725 pass and 47 deselect without it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn --- CLAUDE.md | 1 + plan/tickets/40/plan.md | 6 + plan/tickets/41/plan.md | 6 + ...st_query_helper_relaxations_integration.py | 431 ++++++++++++++++++ 4 files changed, 444 insertions(+) create mode 100644 tests/integration/test_query_helper_relaxations_integration.py diff --git a/CLAUDE.md b/CLAUDE.md index b6509a1..ace18a1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -175,6 +175,7 @@ plan documents one change, `CLAUDE.md` documents the invariant it established. - `tests/unit/test_export_client.py` - Unit tests for ExportClient (`ExportFormat` mapping and unreachable `UNSPECIFIED`, `calculations_spec()`, the zero-source rejection, three-tier error handling) - `tests/unit/test_annotation_client.py` - Unit tests pinning the `AnnotationClient` facade wiring (every feature client present, one shared channel, one stub apiece) - `tests/integration/test_datasets_annotations_integration.py` - Live-server round trip for datasets/annotations/calculations; ingests its own samples first, because `saveDataSet` requires archived PVs +- `tests/integration/test_query_helper_relaxations_integration.py` - Live-server coverage for the #40 key-only `attributes()` search and the #41 browse-all `criteria`, on PV metadata, configurations, activations, and the v2 `PvQuery.attr` selector. Both rest on server behavior a unit test cannot reach: a unit test asserts the request carries `values == []`, but only a real server distinguishes an existence filter from an `$in: []` that matches nothing. Each test therefore stores an attribute value, asserts the key-only form finds the record, and asserts a query for a *different* value does not -- that pairing is what makes the first assertion meaningful. The v2 class ingests its own samples (the selector needs archived data) and polls for bucket visibility rather than sleeping - `tests/unit/test_time_conversions.py` - Unit tests for the shared time converters (`to_timestamp()` input forms and the naive-datetime/bool/unsupported-type rejections; `to_epoch_nanos()` exactness and its round trip with `to_timestamp()`) - `tests/unit/test_data_frame.py` - Unit tests for the data_frame builders (axis relocation, each typed column, `data_column()` bool-before-int and unset-oneof handling, provenance helpers, and every `data_frame()` shape rule incl. array dims and serialized-column name-only checks) - `tests/unit/test_data_frame_conversions.py` - Unit tests for data_frame_conversions (nanosecond-exact expansion, per-column conversion incl. array reshaping, duplicate-name fail-loud, and — skipping cleanly without `[analysis]` — the pandas round trip, dtype mapping, and NaN fail-loud) diff --git a/plan/tickets/40/plan.md b/plan/tickets/40/plan.md index 698c6c2..9767654 100644 --- a/plan/tickets/40/plan.md +++ b/plan/tickets/40/plan.md @@ -10,6 +10,12 @@ same upstream commits `plan/tickets/6/plan.md` was verified against). Triage verified every premise in the ticket body against the protos, the server source, and this repo's own history; three corrections to the ticket are recorded below and folded into [Implementation tasks](#implementation-tasks). +- **Integration coverage added 2026-09-10**, after PR #47 merged: this plan verified server behavior by + *reading* dp-service, and `tests/integration/test_query_helper_relaxations_integration.py` now asserts it + against a live ecosystem. The load-bearing shape is a pair — the key-only query must find a record, and a + query for a value that record does *not* have must not. Alone, the first assertion cannot distinguish a + working existence filter from a match-all. Confirmed non-vacuous by temporarily reintroducing the old + behavior and watching the test fail. ## Overview diff --git a/plan/tickets/41/plan.md b/plan/tickets/41/plan.md index 3bec7fb..2e7645d 100644 --- a/plan/tickets/41/plan.md +++ b/plan/tickets/41/plan.md @@ -8,6 +8,12 @@ - **Status**: written 2026-09-10 against dp-python-lib `e7a77db` (the #40 commit), dp-grpc `6dfff3f`, dp-service `fddf692`. Every premise verified against the protos and the server source; two attribution errors in the ticket body are corrected below. +- **Integration coverage added 2026-09-10**, after PR #47 merged: this plan verified server behavior by + *reading* dp-service, and `tests/integration/test_query_helper_relaxations_integration.py` now asserts it + against a live ecosystem. For this ticket that is the browse-all form on all three families — an omitted + criteria list is accepted rather than rejected (T2), and `iter_*` with no arguments reaches a record the + test just saved. A unit test can only assert the request carries an empty `criteria` list; whether the + server treats that as match-all or as a business error is exactly what needed a live server. ## Overview diff --git a/tests/integration/test_query_helper_relaxations_integration.py b/tests/integration/test_query_helper_relaxations_integration.py new file mode 100644 index 0000000..8b6a02e --- /dev/null +++ b/tests/integration/test_query_helper_relaxations_integration.py @@ -0,0 +1,431 @@ +""" +Integration coverage for the issue #40 / #41 relaxations, against a live MLDP ecosystem. + +Both changes rest on a *server* behavior that unit tests cannot reach -- they assert only the shape of the request +the client builds, not that the server honors it: + + - #40 (`plan/tickets/40/plan.md`): an `attributesCriterion` carrying a key and no values is a key-only existence + search. Server side that is `Filters.exists("attributes.")`; the failure mode this pins is the plausible + alternative, an `$in: []` that matches NOTHING. A unit test asserting `values == []` cannot tell those apart. + - #41 (`plan/tickets/41/plan.md`): an omitted criteria list matches ALL records rather than being rejected. The + failure mode is a business error, which again only a real server can produce. + +So each test here saves a record whose attribute value would NOT match any value-based query, then asserts the +key-only form finds it anyway. That is the assertion that distinguishes a working existence search from an `$in` +against an empty list. + +Prerequisites: +- MLDP services running (annotation service at localhost:50053; TestV2SelectorRelaxations also needs ingestion at + localhost:50051). Tests self-skip when they are absent. +- Run with: pytest tests/integration/test_query_helper_relaxations_integration.py -v + +Every record is namespaced with a per-run id and removed via addCleanup, so repeated runs do not accumulate state +and a mid-test failure still tears down. +""" + +import logging +import os +import sys +import time +import unittest +from datetime import datetime, timedelta, timezone + +import grpc + +# Add src directory to path for imports +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../src")) + +from dp_python_lib.client.machine_config_client import ( + ConfigurationActivationQuery, + ConfigurationQuery, + SaveConfigurationActivationRequestParams, + SaveConfigurationRequestParams, +) +from dp_python_lib.client.mldp_client import MldpClient +from dp_python_lib.client.pv_metadata_client import PvMetadataQuery, SavePvMetadataRequestParams +from dp_python_lib.client.query_client import PvQuery, QueryParams +from dp_python_lib.grpc import ingestion_pb2, ingestion_pb2_grpc + +ANNOTATION_ADDRESS = "localhost:50053" +INGESTION_ADDRESS = "localhost:50051" + +# The value stored on every probe record. Key-only searches must find these WITHOUT naming the value; the +# NON_MATCHING_VALUE below is what a value-based query is asked for instead, to prove the two differ. +STORED_VALUE = "alpha" +NON_MATCHING_VALUE = "not-the-stored-value" + + +def _require_service(logger, label, address): + """Skips the calling test class unless `address` accepts a connection within 5s.""" + try: + channel = grpc.insecure_channel(address) + grpc.channel_ready_future(channel).result(timeout=5) + channel.close() + logger.info("%s service is reachable at %s", label, address) + except grpc.FutureTimeoutError: + raise unittest.SkipTest( + f"MLDP {label} service not available at {address}. Start the MLDP ecosystem before running " + "integration tests." + ) from None + except Exception as e: + raise unittest.SkipTest(f"Cannot connect to MLDP {label} service: {e}.") from None + + +class TestAnnotationServiceRelaxations(unittest.TestCase): + """ + Covers #40 and #41 on the three annotation-service query families: PV metadata, configurations, and + configuration activations. + """ + + @classmethod + def setUpClass(cls): + logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s") + cls.logger = logging.getLogger(__name__) + _require_service(cls.logger, "annotation", ANNOTATION_ADDRESS) + + cls.client = MldpClient() + cls.run_id = int(time.time()) + cls.logger.info("Query-helper relaxation integration run id: %s", cls.run_id) + + # ------------------------------------------------------------------ + # PV metadata + # ------------------------------------------------------------------ + + def test_pv_metadata_key_only_and_browse_all(self): + pv_client = self.client.annotation.pv_metadata + pv_name = f"ITEST:RELAX:PV:{self.run_id}" + # Per-test unique key, so a concurrent run's records cannot satisfy this test's assertions for it. + key = f"itest_pv_key_{self.run_id}" + + self.addCleanup(pv_client.delete_pv_metadata, pv_name) + save = pv_client.save_pv_metadata( + SavePvMetadataRequestParams( + pv_name=pv_name, + attributes={key: STORED_VALUE}, + modified_by="dp-python-lib-integration-test", + ) + ) + self.assertFalse(save.result_status.is_error, f"savePvMetadata failed: {save.result_status.message}") + + # --- #40: key-only existence search finds it --- + for label, criterion in ( + ("values omitted", PvMetadataQuery.attributes(key)), + ("values empty list", PvMetadataQuery.attributes(key, [])), + ): + with self.subTest(form=label): + result = pv_client.query_pv_metadata([criterion]) + self.assertFalse( + result.result_status.is_error, + f"key-only queryPvMetadata ({label}) failed: {result.result_status.message}", + ) + self.assertIn( + pv_name, + [record.pvName for record in result.pv_metadata_list], + f"key-only search ({label}) must match on key existence alone", + ) + + # The assertion that makes the one above meaningful: the same key with a value that does NOT match + # returns nothing, so the key-only hit came from existence rather than from matching everything. + negative = pv_client.query_pv_metadata([PvMetadataQuery.attributes(key, [NON_MATCHING_VALUE])]) + self.assertFalse(negative.result_status.is_error, negative.result_status.message) + self.assertNotIn( + pv_name, + [record.pvName for record in negative.pv_metadata_list], + "a value-based query for a value the record does not have must not match it", + ) + + # --- #41: omitted criteria is match-all, not a rejection --- + for label, page in ( + ("omitted", pv_client.query_pv_metadata()), + ("empty list", pv_client.query_pv_metadata([])), + ): + with self.subTest(form=label): + self.assertFalse( + page.result_status.is_error, + f"browse-all queryPvMetadata ({label}) must not be rejected: {page.result_status.message}", + ) + + # iter_* with no criteria is the documented browse-all form and must reach this run's record. Bound the + # scan: the archive is unbounded in principle, and the point here is reachability, not exhaustion. + found = False + for count, record in enumerate(pv_client.iter_pv_metadata()): + if record.pvName == pv_name: + found = True + break + if count >= 5000: + self.skipTest("catalogue too large to confirm browse-all reachability within the scan bound") + self.assertTrue(found, "iter_pv_metadata() with no criteria should reach the saved record") + self.logger.info("PV metadata: key-only search and browse-all both verified against the live server") + + # ------------------------------------------------------------------ + # Configurations + # ------------------------------------------------------------------ + + def test_configuration_key_only_and_browse_all(self): + mc = self.client.annotation.machine_config + config_name = f"itest-relax-cfg-{self.run_id}" + key = f"itest_cfg_key_{self.run_id}" + + self.addCleanup(mc.delete_configuration, config_name) + save = mc.save_configuration( + SaveConfigurationRequestParams( + configuration_name=config_name, + category="integration-test", # required by the server + attributes={key: STORED_VALUE}, + modified_by="dp-python-lib-integration-test", + ) + ) + self.assertFalse(save.result_status.is_error, f"saveConfiguration failed: {save.result_status.message}") + + for label, criterion in ( + ("values omitted", ConfigurationQuery.attributes(key)), + ("values empty list", ConfigurationQuery.attributes(key, [])), + ): + with self.subTest(form=label): + result = mc.query_configurations([criterion]) + self.assertFalse( + result.result_status.is_error, + f"key-only queryConfigurations ({label}) failed: {result.result_status.message}", + ) + self.assertIn( + config_name, + [c.configurationName for c in result.configurations], + f"key-only search ({label}) must match on key existence alone", + ) + + negative = mc.query_configurations([ConfigurationQuery.attributes(key, [NON_MATCHING_VALUE])]) + self.assertFalse(negative.result_status.is_error, negative.result_status.message) + self.assertNotIn( + config_name, + [c.configurationName for c in negative.configurations], + "a value-based query for a value the record does not have must not match it", + ) + + browse = mc.query_configurations() + self.assertFalse( + browse.result_status.is_error, + f"browse-all queryConfigurations must not be rejected: {browse.result_status.message}", + ) + self.assertIn( + config_name, + [c.configurationName for c in mc.iter_configurations()], + "iter_configurations() with no criteria should reach the saved configuration", + ) + self.logger.info("Configurations: key-only search and browse-all both verified against the live server") + + # ------------------------------------------------------------------ + # Configuration activations + # ------------------------------------------------------------------ + + def test_configuration_activation_key_only_and_browse_all(self): + mc = self.client.annotation.machine_config + config_name = f"itest-relax-act-cfg-{self.run_id}" + activation_id = f"itest-relax-act-{self.run_id}" + key = f"itest_act_key_{self.run_id}" + + # An activation requires its configuration to exist; tear down in reverse order. + self.addCleanup(mc.delete_configuration, config_name) + self.addCleanup(mc.delete_configuration_activation, client_activation_id=activation_id) + + save_config = mc.save_configuration( + SaveConfigurationRequestParams( + configuration_name=config_name, + category="integration-test", + modified_by="dp-python-lib-integration-test", + ) + ) + self.assertFalse( + save_config.result_status.is_error, + f"saveConfiguration failed: {save_config.result_status.message}", + ) + + save = mc.save_configuration_activation( + SaveConfigurationActivationRequestParams( + configuration_name=config_name, + start_time=datetime(2026, 3, 1, tzinfo=timezone.utc), + end_time=datetime(2026, 3, 2, tzinfo=timezone.utc), + client_activation_id=activation_id, + attributes={key: STORED_VALUE}, + modified_by="dp-python-lib-integration-test", + ) + ) + self.assertFalse( + save.result_status.is_error, + f"saveConfigurationActivation failed: {save.result_status.message}", + ) + + for label, criterion in ( + ("values omitted", ConfigurationActivationQuery.attributes(key)), + ("values empty list", ConfigurationActivationQuery.attributes(key, [])), + ): + with self.subTest(form=label): + result = mc.query_configuration_activations([criterion]) + self.assertFalse( + result.result_status.is_error, + f"key-only queryConfigurationActivations ({label}) failed: {result.result_status.message}", + ) + self.assertIn( + activation_id, + [a.clientActivationId for a in result.configuration_activations], + f"key-only search ({label}) must match on key existence alone", + ) + + negative = mc.query_configuration_activations( + [ConfigurationActivationQuery.attributes(key, [NON_MATCHING_VALUE])] + ) + self.assertFalse(negative.result_status.is_error, negative.result_status.message) + self.assertNotIn( + activation_id, + [a.clientActivationId for a in negative.configuration_activations], + "a value-based query for a value the record does not have must not match it", + ) + + browse = mc.query_configuration_activations() + self.assertFalse( + browse.result_status.is_error, + f"browse-all queryConfigurationActivations must not be rejected: {browse.result_status.message}", + ) + self.assertIn( + activation_id, + [a.clientActivationId for a in mc.iter_configuration_activations()], + "iter_configuration_activations() with no criteria should reach the saved activation", + ) + self.logger.info("Activations: key-only search and browse-all both verified against the live server") + + +class TestV2SelectorRelaxations(unittest.TestCase): + """ + Covers #40 on the v2 query service selectors (`PvQuery.attr`), which #41 deliberately does not touch. + + This is the one path where the client-side non-blank-key check is the ONLY one there is: `QueryV2Resolver` + does not validate the key, so a blank one would reach Mongo as an existence test on "attributes." and match + nothing silently (`plan/tickets/40/plan.md` T5). Reaching the selector at all needs archived samples, so this + class ingests its own -- through the generated stub, since IngestionClient wraps only registerProvider() + until #17, the same approach test_datasets_annotations_integration.py takes. + """ + + SAMPLE_COUNT = 5 + SAMPLE_PERIOD_NANOS = 1_000_000_000 + + @classmethod + def setUpClass(cls): + logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s") + cls.logger = logging.getLogger(__name__) + for label, address in (("annotation", ANNOTATION_ADDRESS), ("ingestion", INGESTION_ADDRESS)): + _require_service(cls.logger, label, address) + + cls.client = MldpClient() + cls.run_id = int(time.time()) + cls.pv_name = f"ITEST:RELAX:V2:{cls.run_id}" + cls.attribute_key = f"itest_v2_key_{cls.run_id}" + + # Whole seconds, so the SamplingClock start lands exactly on the ingested axis. + cls.begin_time = datetime.now(timezone.utc).replace(microsecond=0) - timedelta(minutes=5) + cls.end_time = cls.begin_time + timedelta(seconds=cls.SAMPLE_COUNT) + + cls._ingest_samples() + cls._catalogue_pv() + + @classmethod + 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() + + @classmethod + def _ingest_samples(cls): + """Ingests a few samples for this run's PV, so the v2 selector has something to select.""" + cls._ingestion_channel = grpc.insecure_channel(INGESTION_ADDRESS) + stub = ingestion_pb2_grpc.DpIngestionServiceStub(cls._ingestion_channel) + + registration = stub.registerProvider( + ingestion_pb2.RegisterProviderRequest(providerName=f"itest_relax_provider_{cls.run_id}"), timeout=10 + ) + if registration.HasField("exceptionalResult"): + raise unittest.SkipTest( + f"could not register an ingestion provider: {registration.exceptionalResult.message}" + ) + + request = ingestion_pb2.IngestDataRequest( + providerId=registration.registrationResult.providerId, + clientRequestId=f"itest-relax-{cls.run_id}", + ) + clock = request.ingestionDataFrame.dataTimestamps.samplingClock + clock.startTime.epochSeconds = int(cls.begin_time.timestamp()) + clock.periodNanos = cls.SAMPLE_PERIOD_NANOS + clock.count = cls.SAMPLE_COUNT + + column = request.ingestionDataFrame.dataColumns.add() + column.name = cls.pv_name + for i in range(cls.SAMPLE_COUNT): + column.dataValues.add().doubleValue = float(i) + + response = stub.ingestData(request, timeout=15) + if response.HasField("exceptionalResult"): + raise unittest.SkipTest(f"could not ingest test data: {response.exceptionalResult.message}") + cls.logger.info("Ingested %d samples for %s", cls.SAMPLE_COUNT, cls.pv_name) + + @classmethod + def _catalogue_pv(cls): + """Gives the ingested PV the attribute the key-only selector will search on.""" + result = cls.client.annotation.pv_metadata.save_pv_metadata( + SavePvMetadataRequestParams( + pv_name=cls.pv_name, + attributes={cls.attribute_key: STORED_VALUE}, + modified_by="dp-python-lib-integration-test", + ) + ) + if result.result_status.is_error: + raise unittest.SkipTest(f"could not catalogue the test PV: {result.result_status.message}") + + def _query_columns(self, criterion, attempts=20, delay_seconds=0.5): + """ + Runs a v2 query selecting on `criterion`, returning the column names it produced. + + ingestData() acks before the bucket is queryable, so poll rather than sleeping a fixed interval -- the same + reason test_datasets_annotations_integration.py probes for archive visibility. + """ + params = QueryParams( + begin_time=self.begin_time, + end_time=self.end_time, + pv_selector=PvQuery.metadata([criterion]), + ) + for _ in range(attempts): + result = self.client.query.query_samples(params) + self.assertFalse( + result.result_status.is_error, + f"querySamples failed: {result.result_status.message}", + ) + names = [column.name for column in result.column_table.dataColumns] + if names: + return names + time.sleep(delay_seconds) + return [] + + def test_v2_key_only_attribute_selector_returns_samples(self): + # --- #40 on the v2 path: a key-only selector selects the PV and returns its samples --- + for label, criterion in ( + ("values omitted", PvQuery.attr(self.attribute_key)), + ("values empty list", PvQuery.attr(self.attribute_key, [])), + ): + with self.subTest(form=label): + self.assertIn( + self.pv_name, + self._query_columns(criterion), + f"key-only v2 selector ({label}) should select the PV by attribute existence", + ) + + # Same distinguishing check as the annotation-service tests: a non-matching value selects nothing, so the + # hits above came from key existence rather than from an unfiltered match. + self.assertEqual( + self._query_columns(PvQuery.attr(self.attribute_key, [NON_MATCHING_VALUE]), attempts=1), + [], + "a value-based v2 selector for a value the PV does not have must select nothing", + ) + self.logger.info("v2 selector: key-only attribute search verified against the live server") + + +if __name__ == "__main__": + unittest.main(verbosity=2) From 04156a5260cf7ec40430aeb92357d87dedac147d Mon Sep 17 00:00:00 2001 From: Craig McChesney Date: Thu, 10 Sep 2026 16:10:26 -0600 Subject: [PATCH 2/2] test: address review feedback on the #40/#41 integration coverage Four fixes from the PR #48 review (three of them Copilot's findings): - Probe the query service in TestV2SelectorRelaxations. _query_columns() calls client.query.query_samples() against localhost:50052, but setUpClass probed only annotation and ingestion, so a down query service failed at the RPC instead of self-skipping as the module docstring promises. Also guards MldpClient.query being None rather than letting it surface as an AttributeError. - Use a millisecond run id (str(int(time.time() * 1000))), matching test_datasets_annotations_integration.py and test_sample_status_client_integration.py. A whole-second id is not a per-run namespace: two runners started in the same second shared every record name. - Make the v2 negative assertion's precondition explicit. An empty result means both "the selector matched nothing" and "the bucket is not queryable yet", and the negative assertion reads it as the first. Bucket visibility is now established up front with a name-list selector -- one that does not depend on the behavior under test -- so the ordering is no longer load-bearing. Verified non-vacuous: suppressing the ingest makes the guard fail with its own diagnostic rather than the negative case passing for the wrong reason. - Correct the docstring's cleanup claim. Catalogue records are torn down per run, but the ingested samples cannot be: the archive has no delete RPC, so each run leaves SAMPLE_COUNT samples under a run-unique PV name, the same residue test_datasets_annotations_integration.py leaves. CLAUDE.md's entry gets the same correction. 771 pass with the ecosystem up (725 unit + 46 integration), ruff clean, 107 cookbook snippets checked. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn --- CLAUDE.md | 2 +- ...st_query_helper_relaxations_integration.py | 50 +++++++++++++++---- 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ace18a1..d5d5980 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -175,7 +175,7 @@ plan documents one change, `CLAUDE.md` documents the invariant it established. - `tests/unit/test_export_client.py` - Unit tests for ExportClient (`ExportFormat` mapping and unreachable `UNSPECIFIED`, `calculations_spec()`, the zero-source rejection, three-tier error handling) - `tests/unit/test_annotation_client.py` - Unit tests pinning the `AnnotationClient` facade wiring (every feature client present, one shared channel, one stub apiece) - `tests/integration/test_datasets_annotations_integration.py` - Live-server round trip for datasets/annotations/calculations; ingests its own samples first, because `saveDataSet` requires archived PVs -- `tests/integration/test_query_helper_relaxations_integration.py` - Live-server coverage for the #40 key-only `attributes()` search and the #41 browse-all `criteria`, on PV metadata, configurations, activations, and the v2 `PvQuery.attr` selector. Both rest on server behavior a unit test cannot reach: a unit test asserts the request carries `values == []`, but only a real server distinguishes an existence filter from an `$in: []` that matches nothing. Each test therefore stores an attribute value, asserts the key-only form finds the record, and asserts a query for a *different* value does not -- that pairing is what makes the first assertion meaningful. The v2 class ingests its own samples (the selector needs archived data) and polls for bucket visibility rather than sleeping +- `tests/integration/test_query_helper_relaxations_integration.py` - Live-server coverage for the #40 key-only `attributes()` search and the #41 browse-all `criteria`, on PV metadata, configurations, activations, and the v2 `PvQuery.attr` selector. Both rest on server behavior a unit test cannot reach: a unit test asserts the request carries `values == []`, but only a real server distinguishes an existence filter from an `$in: []` that matches nothing. Each test therefore stores an attribute value, asserts the key-only form finds the record, and asserts a query for a *different* value does not -- that pairing is what makes the first assertion meaningful. The v2 class ingests its own samples (the selector needs archived data) and polls for bucket visibility rather than sleeping; it establishes that visibility with a *name-list* selector before asserting the negative case, so an empty result can only mean the attribute selector matched nothing. Catalogue records are torn down per run, but the ingested samples are not -- the archive has no delete RPC, so each run leaves a few samples under a run-unique PV name, the same residue `test_datasets_annotations_integration.py` leaves - `tests/unit/test_time_conversions.py` - Unit tests for the shared time converters (`to_timestamp()` input forms and the naive-datetime/bool/unsupported-type rejections; `to_epoch_nanos()` exactness and its round trip with `to_timestamp()`) - `tests/unit/test_data_frame.py` - Unit tests for the data_frame builders (axis relocation, each typed column, `data_column()` bool-before-int and unset-oneof handling, provenance helpers, and every `data_frame()` shape rule incl. array dims and serialized-column name-only checks) - `tests/unit/test_data_frame_conversions.py` - Unit tests for data_frame_conversions (nanosecond-exact expansion, per-column conversion incl. array reshaping, duplicate-name fail-loud, and — skipping cleanly without `[analysis]` — the pandas round trip, dtype mapping, and NaN fail-loud) diff --git a/tests/integration/test_query_helper_relaxations_integration.py b/tests/integration/test_query_helper_relaxations_integration.py index 8b6a02e..a2368fe 100644 --- a/tests/integration/test_query_helper_relaxations_integration.py +++ b/tests/integration/test_query_helper_relaxations_integration.py @@ -15,12 +15,14 @@ against an empty list. Prerequisites: -- MLDP services running (annotation service at localhost:50053; TestV2SelectorRelaxations also needs ingestion at - localhost:50051). Tests self-skip when they are absent. +- MLDP services running (annotation service at localhost:50053; TestV2SelectorRelaxations additionally needs + ingestion at localhost:50051 and query at localhost:50052). Tests self-skip when they are absent. - Run with: pytest tests/integration/test_query_helper_relaxations_integration.py -v -Every record is namespaced with a per-run id and removed via addCleanup, so repeated runs do not accumulate state -and a mid-test failure still tears down. +Every *catalogue* record (PV metadata, configurations, activations) is namespaced with a per-run id and removed via +addCleanup, so a mid-test failure still tears down. The samples TestV2SelectorRelaxations ingests are the one +exception: the archive has no delete RPC, so each run leaves SAMPLE_COUNT samples behind under a run-unique PV name +-- the same residue test_datasets_annotations_integration.py leaves, and for the same reason. """ import logging @@ -48,6 +50,7 @@ ANNOTATION_ADDRESS = "localhost:50053" INGESTION_ADDRESS = "localhost:50051" +QUERY_ADDRESS = "localhost:50052" # The value stored on every probe record. Key-only searches must find these WITHOUT naming the value; the # NON_MATCHING_VALUE below is what a value-based query is asked for instead, to prove the two differ. @@ -84,7 +87,10 @@ def setUpClass(cls): _require_service(cls.logger, "annotation", ANNOTATION_ADDRESS) cls.client = MldpClient() - cls.run_id = int(time.time()) + # Milliseconds, matching test_datasets_annotations_integration.py and + # test_sample_status_client_integration.py: a whole-second id is not a per-run namespace, since two + # runners started in the same second would share every record name below. + cls.run_id = str(int(time.time() * 1000)) cls.logger.info("Query-helper relaxation integration run id: %s", cls.run_id) # ------------------------------------------------------------------ @@ -311,11 +317,19 @@ class ingests its own -- through the generated stub, since IngestionClient wraps def setUpClass(cls): logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s") cls.logger = logging.getLogger(__name__) - for label, address in (("annotation", ANNOTATION_ADDRESS), ("ingestion", INGESTION_ADDRESS)): + # The query service is a prerequisite here as much as the other two: _query_columns() calls + # client.query.query_samples(). Without this probe the class fails at the RPC instead of self-skipping. + for label, address in ( + ("annotation", ANNOTATION_ADDRESS), + ("ingestion", INGESTION_ADDRESS), + ("query", QUERY_ADDRESS), + ): _require_service(cls.logger, label, address) cls.client = MldpClient() - cls.run_id = int(time.time()) + if cls.client.query is None: + raise unittest.SkipTest("MldpClient has no query channel configured; cannot exercise the v2 selector.") + cls.run_id = str(int(time.time() * 1000)) cls.pv_name = f"ITEST:RELAX:V2:{cls.run_id}" cls.attribute_key = f"itest_v2_key_{cls.run_id}" @@ -380,17 +394,21 @@ def _catalogue_pv(cls): if result.result_status.is_error: raise unittest.SkipTest(f"could not catalogue the test PV: {result.result_status.message}") - def _query_columns(self, criterion, attempts=20, delay_seconds=0.5): + def _query_columns(self, criterion, attempts=20, delay_seconds=0.5, name_list=False): """ Runs a v2 query selecting on `criterion`, returning the column names it produced. ingestData() acks before the bucket is queryable, so poll rather than sleeping a fixed interval -- the same reason test_datasets_annotations_integration.py probes for archive visibility. + + `name_list=True` ignores `criterion` and selects the run's PV by name instead, which is how the caller + establishes bucket visibility without relying on the attribute selector under test. """ + selector = PvQuery.name_list([self.pv_name]) if name_list else PvQuery.metadata([criterion]) params = QueryParams( begin_time=self.begin_time, end_time=self.end_time, - pv_selector=PvQuery.metadata([criterion]), + pv_selector=selector, ) for _ in range(attempts): result = self.client.query.query_samples(params) @@ -405,6 +423,17 @@ def _query_columns(self, criterion, attempts=20, delay_seconds=0.5): return [] def test_v2_key_only_attribute_selector_returns_samples(self): + # An empty result means two different things -- "the selector matched nothing" and "the bucket is not + # queryable yet" -- and the negative assertion below reads it as the first. So establish visibility up + # front with a selector that does NOT depend on the behavior under test: a plain name list. Once this + # passes, an empty result from an attribute selector can only be the selector. + self.assertIn( + self.pv_name, + self._query_columns(None, name_list=True), + "the ingested samples never became queryable; cannot distinguish an empty selector result from an " + "invisible bucket", + ) + # --- #40 on the v2 path: a key-only selector selects the PV and returns its samples --- for label, criterion in ( ("values omitted", PvQuery.attr(self.attribute_key)), @@ -418,7 +447,8 @@ def test_v2_key_only_attribute_selector_returns_samples(self): ) # Same distinguishing check as the annotation-service tests: a non-matching value selects nothing, so the - # hits above came from key existence rather than from an unfiltered match. + # hits above came from key existence rather than from an unfiltered match. Meaningful because the + # visibility assertion above already proved an empty result here is the selector's doing. self.assertEqual( self._query_columns(PvQuery.attr(self.attribute_key, [NON_MATCHING_VALUE]), attempts=1), [],