From f86bbe82d5489968958709630a5c3984a9a5b2e3 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Mon, 17 Nov 2025 15:59:57 -0800 Subject: [PATCH 1/4] test(table): remove conditionals from TableChart tests and improve ARIA validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove conditional logic from table tests following the principle that tests should not contain conditionals. Split 2 tests with if statements into 4 unconditional tests with explicit assertions. Changes: - Remove if(labelledBy) guards from ARIA validation tests - Split "header IDs" tests to separate ARIA validation into new tests - Add complete coverage check: all tbody td cells must have aria-labelledby - Add empty table guard to catch regressions (prevents silent 0===0 pass) - Replace non-null assertions with explicit truthy checks for clearer errors - Scope queries to tbody td (excludes footer cells which legitimately lack labels) Benefits: - No hidden code paths - all assertions always run - Better error messages when attributes missing - 100% data cell coverage for accessibility - Catches empty table regressions - Type-safe without suppressions All 47 tests passing with improved coverage and clarity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../test/TableChart.test.tsx | 92 +++++++++++++------ 1 file changed, 65 insertions(+), 27 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index 83de3752be37..45c7158f0600 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -602,7 +602,7 @@ describe('plugin-chart-table', () => { // Uses originalLabel (e.g., "metric_1") which is sanitized for CSS safety const props = transformProps(testData.comparison); - const { container } = render(); + render(); const headers = screen.getAllByRole('columnheader'); @@ -632,24 +632,41 @@ describe('plugin-chart-table', () => { // IDs should only contain valid characters: alphanumeric, underscore, hyphen expect(header.id).toMatch(/^header-[a-zA-Z0-9_-]+$/); }); + }); - // CRITICAL: Verify ALL cells reference valid headers (no broken ARIA) + test('should validate ARIA references for time-comparison table cells', () => { + // Test that ALL cells with aria-labelledby have valid references + // This is critical for screen reader accessibility + const props = transformProps(testData.comparison); + + const { container } = render(); + + const allCells = container.querySelectorAll('tbody td'); const cellsWithLabels = container.querySelectorAll( - 'td[aria-labelledby]', + 'tbody td[aria-labelledby]', ); + + // First assertion: Table must render data cells (catch empty table regression) + expect(allCells.length).toBeGreaterThan(0); + + // Second assertion: ALL data cells must have aria-labelledby (no unlabeled cells) + expect(cellsWithLabels.length).toBe(allCells.length); + + // Third assertion: ALL aria-labelledby values should be valid cellsWithLabels.forEach(cell => { const labelledBy = cell.getAttribute('aria-labelledby'); - if (labelledBy) { - // Check that the ID doesn't contain spaces (would be interpreted as multiple IDs) - expect(labelledBy).not.toMatch(/\s/); - // Check that the ID doesn't contain special characters - expect(labelledBy).not.toMatch(/[%#△]/); - // Verify the referenced header actually exists - const referencedHeader = container.querySelector( - `#${CSS.escape(labelledBy)}`, - ); - expect(referencedHeader).toBeTruthy(); - } + expect(labelledBy).not.toBeNull(); + expect(labelledBy).not.toBe(''); + const labelledByValue = labelledBy as string; + // Check that the ID doesn't contain spaces (would be interpreted as multiple IDs) + expect(labelledByValue).not.toMatch(/\s/); + // Check that the ID doesn't contain special characters + expect(labelledByValue).not.toMatch(/[%#△]/); + // Verify the referenced header actually exists + const referencedHeader = container.querySelector( + `#${CSS.escape(labelledByValue)}`, + ); + expect(referencedHeader).toBeTruthy(); }); }); @@ -711,24 +728,45 @@ describe('plugin-chart-table', () => { // IDs should only contain valid CSS selector characters expect(header.id).toMatch(/^header-[a-zA-Z0-9_-]+$/); }); + }); - // Test 6: Verify ALL cells reference valid headers (no broken ARIA) + test('should validate ARIA references for regular table cells', () => { + // Test that ALL cells with aria-labelledby have valid references + // This is critical for screen reader accessibility + const props = transformProps(testData.advanced); + + const { container } = render( + ProviderWrapper({ + children: , + }), + ); + + const allCells = container.querySelectorAll('tbody td'); const cellsWithLabels = container.querySelectorAll( - 'td[aria-labelledby]', + 'tbody td[aria-labelledby]', ); + + // First assertion: Table must render data cells (catch empty table regression) + expect(allCells.length).toBeGreaterThan(0); + + // Second assertion: ALL data cells must have aria-labelledby (no unlabeled cells) + expect(cellsWithLabels.length).toBe(allCells.length); + + // Third assertion: ALL aria-labelledby values should be valid cellsWithLabels.forEach(cell => { const labelledBy = cell.getAttribute('aria-labelledby'); - if (labelledBy) { - // Verify no spaces (would be interpreted as multiple IDs) - expect(labelledBy).not.toMatch(/\s/); - // Verify no special characters - expect(labelledBy).not.toMatch(/[%#△]/); - // Verify the referenced header actually exists - const referencedHeader = container.querySelector( - `#${CSS.escape(labelledBy)}`, - ); - expect(referencedHeader).toBeTruthy(); - } + expect(labelledBy).not.toBeNull(); + expect(labelledBy).not.toBe(''); + const labelledByValue = labelledBy as string; + // Verify no spaces (would be interpreted as multiple IDs) + expect(labelledByValue).not.toMatch(/\s/); + // Verify no special characters + expect(labelledByValue).not.toMatch(/[%#△]/); + // Verify the referenced header actually exists + const referencedHeader = container.querySelector( + `#${CSS.escape(labelledByValue)}`, + ); + expect(referencedHeader).toBeTruthy(); }); }); From 8945ee141ab8a27695badc03688febb9e521199a Mon Sep 17 00:00:00 2001 From: Joe Li Date: Mon, 17 Nov 2025 17:35:08 -0800 Subject: [PATCH 2/4] test(table): tighten aria label assertions --- .../plugins/plugin-chart-table/test/TableChart.test.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index 45c7158f0600..bcf6cf37bc79 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -655,8 +655,7 @@ describe('plugin-chart-table', () => { // Third assertion: ALL aria-labelledby values should be valid cellsWithLabels.forEach(cell => { const labelledBy = cell.getAttribute('aria-labelledby'); - expect(labelledBy).not.toBeNull(); - expect(labelledBy).not.toBe(''); + expect(labelledBy).toEqual(expect.stringMatching(/\S/)); const labelledByValue = labelledBy as string; // Check that the ID doesn't contain spaces (would be interpreted as multiple IDs) expect(labelledByValue).not.toMatch(/\s/); @@ -755,8 +754,7 @@ describe('plugin-chart-table', () => { // Third assertion: ALL aria-labelledby values should be valid cellsWithLabels.forEach(cell => { const labelledBy = cell.getAttribute('aria-labelledby'); - expect(labelledBy).not.toBeNull(); - expect(labelledBy).not.toBe(''); + expect(labelledBy).toEqual(expect.stringMatching(/\S/)); const labelledByValue = labelledBy as string; // Verify no spaces (would be interpreted as multiple IDs) expect(labelledByValue).not.toMatch(/\s/); From 918ace5aa5d5d706a3404a756a39d97e33628909 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Tue, 18 Nov 2025 10:59:03 -0800 Subject: [PATCH 3/4] test(table): guard aria-labelledby from null --- .../plugins/plugin-chart-table/test/TableChart.test.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index bcf6cf37bc79..5e1e1fdb7417 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -655,6 +655,7 @@ describe('plugin-chart-table', () => { // Third assertion: ALL aria-labelledby values should be valid cellsWithLabels.forEach(cell => { const labelledBy = cell.getAttribute('aria-labelledby'); + expect(labelledBy).not.toBeNull(); expect(labelledBy).toEqual(expect.stringMatching(/\S/)); const labelledByValue = labelledBy as string; // Check that the ID doesn't contain spaces (would be interpreted as multiple IDs) @@ -754,6 +755,7 @@ describe('plugin-chart-table', () => { // Third assertion: ALL aria-labelledby values should be valid cellsWithLabels.forEach(cell => { const labelledBy = cell.getAttribute('aria-labelledby'); + expect(labelledBy).not.toBeNull(); expect(labelledBy).toEqual(expect.stringMatching(/\S/)); const labelledByValue = labelledBy as string; // Verify no spaces (would be interpreted as multiple IDs) From 9407d8b5d81da4c758b35ebeedab6cdae8b3794a Mon Sep 17 00:00:00 2001 From: Joe Li Date: Tue, 18 Nov 2025 13:36:28 -0800 Subject: [PATCH 4/4] test(table): extract aria validation helper --- .../test/TableChart.test.tsx | 83 +++++++------------ 1 file changed, 29 insertions(+), 54 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index 5e1e1fdb7417..1f19c7944fd7 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -25,6 +25,33 @@ import DateWithFormatter from '../src/utils/DateWithFormatter'; import testData from './testData'; import { ProviderWrapper } from './testHelpers'; +const expectValidAriaLabels = (container: HTMLElement) => { + const allCells = container.querySelectorAll('tbody td'); + const cellsWithLabels = container.querySelectorAll( + 'tbody td[aria-labelledby]', + ); + + // Table must render data cells (catch empty table regression) + expect(allCells.length).toBeGreaterThan(0); + + // ALL data cells must have aria-labelledby (no unlabeled cells) + expect(cellsWithLabels.length).toBe(allCells.length); + + // ALL aria-labelledby values should be valid + cellsWithLabels.forEach(cell => { + const labelledBy = cell.getAttribute('aria-labelledby'); + expect(labelledBy).not.toBeNull(); + expect(labelledBy).toEqual(expect.stringMatching(/\S/)); + const labelledByValue = labelledBy as string; + expect(labelledByValue).not.toMatch(/\s/); + expect(labelledByValue).not.toMatch(/[%#△]/); + const referencedHeader = container.querySelector( + `#${CSS.escape(labelledByValue)}`, + ); + expect(referencedHeader).toBeTruthy(); + }); +}; + test('sanitizeHeaderId should sanitize percent sign', () => { expect(sanitizeHeaderId('%pct_nice')).toBe('percentpct_nice'); }); @@ -641,33 +668,7 @@ describe('plugin-chart-table', () => { const { container } = render(); - const allCells = container.querySelectorAll('tbody td'); - const cellsWithLabels = container.querySelectorAll( - 'tbody td[aria-labelledby]', - ); - - // First assertion: Table must render data cells (catch empty table regression) - expect(allCells.length).toBeGreaterThan(0); - - // Second assertion: ALL data cells must have aria-labelledby (no unlabeled cells) - expect(cellsWithLabels.length).toBe(allCells.length); - - // Third assertion: ALL aria-labelledby values should be valid - cellsWithLabels.forEach(cell => { - const labelledBy = cell.getAttribute('aria-labelledby'); - expect(labelledBy).not.toBeNull(); - expect(labelledBy).toEqual(expect.stringMatching(/\S/)); - const labelledByValue = labelledBy as string; - // Check that the ID doesn't contain spaces (would be interpreted as multiple IDs) - expect(labelledByValue).not.toMatch(/\s/); - // Check that the ID doesn't contain special characters - expect(labelledByValue).not.toMatch(/[%#△]/); - // Verify the referenced header actually exists - const referencedHeader = container.querySelector( - `#${CSS.escape(labelledByValue)}`, - ); - expect(referencedHeader).toBeTruthy(); - }); + expectValidAriaLabels(container); }); test('should set meaningful header IDs for regular table columns', () => { @@ -741,33 +742,7 @@ describe('plugin-chart-table', () => { }), ); - const allCells = container.querySelectorAll('tbody td'); - const cellsWithLabels = container.querySelectorAll( - 'tbody td[aria-labelledby]', - ); - - // First assertion: Table must render data cells (catch empty table regression) - expect(allCells.length).toBeGreaterThan(0); - - // Second assertion: ALL data cells must have aria-labelledby (no unlabeled cells) - expect(cellsWithLabels.length).toBe(allCells.length); - - // Third assertion: ALL aria-labelledby values should be valid - cellsWithLabels.forEach(cell => { - const labelledBy = cell.getAttribute('aria-labelledby'); - expect(labelledBy).not.toBeNull(); - expect(labelledBy).toEqual(expect.stringMatching(/\S/)); - const labelledByValue = labelledBy as string; - // Verify no spaces (would be interpreted as multiple IDs) - expect(labelledByValue).not.toMatch(/\s/); - // Verify no special characters - expect(labelledByValue).not.toMatch(/[%#△]/); - // Verify the referenced header actually exists - const referencedHeader = container.querySelector( - `#${CSS.escape(labelledByValue)}`, - ); - expect(referencedHeader).toBeTruthy(); - }); + expectValidAriaLabels(container); }); test('render cell bars properly, and only when it is toggled on in both regular and percent metrics', () => {