[OPIK-7839] [QA] Proposed e2e specs from the #7873 exploration: chart series colours - #7892
Draft
CometActions wants to merge 4 commits into
Draft
[OPIK-7839] [QA] Proposed e2e specs from the #7873 exploration: chart series colours#7892CometActions wants to merge 4 commits into
CometActions wants to merge 4 commits into
Conversation
Series colors are derived from the label alone: `md5(label) % TAG_VARIANTS.length`. Palette: `primary` (#6366f1) sat at ΔE 14.5 from `purple` and 22.7 from `blue` — below what anyone separates in a thin line, and the pair the customer reported. Substituted with a new `ochre` (#8c683f) entry: ΔE 44.1 to its nearest neighbour (`gray`), and it clears the thin-line contrast floor on both grounds (5.04:1 light, 3.72:1 dark). The palette's worst pair is now `gray`/`turquoise` at ΔE 35.6. Substituting rather than extending keeps the palette at ten entries, so the modulo does not shift and only labels that resolved to that slot change color — uniformly, everywhere that label appears (chart series, tag chips, feedback scores). Also stops the hardcoded metric-name map being applied when a breakdown is active: there the line names are group values, not metric names, and four of its nine keys share one violet, so a group named `cost` or `total_tokens` could silently collide with another. Guard rails in lib/colorVariants.test.ts: minimum pairwise ΔE across the palette (fails with `primary/purple ΔE=14.5` if reintroduced), a contrast floor with a documented allowlist for the pre-existing `yellow` (1.92) and `turquoise` (2.43) entries, and a golden assertion that known labels keep the colors they resolve to today, so a future palette edit cannot silently re-color unrelated labels. Identical colors remain possible when the number of series approaches the palette size — asserted explicitly as a documented limitation. That case is answered by the existing manual per-label override (OPIK-3100); reassigning automatically would break the guarantee that a label keeps one color across widgets. Reserving gray for the `Others`/`Unknown` buckets was attempted and dropped: `gray` is itself in the automatic palette, so reserving it made ~1 label in 10 render identical to `Others` — the defect this change exists to remove. Doing it properly needs a tenth distinct color, which is a design dependency tracked separately. v2 chart container only — v1 is slated for removal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… colour dot Design review on the PR asked for two things. Colour: the tenth palette entry is now `purple-dark` #491b7e from the product palette, replacing the ochre I had proposed. It keeps the property the change exists for — nearest neighbour ΔE 39 (`burgundy`, `purple`), well clear of the ΔE 14.5 pair being removed — and is better under colour-vision deficiency than ochre was (protanopia 23.2 vs 18.3). It does not clear the contrast floor on the dark ground: 1.55 against `#121212` where WCAG 1.4.11 asks 3:1, because at L* 23 it sits close to the dark theme's own background. That is recorded in the contrast test rather than hidden. Raising that test to the real 3:1 floor also exposed that `yellow` 1.92, `turquoise` 2.43, `green` 2.54 and `orange` 2.80 already fail it on the light ground, so five of ten entries are now listed as known. Half the palette failing is the finding: one palette cannot serve both themes, which is why two lightness bands are proposed in the follow-up. Colour dot: the 6px indicator was the reason customers reported the override as missing. Its pointer target is now enlarged by a transparent pseudo-element without changing how it looks, and its tooltip opens with no delay so the hint arrives the moment the dot is found. Horizontal growth is limited to 6px because in a chart legend the label sits immediately to the right and owns its own click action. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ction tests - `metricColorMap` -> `METRIC_COLOR_MAP`, matching the sibling `METRIC_CHART_TYPE` and the repo convention for module-level fixed configuration. - Pin the palette length. The golden test samples four labels, so a length change that only moves other labels would have passed it, while re-colouring the whole product. - Pin the design-approved `purple-dark` hex. It is excused from the contrast assertion, so without this the value could drift to something that assertion would have rejected. - Cover the boundaries of `resolveChartColorMap` (empty list, empty-string label) and `resolveHexColor` directly (already-hex passthrough, unknown token passthrough). An empty-string group value returning no colour makes recharts fall back to black. - New `ColorIndicator.test.tsx`: clicking the indicator opens the picker, and does not fire a surrounding click handler — in a legend that handler navigates to filtered traces, so the two must not both fire. Also asserts the pointer target is larger than the 6px dot. Also restores a class order in the chart container that an earlier autofix had reordered without cause; that line is unrelated to this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two specs from the exploration of #7873, under a new tests/dashboards/ directory (dashboards had no e2e coverage at all). - Grouped by Tags: every group takes its tag-palette colour, the five are distinct, and the groups named after metric keys (`traces`, `cost`) are not painted the metric colour. This is the regression the PR fixes. - No grouping: the fixed metric colour map survives, on both the trace count series and the duration percentiles. That is the other side of the same `breakdown ? undefined : METRIC_COLOR_MAP` guard, which would silently drop the fixed colours product-wide if it over-fired. Supporting changes: - backend client: createDashboard / deleteDashboard / getProjectMetricSeries, behind a shared privateFetch helper for the endpoints the pinned TS SDK does not model (getProjectStats now uses it too). - seriesColorsDashboard fixture: seeds tagged traces and a dashboard carrying the three widgets, verifies via the API that the groups really exist before the browser opens, and deletes the dashboard afterwards (dashboards are outside the run-prefix sweep). - dashboard page object: reads legend swatch and SVG series colours as computed hex. - taxonomy: dashboards.configure-widget -> covered. Generated by the release QA side flow; needs review before merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
📋 PR Linter Failed❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the |
Contributor
⏱️ pre-commit per-hook timingNo linted files changed — nothing to run. ⏭️ 42 skipped (no matching files changed)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Where these came from
Exploratory testing of #7873 —
[OPIK-7839] [FE] fix confusable series colors in grouped charts— on that PR's own deployed environment (https://pr-7873.dev.comet.com, build2.2.30-7873-merge-2988). Each flow below was driven by hand there first; these specs are the two that were worth making permanent.This PR targets
awkoy/OPIK-7839/series-colors, notmain. The behaviour it asserts only exists in #7873: onmaina grouped chart still resolves group colours through the fixed metric colour map, so the first spec would fail there — correctly, but noisily. Retarget it tomainonce #7873 merges.Written and run against
24e8b8c804a2e5c326eb72462abbafa4f3faf6da(that branch's head at the time).dashboardshad no e2e specs at all (0/9 capabilities), so this adds thetests/dashboards/directory alongside the supporting fixture, page object and backend-client methods.What the specs assert
Both live in
tests_end_to_end/e2e/tests/dashboards/chart-series-colors.spec.ts, tagged@t2-cuj @area:dashboards @cap:dashboards.configure-widget.1.
A chart grouped by tags colours every group from the tag palette, never from the fixed metric coloursSeeds five tags — two of which (
traces,cost) deliberately collide with keys ofMETRIC_COLOR_MAP— and a widget grouped by Tags, then asserts:traces→#10b981,cost→#491b7e,beta→#8b5cf6,zeta→#06b6d4,iota→#64748b);tracesandcostspecifically are not#8b5cf6, which is what the fixed map would have painted them.Before this PR the same chart drew
traces,costandbetaall in#8b5cf6: three identical lines out of five, with no error anywhere.Verification: passed against
https://pr-7873.dev.comet.com.2.
A chart with no grouping keeps the fixed metric coloursThe other side of the same
colorMap ?? (breakdown ? undefined : METRIC_COLOR_MAP)guard. If it over-fired it would strip the fixed metric colours from every ungrouped chart in the product, and nothing asserts that today. Asserts:#8b5cf6and is not the#10b981its label hashes to (the same tag is seeded, so the hashed colour is a real alternative here, not a hypothetical);p50/p90/p99keep#06b6d4/#bf399e/#8b5cf6.Verification: passed against
https://pr-7873.dev.comet.com.How they were run
Also run, because this change touches shared files (
core/backend/client.ts,fixtures/index.ts):The assertions were also confirmed non-vacuous: flipping one expected hex in the spec made it fail with
Expected "#8b5cf6" / Received "#491b7e", i.e. it really is reading the rendered colour.Two environment notes for whoever reruns this:
*.comet.comhost. The TS SDK requires an API key for anycomet.comhostname, so the run needs a placeholderOPIK_API_KEYeven though the deployment has no auth. Nothing in the spec depends on it.coverage/reconcile.py --checkcould not run here (tests_end_to_end/visual-testsdeps are not installed in this job). Perqa_coverage_reconcile.ymlit is a post-merge job anyway, and the taxonomy edit below is the coverage claim for a reviewer to accept.Notes on the implementation
stroke/--bg-colorattribute. The attribute holds a CSS variable, so asserting on it would pin the variable's name and stay green if the variable itself were redefined.@cap:dashboards.configure-widgetis claimed for "a configured widget's breakdown is honoured" and not for the editor UI — the taxonomy entry says so in itsnote:. Say if you would rather it stayedcovered: falseuntil the editor itself is driven.global-teardownonly sweeps experiments, datasets and projects.data-testidexists on the widget card or the legend colour indicator, so the page object addresses widgets by title and uses the recharts / react-grid-layout class names. Adding two testids would let it drop the CSS selectors; that is a frontend change and is deliberately not bundled here, since the spec had to run against the already-deployed PR build.What was deliberately not written
The exploration produced three candidates. One was dropped:
weakby the exploration itself. Its load-bearing assertion is on an enlarged pointer target's geometry, which a layout change perturbs without anything being broken, and opening the picker is one step from writing workspace-scoped colour state that the run-prefix sweep cannot reclaim.The exploration also declined to propose three further flows, and those stand:
purple-dark(#491b7emeasures 1.49:1 against the dark chart card). That is a legibility judgement needing a designer's call on the intended value, not something a spec should pin unilaterally. Worth one question to the PR author — it is reported in the exploration, not encoded here.cuj-teardown prefix, so it would leak state into later runs.Taxonomy
dashboardsgainsspec_dir, the new spec inspecs:, andconfigure-widgetflips tocovered: true, tier: t2-cujwith a note scoping the claim.Source PR: #7873 · OPIK-7839