This is an automated email from the ASF dual-hosted git repository. jli pushed a commit to branch fix-table-test-conditional-statements in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7e847315286dae75071d186364a7716bbd4682b2 Author: Joe Li <[email protected]> AuthorDate: Mon Nov 17 15:59:57 2025 -0800 test(table): remove conditionals from TableChart tests and improve ARIA validation 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 <[email protected]> --- .../plugin-chart-table/test/TableChart.test.tsx | 90 +++++++++++++++------- 1 file changed, 63 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 83de3752be..f5ff2fa84f 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(<TableChart {...props} sticky={false} />); + render(<TableChart {...props} sticky={false} />); const headers = screen.getAllByRole('columnheader'); @@ -632,24 +632,40 @@ 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(<TableChart {...props} sticky={false} />); + + 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).toBeTruthy(); + 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 +727,44 @@ 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: <TableChart {...props} sticky={false} />, + }), + ); + + 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).toBeTruthy(); + 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(); }); });
