sadpandajoe commented on code in PR #37378:
URL: https://github.com/apache/superset/pull/37378#discussion_r2775441111
##########
superset-frontend/src/features/themes/ThemeModal.test.tsx:
##########
@@ -37,6 +38,27 @@ jest.mock('src/dashboard/util/permissionUtils', () => ({
isUserAdmin: jest.fn(() => true),
}));
+// Mock JsonEditor to avoid direct DOM manipulation in tests
+jest.mock('@superset-ui/core/components/AsyncAceEditor', () => ({
+ ...jest.requireActual('@superset-ui/core/components/AsyncAceEditor'),
+ JsonEditor: ({
+ onChange,
+ value,
+ readOnly,
+ }: {
+ onChange: (value: string) => void;
+ value: string;
+ readOnly?: boolean;
+ }) => (
+ <textarea
+ data-test="json-editor"
Review Comment:
The testing library is configured to use `data-test` attribute. See
`spec/helpers/setup.ts` line 27:
```typescript
testIdAttribute: 'data-test',
```
So `screen.getByTestId('json-editor')` queries for
`data-test="json-editor"`, which matches the mock. This is correct and the
tests pass.
##########
superset-frontend/src/features/themes/ThemeModal.test.tsx:
##########
@@ -304,10 +357,52 @@ test('validates JSON format and enables save button',
async () => {
const nameInput = screen.getByPlaceholderText('Enter theme name');
await userEvent.type(nameInput, 'Test Theme');
+ await addValidJsonData();
- const saveButton = await screen.findByRole('button', { name: 'Add' });
+ // Wait for validation to complete and button to become enabled
+ await waitFor(
+ () => {
+ const saveButton = screen.getByRole('button', { name: 'Add' });
+ expect(saveButton).toBeEnabled();
+ },
+ { timeout: 10000 },
+ );
+});
+
+test('warnings do not block save - unknown tokens allow save with warnings',
async () => {
+ // First verify the test data actually produces warnings (not errors)
+ const testTheme = {
+ token: { colorPrimary: '#1890ff', unknownTokenName: 'value' },
+ };
+ const validationResult = validateTheme(testTheme);
+ expect(validationResult.valid).toBe(true); // No errors
+ expect(validationResult.warnings.length).toBeGreaterThan(0); // Has warnings
+ expect(validationResult.warnings[0].tokenName).toBe('unknownTokenName');
Review Comment:
This is a minor nitpick. The test theme has only ONE unknown token
(`unknownTokenName`), so `warnings[0]` will always be that token. Using
`.some()` would be marginally more defensive, but the test is correct as-is and
the assertion clearly documents what we're testing.
If we had multiple unknown tokens, using `.some()` would make sense.
##########
superset-frontend/src/features/themes/ThemeModal.test.tsx:
##########
@@ -418,6 +513,19 @@ test('saves changes when clicking Save button in unsaved
changes alert', async (
const nameInput = screen.getByPlaceholderText('Enter theme name');
await userEvent.type(nameInput, 'Modified Theme');
+ await addValidJsonData();
+
+ // Wait for validation to complete before canceling
+ await waitFor(
+ () => {
+ const addButton = screen.getByRole('button', { name: 'Add' });
+ expect(addButton).toBeEnabled();
+ },
+ { timeout: 10000 },
+ );
+
+ // Give extra time for all state updates to complete
+ await new Promise(resolve => setTimeout(resolve, 500));
Review Comment:
The 500ms delay is a pragmatic workaround for async state updates in React.
While not ideal, removing it could cause flakiness. The test has explicit
`waitFor` calls for DOM state, but some internal state updates may not be
captured by DOM assertions.
This is a test smell but not a bug. The overall test timeout is 30 seconds,
so 500ms is minimal overhead.
##########
superset-frontend/src/theme/tests/ThemeController.test.ts:
##########
@@ -833,6 +833,206 @@ test('ThemeController handles theme application errors',
() => {
fallbackSpy.mockRestore();
});
+test('ThemeController constructor recovers from corrupted stored theme', () =>
{
+ // Simulate corrupted dev theme override in storage
+ const corruptedTheme = { token: { colorPrimary: '#ff0000' } };
+ mockLocalStorage.getItem.mockImplementation((key: string) => {
+ if (key === 'superset-dev-theme-override') {
+ return JSON.stringify(corruptedTheme);
+ }
+ return null;
+ });
+
+ // Mock Theme.fromConfig to return object with toSerializedConfig
+ mockThemeFromConfig.mockReturnValue({
+ ...mockThemeObject,
+ toSerializedConfig: () => corruptedTheme,
+ });
+
+ // First call throws (corrupted theme), second call succeeds (fallback)
+ let callCount = 0;
+ mockSetConfig.mockImplementation(() => {
+ callCount += 1;
+ if (callCount === 1) {
+ throw new Error('Invalid theme configuration');
+ }
+ });
+
+ const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+ // Should not throw - constructor should recover
+ const controller = createController();
+
+ // Verify recovery happened
+ expect(consoleWarnSpy).toHaveBeenCalledWith(
+ 'Failed to apply stored theme, clearing invalid overrides:',
+ expect.any(Error),
+ );
+
+ // Verify invalid overrides were cleared from storage
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-dev-theme-override',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-crud-theme-id',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-applied-theme-id',
+ );
+
+ // Verify controller is in a valid state
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+
+ consoleWarnSpy.mockRestore();
Review Comment:
**Valid feedback - fixed!** ✅
The local `consoleWarnSpy` with `mockRestore()` was interfering with the
shared `consoleSpy`. Updated the test to use the shared spy instead, avoiding
the restore issue.
--
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]
