diff --git a/docs/TESTING.md b/docs/TESTING.md index a93b600f6..183e9327d 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -112,18 +112,18 @@ cd src/ingestion/tests/e2e **shallow acceptance validation** runs in **Beta**. - Every user-facing surface **should** have at least one smoke assertion. - A separate **compose-stand suite** (`tests/stand`, documented in `tests/stand/README.md`) drives a real Keycloak - login and four browser journeys against the SPA, plus an API-contract suite — all against a local + login and a set of browser journeys against the SPA, plus an API-contract suite — all against a local `docker-compose` stand seeded deterministically for tests (`deploy/seed`). Run it with - `./dev-compose.sh test-stand up|test|down`. It asserts no metric VALUE: the seed's `golden_metrics` is empty by - design, and a harness for it is being migrated separately. + `./dev-compose.sh test-stand up|test|down`. It asserts no metric VALUE against a declared expectation: the seed's + `golden_metrics` is empty by design, and a harness for it is being migrated separately. It does reconcile every + metric's drilldown evidence against that metric's own served value, which needs no declared expectation. **CI:** `functional-k3s.yml` — ephemeral k3d install. Today it only *installs*; a real smoke must build + import the PR's images and assert `/health` + a few golden metrics. -**CI:** `e2e-stand.yml` — two **non-required** checks against the compose-stand suite: `api-smoke` (117 HTTP -contract tests, no browser) and `ui-journeys` (10 tests: the four browser journeys, run inside the published -`ui-tests` image). Neither blocks merge — both stand up a full stack against a live IdP and their flake rate is -still unmeasured. +**CI:** `e2e-stand.yml` — two **non-required** checks against the compose-stand suite: `api-smoke` (the HTTP +contract tests, no browser) and `ui-journeys` (the browser journeys, run inside the published `ui-tests` image). +Neither blocks merge — both stand up a full stack against a live IdP and their flake rate is still unmeasured. --- diff --git a/src/backend/services/analytics/src/domain/metric_definitions/builtin.rs b/src/backend/services/analytics/src/domain/metric_definitions/builtin.rs index 2b094727c..fd87afbe5 100644 --- a/src/backend/services/analytics/src/domain/metric_definitions/builtin.rs +++ b/src/backend/services/analytics/src/domain/metric_definitions/builtin.rs @@ -203,6 +203,19 @@ mod tests { } } + #[test] + fn evidence_refs_parse_as_evidence_relations() { + use crate::domain::metric_definitions::definition::EvidenceRelation; + for builtin_source in builtin_sources() { + assert!( + EvidenceRelation::parse(&builtin_source.source.evidence_ref).is_some(), + "builtin source {} declares an invalid evidence relation {:?}", + builtin_source.source.key, + builtin_source.source.evidence_ref, + ); + } + } + #[test] fn every_source_declares_at_least_one_measure() { for builtin_source in builtin_sources() { diff --git a/tests/stand/README.md b/tests/stand/README.md index 7d6320fda..44d956d7f 100644 --- a/tests/stand/README.md +++ b/tests/stand/README.md @@ -1,7 +1,7 @@ # The compose-stand suite -Deployed-stand tests for Insight: a real Keycloak login, four browser -journeys against the SPA, and an API-contract suite — all run against a local +Deployed-stand tests for Insight: a real Keycloak login, browser journeys +against the SPA, and an API-contract suite — all run against a local `docker-compose` stand seeded deterministically for tests (`deploy/seed`). This suite assumes an **already-running, already-seeded** stand. It never @@ -20,7 +20,7 @@ persona resolution) lives in `../lib`; both are one uv project | `api/analytics/` | The `/api/analytics` prefix, one module per path group. | | `api/identity/` | The `/api/identity` prefix, one module per concern. | | `api/test_gateway.py` | Neither service — the edge, sweeping 401 over every catalogued operation at once. | -| `ui/` | The four browser journeys, plus `ui/pages/` (page objects). | +| `ui/` | The browser journeys, plus `ui/pages/` (page objects). | Split by service because that is the axis along which a test's setup differs: identity's answers depend on **who is asking** (the org chart, the @@ -76,6 +76,13 @@ disagree. Before adding a test, read it for: test here asserts a metric's exact value, and none should until the table has entries — reading a number off a running stand and asserting it back only proves that the code which produced it produced it. + + What `api/analytics/test_drilldown.py` does is a different thing and is + allowed: it asks two independent serving relations the same question — the + evidence rows behind a metric, and the metric's own value — and requires them + to agree. Neither side is a number typed into the test, so the seed can change + underneath it, and a disagreement is a real defect rather than a stale + expectation. `drilldown_matrix.py` states what "agree" means per metric. - **capabilities** — e.g. `ingestion`, which this stand does not have (compose seeds silver/gold directly). A test that needs a capability the stand may lack should carry the matching marker (below), not assume it. @@ -118,7 +125,7 @@ more API test than one more browser test whenever the two would prove the same thing. State the reason as a paragraph in the test module's docstring, in the -shape the four shipped journeys already use — for example +shape the shipped journeys already use — for example `ui/test_logged_out_access_refused.py`: > Why this is a browser test and not an API test, measured rather than diff --git a/tests/stand/api/analytics/drilldown_matrix.py b/tests/stand/api/analytics/drilldown_matrix.py new file mode 100644 index 000000000..3c712676a --- /dev/null +++ b/tests/stand/api/analytics/drilldown_matrix.py @@ -0,0 +1,201 @@ +"""What each metric's evidence must add up to, one row per metric key. + +Every metric in the registry is drilldown-declared: a source carries an +`evidence_ref` and a measure carries an `evidence_granularity`, both mandatory, +so capability is decided at run time from the health of the evidence relation +rather than per metric. Sweeping the catalogue is therefore the only way to +notice that one source's evidence has drifted from the observations derived from +it — the modal would show wrong numbers with every other test still green. + +The sweep needs an expectation per metric, and it cannot be one rule: an +evidence row means something different at each granularity, and the period +scalar is a different aggregate per computation. `Tier` is that expectation, and +each variant is the STRONGEST statement provable from the serving path: + +* observations are derived from evidence by the gold models, under the same + scope predicate the drilldown compiler uses, so the two sides are the same + rows aggregated twice; +* rows a person's identity did not resolve to reach neither side, so the two + cannot disagree about which rows exist; +* `max`/`min` collapse across one person's several source accounts is the one + place they legitimately can, which is why those metrics get an inequality and + not an equality. + +Kept as a literal rather than derived from a live response so that collection +stays offline, matching the rest of this suite, and so that adding a metric +without deciding what its evidence means is a failure rather than a silent gap. +`test_every_metric_definition_is_in_the_drilldown_matrix` is what enforces that. +""" + +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import dataclass +from enum import StrEnum + + +class Tier(StrEnum): + """The reconciliation a metric's evidence supports.""" + + #: Event grain whose contribution is a constant 1 and which projects no + #: value column: one row IS one unit of the metric. + EXACT_COUNT = "exact_count" + #: Additive measure summed into observations: the projected values sum to + #: the period scalar. + EXACT_SUM = "exact_sum" + #: Median over event rows passed through to observations one-for-one, so + #: the scalar is one of the projected values. + EXACT_MEDIAN = "exact_median" + #: Median over a measure whose value column is not projected, recoverable + #: from the detail columns it was computed from. + DERIVED_MEDIAN = "derived_median" + #: Ratio of two additive measures: the summed numerator over the summed + #: denominator, scaled, then transformed. + EXACT_RATIO = "exact_ratio" + #: Distinct count whose counted subject is the day itself. + EXACT_DISTINCT_DATES = "exact_distinct_dates" + #: Sum of a day flag that collapses by `max` or `min` across a person's + #: source accounts: evidence sums to at least the period scalar. + COLLAPSE_BOUNDED_SUM = "collapse_bounded_sum" + #: Ratio whose denominator is such a day flag: the evidence ratio is at + #: most the period scalar. + COLLAPSE_BOUNDED_RATIO = "collapse_bounded_ratio" + #: Distinct count whose counted subject is projected nowhere; only the + #: bound `1 <= period <= rows` survives. + STRUCTURAL_ONLY = "structural_only" + + +@dataclass(frozen=True) +class Transform: + """The affine-and-clamp a definition applies AFTER aggregation. + + It reaches no response — `scale` is on the wire but this is not — so + reconciling a clamped metric means applying it here, in the same order the + SQL does, and a metric pinned at its bound compares exactly. + """ + + multiplier: float = 1.0 + offset: float = 0.0 + clamp_min: float | None = None + clamp_max: float | None = None + + def apply(self, value: float) -> float: + transformed = self.multiplier * value + self.offset + if self.clamp_min is not None: + transformed = max(transformed, self.clamp_min) + if self.clamp_max is not None: + transformed = min(transformed, self.clamp_max) + return transformed + + +@dataclass(frozen=True) +class Expectation: + """One metric's drilldown expectation. + + `source` is the evidence family, and it is what makes a failure readable: + capability and schema health are per source, so a whole family failing at + once is a different defect from one metric failing alone. + """ + + metric_key: str + source: str + tier: Tier + #: Ratio scale, from the definition's computation. + scale: float | None = None + transform: Transform | None = None + #: Detail columns a `DERIVED_MEDIAN` metric's value is the sum of. + derived_from: tuple[str, ...] = () + + +_PERCENT = Transform(clamp_max=100.0) +_INVERTED_PERCENT = Transform(multiplier=-1.0, offset=100.0, clamp_min=0.0, clamp_max=100.0) + +MATRIX: Sequence[Expectation] = ( + Expectation("ai.accepted_edit_actions", "ai", Tier.EXACT_SUM), + Expectation("ai.accepted_lines", "ai", Tier.EXACT_SUM), + Expectation("ai.active_days", "ai", Tier.COLLAPSE_BOUNDED_SUM), + Expectation("ai.assistant_actions", "ai", Tier.EXACT_SUM), + Expectation("ai.assistant_messages", "ai", Tier.EXACT_SUM), + Expectation("ai.chat_assistant_conversations", "ai", Tier.EXACT_SUM), + Expectation("ai.cost", "ai", Tier.EXACT_SUM), + Expectation("ai.dev_conversations", "ai", Tier.EXACT_SUM), + Expectation("ai.removed_lines", "ai", Tier.EXACT_SUM), + Expectation("ai.tool_acceptance_rate", "ai", Tier.EXACT_RATIO, scale=100.0), + Expectation("collab.active_days", "collab", Tier.EXACT_DISTINCT_DATES), + Expectation("collab.adhoc_meetings", "collab", Tier.EXACT_SUM), + Expectation("collab.breadth", "collab", Tier.STRUCTURAL_ONLY), + Expectation("collab.channel_posts", "collab", Tier.EXACT_SUM), + Expectation("collab.dm_ratio", "collab", Tier.EXACT_RATIO, scale=100.0), + Expectation("collab.emails_read", "collab", Tier.EXACT_SUM), + Expectation("collab.emails_received", "collab", Tier.EXACT_SUM), + Expectation("collab.emails_sent", "collab", Tier.EXACT_SUM), + Expectation("collab.files_engaged", "collab", Tier.EXACT_SUM), + Expectation("collab.files_shared", "collab", Tier.EXACT_SUM), + Expectation("collab.files_shared_external", "collab", Tier.EXACT_SUM), + Expectation("collab.files_shared_internal", "collab", Tier.EXACT_SUM), + Expectation("collab.focus_time_pct", "collab", Tier.EXACT_RATIO, scale=100.0), + Expectation("collab.meeting_free_days", "collab", Tier.COLLAPSE_BOUNDED_SUM), + Expectation("collab.meeting_hours", "collab", Tier.EXACT_SUM), + Expectation("collab.meetings_count", "collab", Tier.EXACT_SUM), + Expectation("collab.meetings_organized", "collab", Tier.EXACT_SUM), + Expectation("collab.messages_sent", "collab", Tier.EXACT_SUM), + Expectation("collab.msgs_per_active_day", "collab", Tier.COLLAPSE_BOUNDED_RATIO, scale=1.0), + Expectation("collab.scheduled_meetings", "collab", Tier.EXACT_SUM), + Expectation("git.code_lines", "git", Tier.EXACT_SUM), + Expectation( + "git.commit_size", + "git", + Tier.DERIVED_MEDIAN, + derived_from=("lines_added", "lines_removed"), + ), + Expectation("git.commits", "git", Tier.EXACT_COUNT), + Expectation("git.commits_per_active_day", "git", Tier.COLLAPSE_BOUNDED_RATIO, scale=1.0), + Expectation("git.lines_added", "git", Tier.EXACT_SUM), + Expectation("git.lines_removed", "git", Tier.EXACT_SUM), + Expectation("git.merge_rate", "git", Tier.EXACT_RATIO, scale=100.0), + Expectation("git.pr_cycle_time_h", "git", Tier.EXACT_MEDIAN), + Expectation("git.pr_size", "git", Tier.EXACT_MEDIAN), + Expectation("git.prs_created", "git", Tier.EXACT_COUNT), + Expectation("git.prs_merged", "git", Tier.EXACT_COUNT), + Expectation("tasks.avg_slip", "task", Tier.EXACT_RATIO, scale=1.0), + Expectation("tasks.bugs_fixed", "task", Tier.EXACT_COUNT), + Expectation("tasks.bugs_ratio", "task", Tier.EXACT_RATIO, scale=100.0), + Expectation("tasks.closed", "task", Tier.EXACT_COUNT), + Expectation("tasks.dev_time", "task", Tier.EXACT_MEDIAN), + Expectation("tasks.due_date_compliance", "task", Tier.EXACT_RATIO, scale=100.0), + Expectation( + "tasks.estimation_accuracy", + "task", + Tier.EXACT_RATIO, + scale=1.0, + transform=_INVERTED_PERCENT, + ), + Expectation("tasks.flow_efficiency", "task", Tier.EXACT_RATIO, scale=100.0, transform=_PERCENT), + Expectation("tasks.on_time_delivery", "task", Tier.EXACT_RATIO, scale=100.0), + Expectation("tasks.pickup_time", "task", Tier.EXACT_MEDIAN), + Expectation("tasks.reopen_rate", "task", Tier.EXACT_RATIO, scale=100.0), + Expectation("tasks.resolution_time", "task", Tier.EXACT_MEDIAN), + Expectation("tasks.stale_in_progress", "task", Tier.EXACT_SUM), + Expectation( + "tasks.worklog_accuracy", "task", Tier.EXACT_RATIO, scale=100.0, transform=_PERCENT + ), + Expectation("wiki.comments", "wiki", Tier.EXACT_SUM), + Expectation("wiki.edits", "wiki", Tier.EXACT_SUM), + Expectation("wiki.pages_created", "wiki", Tier.EXACT_COUNT), + Expectation("wiki.pages_edited", "wiki", Tier.EXACT_SUM), +) + +#: One metric per distinct evidence presentation, plus the capable-but-empty +#: case. A presentation is all an export can differ by — the column set and the +#: header labels are everything it serializes — and every other metric in the +#: catalogue reuses one of these, so exporting the whole catalogue would repeat +#: these answers rather than add any. +EXPORT_SHAPES: Sequence[str] = ( + "git.prs_created", + "git.pr_cycle_time_h", + "tasks.closed", + "tasks.dev_time", + "git.merge_rate", + "collab.messages_sent", + "wiki.pages_created", +) diff --git a/tests/stand/api/analytics/test_drilldown.py b/tests/stand/api/analytics/test_drilldown.py index bc97ebe91..174e67404 100644 --- a/tests/stand/api/analytics/test_drilldown.py +++ b/tests/stand/api/analytics/test_drilldown.py @@ -4,50 +4,140 @@ POST /v1/metric-drilldown/export 200 CSV/XLSX · 400 empty-entity The 415 half is in `test_request_contracts.py`, swept over every body route. + +Two kinds of case live here. `git.commits` is the one metric exercised with a +filter, a display dimension and a one-row page limit, so the selection plumbing +and the cursor walk are covered somewhere concretely. Everything else is a sweep +over the whole catalogue, because drilldown capability is not declared per metric +— a source declares an evidence relation and a measure declares a granularity, +both mandatory — so the interesting failure is a whole evidence family drifting +from the observations derived from it, and no single-metric test can see that. + +The sweep reconciles rather than smoke-tests: each metric's evidence must add up +to the value the dashboard shows for the same person and period. What "add up" +means per metric is `drilldown_matrix.py`, which also explains why a few metrics +only support an inequality. + +Exports cover one metric per evidence presentation rather than the whole +catalogue: a presentation is all an export can differ by. `EXPORT_SHAPES` names +them, and the metrics not in it are deliberately not exported here. """ from __future__ import annotations import csv import io +import math +import warnings import zipfile +from collections.abc import Mapping, Sequence +from dataclasses import dataclass +from typing import assert_never from xml.etree import ElementTree import pytest -from insight_stand import ApiClient, ApiResponse, Manifest, analytics_path +from insight_stand import ApiClient, ApiResponse, Manifest, PersonaSession, analytics_path from insight_stand.api import JsonValue -from ..schemas import MetricResultsResponse, PeriodView, ProblemDocument -from ..schemas.analytics import MetricDrilldownResponse +from ..schemas import ( + MetricDefinitionListResponse, + MetricResultsResponse, + PeriodView, + ProblemDocument, +) +from ..schemas.analytics import ( + MetricDrilldownCapability, + MetricDrilldownResponse, +) +from .drilldown_matrix import EXPORT_SHAPES, MATRIX, Expectation, Tier DRILLDOWN = analytics_path("/v1/metric-drilldown") DRILLDOWN_EXPORT = analytics_path("/v1/metric-drilldown/export") METRIC_RESULTS = analytics_path("/v1/metric-results") +METRIC_DEFINITIONS = analytics_path("/v1/metric-definitions") GIT_COMMITS = "git.commits" +#: The endpoint's own maximum, so the sweep walks a metric in as few requests as +#: the contract allows. +_PAGE_LIMIT = 250 + +#: Pages the sweep will walk before it stops asking. Reconciliation needs every +#: row, so a metric that exceeds this budget is reported as unreconciled rather +#: than reconciled against a prefix. +_PAGE_BUDGET = 40 + +#: Aggregation order differs between the service (vectorized `sumIf` over +#: `Float64`) and this suite (row-ordered `sum`), and that is the only error +#: source — the transport is lossless both ways. +_REL_TOL = 1e-9 +_ABS_TOL = 1e-9 + #: Well-formed apart from the one field under test. An empty entity id is the #: cheapest rejection that is unambiguously the HANDLER's — it needs no seeded #: metric to reach, and no lookup can turn it into a 404 on the way. _EMPTY_ENTITY_ID = "" -def _seeded_request( +@dataclass(frozen=True) +class _Walk: + """Every page of one selection, and whether the walk reached the end.""" + + first: MetricDrilldownResponse + rows: Sequence[Mapping[str, object]] + complete: bool + + @property + def column_keys(self) -> list[str]: + return [column.key for column in self.first.columns] + + +@pytest.fixture(scope="session") +def drilldown_capabilities( + lead_session: PersonaSession, +) -> Mapping[str, MetricDrilldownCapability | None]: + """What the catalogue says each metric supports, read once for the sweep. + + Session-scoped because it is one answer for the whole run, and because the + alternative — a listing request per parametrized case — would make the sweep + cost twice what it needs to. + + Capability is derived per request from the health of the evidence relation, + so this is a claim the endpoint can contradict, and it does: the catalogue's + query requires the definition's schema to be checked as well, so it withholds + the capability for metrics the endpoint serves. The sweep asserts the + direction that holds — an advertised metric must answer — and + `test_advertised_capability_matches_what_the_endpoint_serves` carries the + other as a strict xfail. + """ + response = lead_session.client.get(METRIC_DEFINITIONS) + assert response.status_code == 200, f"definitions: {response.status_code}" + metrics = response.parse(MetricDefinitionListResponse).metrics + assert metrics, "no metric definitions — did the migrations run?" + return {metric.metric_key: metric.drilldown for metric in metrics} + + +def _request_for( manifest: Manifest, + metric_key: str, *, entity_id: str | None = None, - limit: int | None = 1, + limit: int | None = None, cursor: str | None = None, + filters: Sequence[JsonValue] = (), + display_dimensions: Sequence[str] = (), ) -> dict[str, JsonValue]: start, _, end = manifest.data_window.partition("..") request: dict[str, JsonValue] = { - "metric_key": GIT_COMMITS, + "metric_key": metric_key, "entity": { "type": "person", - "id": entity_id or manifest.fixture("dev_lead").uuid, + # Not `or`: an empty id is a case a caller may want to send, and + # falling back to a real person would quietly test something else. + "id": manifest.fixture("dev_lead").uuid if entity_id is None else entity_id, }, "period": {"from": start, "to": end}, - "filters": [{"dimension": "source", "values": ["github"]}], - "display_dimensions": ["repository"], + "filters": list(filters), + "display_dimensions": list(display_dimensions), } if limit is not None: request["limit"] = limit @@ -56,20 +146,76 @@ def _seeded_request( return request -def _all_rows( - api: ApiClient, manifest: Manifest -) -> tuple[MetricDrilldownResponse, list[dict[str, object]]]: - cursor: str | None = None +def _seeded_request( + manifest: Manifest, + *, + entity_id: str | None = None, + limit: int | None = 1, + cursor: str | None = None, +) -> dict[str, JsonValue]: + return _request_for( + manifest, + GIT_COMMITS, + entity_id=entity_id, + limit=limit, + cursor=cursor, + filters=[{"dimension": "source", "values": ["github"]}], + display_dimensions=["repository"], + ) + + +def _page( + api: ApiClient, + manifest: Manifest, + metric_key: str, + *, + limit: int, + cursor: str | None = None, + filters: Sequence[JsonValue] = (), + display_dimensions: Sequence[str] = (), +) -> ApiResponse: + return api.post( + DRILLDOWN, + json_body=_request_for( + manifest, + metric_key, + limit=limit, + cursor=cursor, + filters=filters, + display_dimensions=display_dimensions, + ), + ) + + +def _walk( + api: ApiClient, + manifest: Manifest, + metric_key: str, + *, + limit: int, + filters: Sequence[JsonValue] = (), + display_dimensions: Sequence[str] = (), + page_budget: int | None = None, + initial: ApiResponse | None = None, +) -> _Walk: + """Every page from `initial` (or a fresh first page) to the last or the budget.""" cursors: set[str] = set() first: MetricDrilldownResponse | None = None - rows: list[dict[str, object]] = [] + rows: list[Mapping[str, object]] = [] + pages = 0 + response = initial or _page( + api, + manifest, + metric_key, + limit=limit, + filters=filters, + display_dimensions=display_dimensions, + ) while True: - response = api.post( - DRILLDOWN, - json_body=_seeded_request(manifest, cursor=cursor), + assert response.status_code == 200, ( + f"{metric_key}: status={response.status_code} {response.text[:300]}" ) - assert response.status_code == 200, f"status={response.status_code} {response.text[:300]}" page = response.parse(MetricDrilldownResponse) if first is None: first = page @@ -77,17 +223,41 @@ def _all_rows( assert page.columns == first.columns assert page.selection == first.selection rows.extend(row.values for row in page.rows) + pages += 1 cursor = page.next_cursor if cursor is None: break assert cursor not in cursors, f"repeated pagination cursor {cursor!r}" cursors.add(cursor) + if page_budget is not None and pages >= page_budget: + return _Walk(first=first, rows=rows, complete=False) + response = _page( + api, + manifest, + metric_key, + limit=limit, + cursor=cursor, + filters=filters, + display_dimensions=display_dimensions, + ) assert first is not None - return first, rows + return _Walk(first=first, rows=rows, complete=True) -def _period_value(api: ApiClient, manifest: Manifest) -> float: +def _period_value( + api: ApiClient, + manifest: Manifest, + metric_key: str, + *, + filters: Sequence[JsonValue] = (), +) -> float | None: + """The scalar the dashboard shows for the same person, period and filters. + + `None` is a real answer rather than a failure: nothing zero-fills, so a + person with no observations in the period has no value at all, and that is + what an empty evidence page has to agree with. + """ start, _, end = manifest.data_window.partition("..") person_id = manifest.fixture("dev_lead").uuid response = api.post( @@ -97,8 +267,8 @@ def _period_value(api: ApiClient, manifest: Manifest) -> float: "period": {"from": start, "to": end}, "metrics": [ { - "metric_key": GIT_COMMITS, - "filters": [{"dimension": "source", "values": ["github"]}], + "metric_key": metric_key, + "filters": list(filters), "views": [{"view": "period"}], } ], @@ -113,12 +283,12 @@ def _period_value(api: ApiClient, manifest: Manifest) -> float: values = views[0].root.values assert len(values) == 1 assert values[0].entity_id == person_id - assert values[0].value is not None return values[0].value -def _export(api: ApiClient, manifest: Manifest, file_format: str) -> ApiResponse: - request = _seeded_request(manifest, limit=None) +def _export(api: ApiClient, request: dict[str, JsonValue], file_format: str) -> ApiResponse: + request = dict(request) + request.pop("limit", None) request["format"] = file_format return api.post(DRILLDOWN_EXPORT, json_body=request) @@ -131,46 +301,403 @@ def _xlsx_rows(content: bytes) -> int: ) +def _numbers(rows: Sequence[Mapping[str, object]], column: str, metric_key: str) -> list[float]: + values: list[float] = [] + for index, row in enumerate(rows): + value = row[column] + assert isinstance(value, int | float) and not isinstance(value, bool), ( + f"{metric_key}: row {index} column {column!r} is {value!r}, not a number" + ) + values.append(float(value)) + return values + + +def _close(actual: float, expected: float) -> bool: + return math.isclose(actual, expected, rel_tol=_REL_TOL, abs_tol=_ABS_TOL) + + +def _assert_shape(walk: _Walk, expectation: Expectation, person_id: str) -> None: + selection = walk.first.selection + assert selection.metric_key == expectation.metric_key + assert selection.entity.id == person_id + assert selection.filters == [] + assert selection.display_dimensions == [] + + keys = walk.column_keys + assert "date" in keys, f"{expectation.metric_key}: no date column in {keys}" + assert all(set(row) == set(keys) for row in walk.rows), ( + f"{expectation.metric_key}: a row's keys disagree with {keys}" + ) + + match expectation.tier: + case Tier.EXACT_COUNT: + assert "value" not in keys, ( + f"{expectation.metric_key}: a counted metric projects no value column, got {keys}" + ) + case Tier.DERIVED_MEDIAN: + assert "value" not in keys, ( + f"{expectation.metric_key}: unexpected value column in {keys}" + ) + for column in expectation.derived_from: + assert column in keys, f"{expectation.metric_key}: {column!r} missing from {keys}" + case Tier.EXACT_RATIO | Tier.COLLAPSE_BOUNDED_RATIO: + assert "numerator" in keys and "denominator" in keys, ( + f"{expectation.metric_key}: a ratio projects both sides, got {keys}" + ) + assert "value" not in keys, ( + f"{expectation.metric_key}: unexpected value column in {keys}" + ) + case ( + Tier.EXACT_SUM + | Tier.EXACT_MEDIAN + | Tier.EXACT_DISTINCT_DATES + | Tier.COLLAPSE_BOUNDED_SUM + | Tier.STRUCTURAL_ONLY + ): + assert "value" in keys, f"{expectation.metric_key}: no value column in {keys}" + case unhandled: + # A tier added without a shape rule would otherwise assert nothing. + assert_never(unhandled) + + +def _assert_median(period: float | None, values: Sequence[float], metric_key: str) -> None: + """`quantileExact(0.5)` returns a stored element, so this is an identity. + + Both middle elements are accepted rather than one: which of them the server + returns for an even count is its own tie rule, and pinning it here would test + ClickHouse rather than the evidence. + """ + ordered = sorted(values) + middle = {ordered[(len(ordered) - 1) // 2], ordered[len(ordered) // 2]} + assert period in middle, ( + f"{metric_key}: period {period} is neither middle value of {len(ordered)} " + f"evidence rows {sorted(middle)}" + ) + + +def _assert_ratio( + period: float | None, walk: _Walk, expectation: Expectation, *, bounded: bool +) -> None: + """A ratio reconciles as summed-numerator over summed-denominator. + + Two degenerate cases are the endpoint's, not this test's. An all-zero + denominator has no ratio, so the metric is null. A numerator with no rows is + also null, while the drilldown still returns a row per day whose numerator + reads `0` — the evidence cannot tell "absent" from "zero" and the metric + deliberately can, so the only safe statement there is null-or-transformed-zero. + """ + numerator = sum(_numbers(walk.rows, "numerator", expectation.metric_key)) + denominator = sum(_numbers(walk.rows, "denominator", expectation.metric_key)) + assert expectation.scale is not None, f"{expectation.metric_key}: ratio without a scale" + transform = expectation.transform + + if denominator == 0: + assert period is None, ( + f"{expectation.metric_key}: denominator sums to zero but the metric answered {period}" + ) + return + if numerator == 0: + zero = transform.apply(0.0) if transform else 0.0 + assert period is None or _close(period, zero), ( + f"{expectation.metric_key}: numerator sums to zero, so the metric is null or {zero}, " + f"and it answered {period}" + ) + return + + evidence = expectation.scale * numerator / denominator + if bounded: + assert period is not None and (period > evidence or _close(period, evidence)), ( + f"{expectation.metric_key}: evidence ratio {evidence} exceeds the metric's {period}; " + "day flags collapse to at most one row per person per day, so the metric can only be " + "the larger of the two" + ) + return + + expected = transform.apply(evidence) if transform else evidence + assert period is not None and _close(period, expected), ( + f"{expectation.metric_key}: evidence gives {expected} ({numerator}/{denominator} " + f"scaled by {expectation.scale}) but the metric answered {period}" + ) + + +def _reconcile(period: float | None, walk: _Walk, expectation: Expectation) -> None: + metric_key = expectation.metric_key + + match expectation.tier: + case Tier.EXACT_COUNT: + assert len(walk.rows) == period, ( + f"{metric_key}: {len(walk.rows)} evidence rows against a metric value of {period}" + ) + case Tier.EXACT_SUM: + total = sum(_numbers(walk.rows, "value", metric_key)) + assert period is not None and _close(total, period), ( + f"{metric_key}: evidence sums to {total} against a metric value of {period}" + ) + case Tier.EXACT_MEDIAN: + _assert_median(period, _numbers(walk.rows, "value", metric_key), metric_key) + case Tier.DERIVED_MEDIAN: + parts = [_numbers(walk.rows, column, metric_key) for column in expectation.derived_from] + _assert_median(period, [sum(row) for row in zip(*parts, strict=True)], metric_key) + case Tier.EXACT_RATIO: + _assert_ratio(period, walk, expectation, bounded=False) + case Tier.COLLAPSE_BOUNDED_RATIO: + _assert_ratio(period, walk, expectation, bounded=True) + case Tier.EXACT_DISTINCT_DATES: + dates = {row["date"] for row in walk.rows} + assert len(dates) == period, ( + f"{metric_key}: {len(dates)} distinct evidence dates against a metric value " + f"of {period}" + ) + case Tier.COLLAPSE_BOUNDED_SUM: + total = sum(_numbers(walk.rows, "value", metric_key)) + assert period is not None and (total > period or _close(total, period)), ( + f"{metric_key}: evidence sums to {total}, below the metric's {period}; a day flag " + "collapses across a person's accounts, so evidence can only be the larger side" + ) + case Tier.STRUCTURAL_ONLY: + assert period is not None and 1 <= period <= len(walk.rows), ( + f"{metric_key}: a distinct count over {len(walk.rows)} evidence rows cannot " + f"be {period}" + ) + case unhandled: + # A tier added without a reconciliation would otherwise pass silently. + assert_never(unhandled) + + +def _assert_evidence_unavailable( + response: ApiResponse, metric_key: str, capability: MetricDrilldownCapability | None +) -> None: + """A refusal has to be the documented one, and the catalogue has to agree. + + Only this direction is sound. The endpoint refuses when the evidence relation + is unhealthy, and the catalogue's capability query requires everything that + refusal tests plus more, so a refused metric can never be an advertised one. + The converse does NOT hold — see + `test_advertised_capability_matches_what_the_endpoint_serves`. + """ + assert response.status_code == 400, ( + f"{metric_key}: expected the documented refusal, got {response.status_code}: " + f"{response.text[:300]}" + ) + assert response.parse(ProblemDocument).status == 400 + assert "EVIDENCE_UNAVAILABLE" in response.text, ( + f"{metric_key}: refused without naming the precondition: {response.text[:300]}" + ) + assert capability is None, ( + f"{metric_key}: the catalogue advertises drilldown for it, so a reader is offered " + "supporting data the endpoint then refuses to serve" + ) + + +def test_every_metric_definition_is_in_the_drilldown_matrix( + drilldown_capabilities: Mapping[str, MetricDrilldownCapability | None], +) -> None: + """The sweep's denominator, pinned against the catalogue the stand serves. + + Every metric is drilldown-declared by construction, so a metric added + without an entry here would silently narrow the sweep instead of failing it. + Compared as sets rather than counted: a count would only say that the two + disagree, and the metric key is what a reader needs. + """ + expected = {expectation.metric_key for expectation in MATRIX} + served = set(drilldown_capabilities) + assert served == expected, ( + f"served but unexpected: {sorted(served - expected)}; " + f"expected but not served: {sorted(expected - served)}" + ) + + +@pytest.mark.requires_seed("dev_lead") +@pytest.mark.xfail( + strict=True, + reason="the catalogue withholds the capability for a metric whose definition schema_status " + "is not ok, while the drilldown endpoint serves that metric's evidence, so the UI hides " + "supporting data that exists", +) +def test_advertised_capability_matches_what_the_endpoint_serves( + api: ApiClient, + stand_manifest: Manifest, + drilldown_capabilities: Mapping[str, MetricDrilldownCapability | None], +) -> None: + """The half of the capability contract that does not hold yet. + + `GET /v1/metric-definitions` is what the UI reads to decide whether to offer + "supporting data" at all, so a metric the endpoint would answer but the + catalogue calls incapable has evidence no reader can reach. One metric per + evidence presentation is asked, which is enough to catch a family-wide + difference without a second sweep over the catalogue. + + Strict, so the day the two sides agree this fails as an XPASS and the marker + comes off rather than quietly staying. + """ + hidden = sorted( + metric_key + for metric_key in EXPORT_SHAPES + if drilldown_capabilities.get(metric_key) is None + and _page(api, stand_manifest, metric_key, limit=1).status_code == 200 + ) + assert hidden == [], ( + f"the endpoint serves evidence for {hidden}, and the catalogue advertises none, " + "so the affordance is hidden for metrics that have supporting data" + ) + + +@pytest.mark.requires_seed("dev_lead") +@pytest.mark.parametrize( + "expectation", MATRIX, ids=[expectation.metric_key for expectation in MATRIX] +) +def test_drilldown_reconciles_with_the_metric_value( + api: ApiClient, + stand_manifest: Manifest, + drilldown_capabilities: Mapping[str, MetricDrilldownCapability | None], + expectation: Expectation, +) -> None: + """One metric's evidence against the value the dashboard shows for it. + + No filter and no display dimension, deliberately. A ratio's two measures + need not carry the same dimensions, so filtering on one of them can empty + the denominator on the metric side while the evidence side still returns + rows — a difference in the request, read as a difference in the data. The + filtered path is covered concretely by the `git.commits` cases above. + + An empty page is a legitimate answer, and it is still an assertion: nothing + zero-fills, so evidence and metric have to be empty together. + + Driven off what the endpoint answers rather than off the advertised + capability, because the two do not agree today and the endpoint is the side + that decides whether evidence exists. + """ + person_id = stand_manifest.fixture("dev_lead").uuid + probe = _page(api, stand_manifest, expectation.metric_key, limit=_PAGE_LIMIT) + if probe.status_code != 200: + _assert_evidence_unavailable( + probe, expectation.metric_key, drilldown_capabilities.get(expectation.metric_key) + ) + return + + walk = _walk( + api, + stand_manifest, + expectation.metric_key, + limit=_PAGE_LIMIT, + page_budget=_PAGE_BUDGET, + initial=probe, + ) + _assert_shape(walk, expectation, person_id) + + period = _period_value(api, stand_manifest, expectation.metric_key) + if not walk.rows: + assert period is None, ( + f"{expectation.metric_key}: no evidence rows, but the metric answered {period}" + ) + return + + if not walk.complete: + warnings.warn( + f"{expectation.metric_key}: stopped after {_PAGE_BUDGET} pages of " + f"{_PAGE_LIMIT} rows, so its value was NOT reconciled — only the page shape was", + stacklevel=1, + ) + return + + _reconcile(period, walk, expectation) + + +@pytest.mark.requires_seed("dev_lead") +@pytest.mark.parametrize("metric_key", EXPORT_SHAPES) +def test_drilldown_export_carries_every_row( + api: ApiClient, + stand_manifest: Manifest, + metric_key: str, +) -> None: + """Both export formats, against the page the same selection returns. + + The header is the column LABELS while the page carries keys, so a serializer + that lost the labelling would still round-trip its own output — comparing + against the page is what catches it. The empty case is in here too: an export + of no evidence is a header and nothing else, not an error. + """ + walk = _walk(api, stand_manifest, metric_key, limit=_PAGE_LIMIT, page_budget=_PAGE_BUDGET) + assert walk.complete, f"{metric_key}: export shapes must be small enough to walk whole" + request = _request_for(stand_manifest, metric_key) + + csv_response = _export(api, request, "csv") + assert csv_response.status_code == 200, f"{metric_key}: {csv_response.text[:300]}" + assert csv_response.content_type.startswith("text/csv") + assert ".csv" in csv_response.headers.get("content-disposition", "") + csv_rows = list(csv.reader(io.StringIO(csv_response.content.decode("utf-8-sig")))) + assert csv_rows[0] == [column.label for column in walk.first.columns] + assert len(csv_rows) == len(walk.rows) + 1 + + xlsx_response = _export(api, request, "xlsx") + assert xlsx_response.status_code == 200, f"{metric_key}: {xlsx_response.text[:300]}" + assert xlsx_response.content_type == ( + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" + ) + assert ".xlsx" in xlsx_response.headers.get("content-disposition", "") + assert _xlsx_rows(xlsx_response.content) == len(walk.rows) + 1 + + @pytest.mark.requires_seed("dev_lead") def test_git_commit_drilldown_pages_and_reconciles( api: ApiClient, stand_manifest: Manifest ) -> None: - first, rows = _all_rows(api, stand_manifest) + walk = _walk( + api, + stand_manifest, + GIT_COMMITS, + limit=1, + filters=[{"dimension": "source", "values": ["github"]}], + display_dimensions=["repository"], + ) person_id = stand_manifest.fixture("dev_lead").uuid - assert first.selection.metric_key == GIT_COMMITS - assert first.selection.entity.id == person_id - assert first.selection.display_dimensions == ["repository"] - assert [(item.dimension, item.values) for item in first.selection.filters] == [ + assert walk.first.selection.metric_key == GIT_COMMITS + assert walk.first.selection.entity.id == person_id + assert walk.first.selection.display_dimensions == ["repository"] + assert [(item.dimension, item.values) for item in walk.first.selection.filters] == [ ("source", ["github"]) ] - column_keys = [column.key for column in first.columns] + column_keys = walk.column_keys assert "repository" in column_keys assert "date" in column_keys - assert rows - assert all(set(row) == set(column_keys) for row in rows) - assert len(rows) == _period_value(api, stand_manifest) + assert walk.rows + assert all(set(row) == set(column_keys) for row in walk.rows) + assert len(walk.rows) == _period_value( + api, + stand_manifest, + GIT_COMMITS, + filters=[{"dimension": "source", "values": ["github"]}], + ) @pytest.mark.requires_seed("dev_lead") def test_git_commit_drilldown_exports_all_rows(api: ApiClient, stand_manifest: Manifest) -> None: - first, rows = _all_rows(api, stand_manifest) + walk = _walk( + api, + stand_manifest, + GIT_COMMITS, + limit=1, + filters=[{"dimension": "source", "values": ["github"]}], + display_dimensions=["repository"], + ) + request = _seeded_request(stand_manifest, limit=None) - csv_response = _export(api, stand_manifest, "csv") + csv_response = _export(api, request, "csv") assert csv_response.status_code == 200 assert csv_response.content_type.startswith("text/csv") assert ".csv" in csv_response.headers.get("content-disposition", "") csv_rows = list(csv.reader(io.StringIO(csv_response.content.decode("utf-8-sig")))) - assert csv_rows[0] == [column.label for column in first.columns] - assert len(csv_rows) == len(rows) + 1 + assert csv_rows[0] == [column.label for column in walk.first.columns] + assert len(csv_rows) == len(walk.rows) + 1 - xlsx_response = _export(api, stand_manifest, "xlsx") + xlsx_response = _export(api, request, "xlsx") assert xlsx_response.status_code == 200 assert xlsx_response.content_type == ( "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" ) assert ".xlsx" in xlsx_response.headers.get("content-disposition", "") - assert _xlsx_rows(xlsx_response.content) == len(rows) + 1 + assert _xlsx_rows(xlsx_response.content) == len(walk.rows) + 1 @pytest.mark.requires_seed("dev_lead", "sales_ic") diff --git a/tests/stand/ui/evidence_requests.py b/tests/stand/ui/evidence_requests.py new file mode 100644 index 000000000..ca143cbe8 --- /dev/null +++ b/tests/stand/ui/evidence_requests.py @@ -0,0 +1,35 @@ +"""What the SPA actually asked for when an evidence dialog opened. + +A dialog with rows in it proves the request succeeded, not that it was the right +request: the same table renders whether the selection carried the person whose +cell was clicked, the metric that was chosen, or the period the reader was +looking at. The selection is only visible on the wire, so a journey that cares +which one was sent reads it from the request the browser made. + +`/export` is a different path, so it never satisfies this predicate. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from contextlib import contextmanager +from typing import Any + +from playwright.sync_api import Page, Request + +DRILLDOWN_PATH = "/api/analytics/v1/metric-drilldown" + + +def is_drilldown(request: Request) -> bool: + return request.method == "POST" and request.url.endswith(DRILLDOWN_PATH) + + +@contextmanager +def evidence_selection(page: Page) -> Iterator[dict[str, Any]]: + """The drilldown selection sent while the block runs, readable after it.""" + selection: dict[str, Any] = {} + with page.expect_request(is_drilldown) as request_info: + yield selection + body = request_info.value.post_data_json + assert isinstance(body, dict), f"drilldown request carried {body!r}, not a selection" + selection.update(body) diff --git a/tests/stand/ui/pages/group_dialog.py b/tests/stand/ui/pages/group_dialog.py new file mode 100644 index 000000000..c387c513f --- /dev/null +++ b/tests/stand/ui/pages/group_dialog.py @@ -0,0 +1,129 @@ +"""A metric group's details dialog, and the evidence dialog it opens. + +One object for all five groups, because the SPA renders one component for all +five: the dialog's accessible name is the group's title, and everything inside it +is composed from the same metric widgets. A per-group object would be five copies +of these locators differing only in a string. + +The evidence dialog is the second, nested one. Its accessible name is the metric +label the API served, so a caller either passes the label it read from the view or +a pattern — never a label this suite invented, since the wording is the server's. + +Accessibility-first, like every page object here: the published SPA carries no +`data-testid` attributes, so roles and accessible names are the only stable +handles. Two exceptions, both unavoidable and both narrow: `[data-slot="card"]` +to scope a locator to the block that owns a control, and positional row/cell +indexes where a value button's only accessible name is the number it renders. +""" + +from __future__ import annotations + +import re + +from playwright.sync_api import Locator, Page + +#: The evidence affordance every metric widget shares, whether it hangs off a +#: card's overflow menu or a timeseries block's header. +SUPPORTING_DATA = "View supporting data" + + +class MetricEvidenceDialog: + def __init__(self, page: Page, metric: str | re.Pattern[str]) -> None: + self.page = page + self.dialog = page.get_by_role("dialog", name=metric) + + def table(self) -> Locator: + return self.dialog.get_by_role("table") + + def column_header(self, name: str) -> Locator: + return self.table().get_by_role("columnheader", name=name) + + def export(self) -> Locator: + return self.dialog.get_by_role("button", name="Export") + + def copy_ref(self) -> Locator: + return self.dialog.get_by_role("button", name=re.compile(r"^Copy ")) + + def close(self) -> Locator: + return self.dialog.get_by_role("button", name="Close") + + def metric_selector(self) -> Locator: + """Present only when the caller opened more than one metric at once.""" + return self.dialog.get_by_role("combobox", name="Metric") + + def empty_state(self) -> Locator: + return self.dialog.get_by_text("No supporting data for this selection") + + +class GroupDialog: + def __init__(self, page: Page, title: str) -> None: + self.page = page + self.title = title + self.dialog = page.get_by_role("dialog", name=title) + + def close(self) -> Locator: + return self.dialog.get_by_role("button", name="Close") + + def card_actions(self, label: str) -> Locator: + return self.dialog.get_by_role("button", name=f"More actions for {label}") + + def any_card_actions(self) -> Locator: + return self.dialog.get_by_role("button", name=re.compile(r"^More actions for ")).first + + def card_label(self, actions: Locator) -> str: + """The metric a card is about, read from the overflow button it owns. + + The label belongs to the server, so a test that needs it to name the + evidence dialog reads it from the view rather than restating it. + """ + name = actions.get_attribute("aria-label") or "" + return name.removeprefix("More actions for ") + + def supporting_data_item(self) -> Locator: + return self.page.get_by_role("menuitem", name=SUPPORTING_DATA) + + def evidence_for(self, metric: str | re.Pattern[str]) -> MetricEvidenceDialog: + """The evidence dialog this group's widgets open, by the name it takes.""" + return MetricEvidenceDialog(self.page, metric) + + def open_card_evidence(self, actions: Locator) -> MetricEvidenceDialog: + label = self.card_label(actions) + actions.click() + self.supporting_data_item().click() + return MetricEvidenceDialog(self.page, label) + + def evidence_button(self) -> Locator: + return self.dialog.get_by_role("button", name=SUPPORTING_DATA).first + + def timeseries_block(self) -> Locator: + """The card that owns the first timeseries block's header controls.""" + return self.evidence_button().locator('xpath=ancestor::*[@data-slot="card"][1]') + + def block_metric_heading(self) -> Locator: + """The block's own heading, rendered when it carries a single metric.""" + return self.timeseries_block().get_by_role("heading") + + def table_view(self) -> Locator: + return self.timeseries_block().get_by_role("button", name="Table view") + + def chart_view(self) -> Locator: + return self.timeseries_block().get_by_role("button", name="Chart view") + + def block_table(self) -> Locator: + return self.timeseries_block().get_by_role("table") + + def open_bucket_evidence(self, metric: str) -> MetricEvidenceDialog: + """Evidence behind the table's first body row — one time bucket.""" + body = self.block_table().get_by_role("rowgroup").nth(1) + body.get_by_role("row").first.get_by_role("button").first.click() + return MetricEvidenceDialog(self.page, metric) + + def open_total_row_evidence(self, metric: str) -> MetricEvidenceDialog: + """Evidence behind the table's Total row — the whole period, not a bucket. + + The footer rather than a body row on purpose: a total passes no bucket to + the selection, so it is the one cell whose period is the block's own. + """ + footer = self.block_table().get_by_role("rowgroup").last + footer.get_by_role("row").first.get_by_role("button").first.click() + return MetricEvidenceDialog(self.page, metric) diff --git a/tests/stand/ui/pages/person_view.py b/tests/stand/ui/pages/person_view.py index a34b0bf37..3d57c582d 100644 --- a/tests/stand/ui/pages/person_view.py +++ b/tests/stand/ui/pages/person_view.py @@ -12,31 +12,23 @@ from __future__ import annotations -import re from urllib.parse import quote from playwright.sync_api import Locator, Page +from .group_dialog import GroupDialog, MetricEvidenceDialog -class MetricEvidenceDialog: - def __init__(self, page: Page, metric: str) -> None: - self.page = page - self.dialog = page.get_by_role("dialog", name=metric) - - def table(self) -> Locator: - return self.dialog.get_by_role("table") - - def export(self) -> Locator: - return self.dialog.get_by_role("button", name="Export") - def copy_ref(self) -> Locator: - return self.dialog.get_by_role("button", name=re.compile(r"^Copy ")) +class GitOutputDetails(GroupDialog): + """The Git group's dialog, plus the repository timeseries it leads with. + Everything general lives in `GroupDialog`; what is here is specific to the + one block this group opens on — a table by repository, and the commit column + inside it. + """ -class GitOutputDetails: def __init__(self, page: Page) -> None: - self.page = page - self.dialog = page.get_by_role("dialog", name="Git output") + super().__init__(page, "Git output") def repository_table(self) -> Locator: return self.dialog.get_by_role("table").filter(has_text="PRs merged") @@ -61,9 +53,6 @@ def export(self) -> Locator: def metric_selector(self) -> Locator: return self.dialog.get_by_role("combobox", name="Metric").filter(has_text="Commits") - def close(self) -> Locator: - return self.dialog.get_by_role("button", name="Close") - def open_first_commit_bucket(self) -> MetricEvidenceDialog: table = self.repository_table() data_row = table.get_by_role("rowgroup").nth(1).get_by_role("row").first @@ -102,6 +91,10 @@ def empty_domain_card(self, label: str) -> Locator: has=self.page.get_by_text(label, exact=True) ) + def open_domain(self, label: str) -> GroupDialog: + self.populated_domain_card(label).click() + return GroupDialog(self.page, label) + def open_git_output(self) -> GitOutputDetails: self.populated_domain_card("Git output").click() return GitOutputDetails(self.page) diff --git a/tests/stand/ui/pages/team_view.py b/tests/stand/ui/pages/team_view.py index 80253205e..a731a56a6 100644 --- a/tests/stand/ui/pages/team_view.py +++ b/tests/stand/ui/pages/team_view.py @@ -14,6 +14,8 @@ from playwright.sync_api import Locator, Page +from .group_dialog import GroupDialog, MetricEvidenceDialog + class TeamView: def __init__(self, page: Page) -> None: @@ -80,3 +82,22 @@ def any_recorded_metric_cell(self, display_name: str) -> Locator: def domain_card(self, label: str) -> Locator: return self.page.get_by_role("button", name=f"Open {label} details") + + def open_domain(self, label: str) -> GroupDialog: + self.domain_card(label).click() + return GroupDialog(self.page, label) + + def cell_metric_label(self, cell: Locator, display_name: str) -> str: + """Which metric a heatmap cell is about, read from the cell itself. + + The accessible name composes the member, the metric label and the value, + and the label is the server's wording — the same reason `team_heading` + matches a substring instead of rebuilding the copy. + """ + name = cell.get_attribute("aria-label") or "" + return name.removeprefix(f"{display_name} — ").split(":", 1)[0] + + def open_cell_evidence(self, cell: Locator, display_name: str) -> MetricEvidenceDialog: + label = self.cell_metric_label(cell, display_name) + cell.click() + return MetricEvidenceDialog(self.page, label) diff --git a/tests/stand/ui/test_collaboration_card_evidence.py b/tests/stand/ui/test_collaboration_card_evidence.py new file mode 100644 index 000000000..1af810350 --- /dev/null +++ b/tests/stand/ui/test_collaboration_card_evidence.py @@ -0,0 +1,73 @@ +"""Journey 6 — supporting data from a metric card's overflow menu. + +Why this is a browser test and not an API test: the analytics suite reconciles +every metric's evidence against its value directly, so nothing about the payload +needs a browser. What needs one is the affordance. A group whose drilldown body +carries no timeseries block — Collaboration is the only one — reaches evidence +solely through a card's `⋯` menu, and that menu is the whole path: the card has to +be told the metric supports drilldown, the item has to build a selection from the +card's own metric, and the dialog has to open over the group dialog already on +screen rather than replacing it. + +The shape asserted here is the summary grain's, and it is the complement of the +Git journey's: a day and a number, no record reference to copy, because a +summary-grain source has no per-event rows to point at. +""" + +from __future__ import annotations + +import re +from collections.abc import Callable +from pathlib import Path + +import pytest +from insight_stand import PersonaSession +from playwright.sync_api import Page, expect + +from .flows import sign_in +from .pages.person_view import PersonView + +#: A chat metric rather than a file or meeting one: the label is the server's, +#: and this is the one the group's card leads with. +MESSAGES_SENT = "Messages Sent" + + +@pytest.mark.requires_seed("dev_lead") +def test_collaboration_card_menu_opens_and_exports_supporting_data( + page: Page, + base_url: str, + session_for: Callable[[str], PersonaSession], + tmp_path: Path, +) -> None: + persona = session_for("dev_lead") + sign_in(page, base_url, persona) + + person = PersonView(page) + person.go(persona.person.uuid) + expect(person.person_heading(persona.person.display_name)).to_be_visible() + + collaboration = person.open_domain("Collaboration") + expect(collaboration.dialog).to_be_visible() + + actions = collaboration.card_actions(MESSAGES_SENT) + expect(actions).to_be_visible() + evidence = collaboration.open_card_evidence(actions) + expect(evidence.dialog).to_be_visible() + + table = evidence.table() + expect(table).to_be_visible() + expect(evidence.column_header("Date")).to_be_visible() + expect(evidence.column_header("Value")).to_be_visible() + expect(table).to_have_attribute("aria-rowcount", re.compile(r"^[1-9]\d*$")) + assert evidence.copy_ref().count() == 0, ( + "summary-grain evidence has no per-record reference, so nothing should offer to copy one" + ) + + evidence.export().click() + with page.expect_download() as download_info: + page.get_by_role("menuitem", name="CSV", exact=True).click() + download = download_info.value + assert download.suggested_filename.endswith(".csv") + destination = tmp_path / download.suggested_filename + download.save_as(destination) + assert destination.stat().st_size > 0 diff --git a/tests/stand/ui/test_team_grid_cell_evidence.py b/tests/stand/ui/test_team_grid_cell_evidence.py new file mode 100644 index 000000000..2486f30ed --- /dev/null +++ b/tests/stand/ui/test_team_grid_cell_evidence.py @@ -0,0 +1,78 @@ +"""Journey 7 — supporting data behind one member's cell on the team heatmap. + +Why this is a browser test and not an API test: the API takes a person, a metric +and a period, and the analytics suite already asks it about every metric. What it +cannot show is that a lead looking at somebody else's cell gets THAT person's +evidence. The heatmap builds its selection from the cell — a different person per +row and a different metric per column, on a surface whose scope is the team rather +than the signed-in user — and a selection built from the wrong axis would still +answer 200 with somebody's rows. + +The member and the cell both come from the manifest and from the view: the roster +decides who is on the team, and the cell's own accessible name decides which +metric the dialog should be about. +""" + +from __future__ import annotations + +import re +from collections.abc import Callable + +import pytest +from insight_stand import Manifest, PersonaSession +from playwright.sync_api import Page, expect + +from .evidence_requests import evidence_selection +from .flows import sign_in +from .pages.person_view import PersonView +from .pages.team_view import TeamView + + +@pytest.mark.requires_seed("dev_lead") +def test_team_heatmap_cell_opens_that_members_supporting_data( + page: Page, + base_url: str, + session_for: Callable[[str], PersonaSession], + stand_manifest: Manifest, +) -> None: + persona = session_for("dev_lead") + lead = persona.person + reports = sorted( + (p for p in stand_manifest.personas if p.team == lead.team and p.role == "ic"), + key=lambda p: p.display_name, + ) + assert reports, ( + f"the manifest places nobody under {lead.display_name} on team {lead.team!r}, " + "so this test would have no cell to open" + ) + member = reports[0] + + sign_in(page, base_url, persona) + + personal = PersonView(page) + personal.go(lead.uuid) + expect(personal.person_heading(lead.display_name)).to_be_visible() + personal.team_view_switch().click() + + team = TeamView(page) + expect(page).to_have_url(f"{base_url}{TeamView.path(lead.uuid)}") + expect(team.metrics_overview()).to_be_visible() + + cell = team.any_recorded_metric_cell(member.display_name) + expect(cell).to_be_visible() + with evidence_selection(page) as selection: + evidence = team.open_cell_evidence(cell, member.display_name) + expect(evidence.dialog).to_be_visible() + + assert selection["entity"]["id"] == member.uuid, ( + f"the cell belongs to {member.display_name}, and the request asked about " + f"{selection['entity']}" + ) + + table = evidence.table() + expect(table).to_be_visible() + expect(evidence.column_header("Date")).to_be_visible() + expect(table).to_have_attribute("aria-rowcount", re.compile(r"^[1-9]\d*$")) + assert evidence.metric_selector().count() == 0, ( + "a cell names one metric, so its dialog must not offer a metric to choose" + ) diff --git a/tests/stand/ui/test_timeseries_block_evidence.py b/tests/stand/ui/test_timeseries_block_evidence.py new file mode 100644 index 000000000..ba671c9d8 --- /dev/null +++ b/tests/stand/ui/test_timeseries_block_evidence.py @@ -0,0 +1,157 @@ +"""Journey 8 — supporting data from a timeseries block, whole-block and total. + +Why this is a browser test and not an API test: the endpoint answers about one +metric at a time, so the two things exercised here exist only in the SPA. A block +that charts several metrics together opens ONE dialog for all of them and lets the +reader switch between them inside it — a branch with its own title, its own +selector and its own query per target. And a table's Total cell asks for the +block's whole period rather than a bucket, which is the one selection whose period +is not the cell's own row. + +Both groups the sweep covers here are picked for their block shape rather than +their domain: one block carries several metrics, the other carries one, and the +selector must appear in exactly the first case. +""" + +from __future__ import annotations + +import re +from collections.abc import Callable + +import pytest +from insight_stand import PersonaSession +from playwright.sync_api import Page, expect + +from .evidence_requests import evidence_selection +from .flows import sign_in +from .pages.person_view import PersonView + +#: The first column of the Task delivery table, so the Total cell opened below +#: names this metric. The label is the server's. +TASKS_CLOSED = "Tasks closed" + +#: A dialog opened for several metrics at once titles itself with all of their +#: labels joined, and that joining is the assertion — no single label would do. +_JOINED_TITLE = re.compile(r" & ") + + +@pytest.mark.requires_seed("dev_lead") +def test_multi_metric_block_offers_every_metric_in_one_dialog( + page: Page, + base_url: str, + session_for: Callable[[str], PersonaSession], +) -> None: + persona = session_for("dev_lead") + sign_in(page, base_url, persona) + + person = PersonView(page) + person.go(persona.person.uuid) + expect(person.person_heading(persona.person.display_name)).to_be_visible() + + tasks = person.open_domain("Task delivery") + expect(tasks.dialog).to_be_visible() + with evidence_selection(page) as opened_selection: + tasks.evidence_button().click() + + evidence = tasks.evidence_for(_JOINED_TITLE) + expect(evidence.dialog).to_be_visible() + + selector = evidence.metric_selector() + expect(selector).to_be_visible() + opened_with = selector.inner_text().strip() + + selector.click() + options = page.get_by_role("option") + labels = [label.strip() for label in options.all_inner_texts()] + assert len(labels) > 1, f"a joined dialog must list every metric it was opened for: {labels}" + other = next(label for label in labels if label != opened_with) + + with evidence_selection(page) as switched_selection: + page.get_by_role("option", name=other, exact=True).click() + expect(selector).to_contain_text(other) + expect(evidence.table()).to_be_visible() + expect(evidence.column_header("Date")).to_be_visible() + + assert switched_selection["metric_key"] != opened_selection["metric_key"], ( + f"choosing {other} left the dialog querying {opened_selection['metric_key']}, so the " + "table below the selector is the previous metric's" + ) + + +@pytest.mark.requires_seed("dev_lead") +def test_single_metric_block_opens_that_metrics_dialog( + page: Page, + base_url: str, + session_for: Callable[[str], PersonaSession], +) -> None: + persona = session_for("dev_lead") + sign_in(page, base_url, persona) + + person = PersonView(page) + person.go(persona.person.uuid) + expect(person.person_heading(persona.person.display_name)).to_be_visible() + + ai = person.open_domain("AI adoption") + expect(ai.dialog).to_be_visible() + + heading = ai.block_metric_heading().first + expect(heading).to_be_visible() + metric = heading.inner_text().strip() + + ai.evidence_button().click() + evidence = ai.evidence_for(metric) + expect(evidence.dialog).to_be_visible() + expect(evidence.table()).to_be_visible() + expect(evidence.column_header("Date")).to_be_visible() + assert evidence.metric_selector().count() == 0, ( + f"{metric} is the block's only metric, so its dialog must offer nothing to switch to" + ) + + +@pytest.mark.requires_seed("dev_lead") +def test_table_total_opens_supporting_data_for_the_whole_period( + page: Page, + base_url: str, + session_for: Callable[[str], PersonaSession], +) -> None: + """A total is the one cell whose period is the block's, not a row's. + + Asserted against a body row from the same table rather than against a date + typed here: the total's period has to contain the bucket's and to be wider + than it, which is exactly what sending a bucket for the total would break. + """ + persona = session_for("dev_lead") + sign_in(page, base_url, persona) + + person = PersonView(page) + person.go(persona.person.uuid) + expect(person.person_heading(persona.person.display_name)).to_be_visible() + + tasks = person.open_domain("Task delivery") + expect(tasks.dialog).to_be_visible() + tasks.table_view().click() + expect(tasks.block_table()).to_be_visible() + + with evidence_selection(page) as bucket_selection: + bucket = tasks.open_bucket_evidence(TASKS_CLOSED) + expect(bucket.dialog).to_be_visible() + bucket.close().click() + expect(bucket.dialog).not_to_be_visible() + + with evidence_selection(page) as total_selection: + evidence = tasks.open_total_row_evidence(TASKS_CLOSED) + expect(evidence.dialog).to_be_visible() + + table = evidence.table() + expect(table).to_be_visible() + expect(evidence.column_header("Ref")).to_be_visible() + expect(evidence.column_header("Date")).to_be_visible() + expect(table).to_have_attribute("aria-rowcount", re.compile(r"^[1-9]\d*$")) + + total_period = total_selection["period"] + bucket_period = bucket_selection["period"] + assert total_period["from"] <= bucket_period["from"], (total_period, bucket_period) + assert total_period["to"] >= bucket_period["to"], (total_period, bucket_period) + assert total_period != bucket_period, ( + f"the Total row asked for {total_period}, the same period as a single bucket" + )