EnxDev commented on PR #41389: URL: https://github.com/apache/superset/pull/41389#issuecomment-4834284227
## EnxDev's Review Agent — apache/superset#41389 · HEAD 301e149 **comment** — test-only PR (draft); the tests are correct and assert real behavior, but the `fix:` title and `as any` casts are worth tidying. Test-only change: one new Jest file, no production code. Verified each test against `getColorFunction`/`getOpacity` in `getColorFormatters.ts` — they assert genuine behavior, not smoke tests. Ranged ops (cutoff=`targetValueLeft`, extreme=`targetValueRight`) yield distinct proportional opacities for 10/30/50 (≈0.19/0.52/0.84), string bounds resolve via `parseFloat` in `getOpacity:53`, and `useGradient=false` returns the solid `colorScheme` (`getColorFormatters.ts:274`). All Jest shards green; codecov reports no source-line delta (expected — test-only). ### 🟡 Should-fix - **PR title** — `fix(ag-grid):` but the PR changes no behavior (the body says the color math is already correct and this only adds tests). Use `test(...)`; scope `ag-grid` is also off since the code lives in `superset-ui-chart-controls`. pr-lint passes either way, but `fix` with no fix is misleading in the changelog. - **`getColorFormatters.gradient.test.ts:23-39` (and each block)** — the config is cast `as any`, which defeats the type-checker. A rename of `useGradient`/`targetValueLeft` would let these "regression guards" keep compiling green while silently no longer testing the field. Cast to `ConditionalFormattingConfig` (the real `getColorFunction` param type) so the guard also catches shape drift. ### 🔵 Nits - `getColorFormatters.gradient.test.ts:62` — the string-bounds test only asserts `fn(10) !== fn(50)`; mirroring the numeric test's 3-distinct-opacity check would make it a stronger gradient assertion. - Scope note: these tests lock in the *color-math* layer only. The PR is honest that a flat-fill, if still real, likely lives in the control/save path — so green here shouldn't be read as "#107917 covered." ### 🙌 Praise - Honest write-up: couldn't reproduce at the color layer, so adds a regression baseline instead of changing correct code. Uses flat `test()`/`test.each` and a proper license header. <!-- enxdev-review-agent:301e149 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 301e149._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
