Skip to content

Conversation

rusackas
Copy link
Member

Summary

This PR implements a comprehensive migration strategy to move away from nested describe/it test patterns to flat test() functions, following Kent C. Dodds' "avoid nesting when testing" best practices.

Changes

1. ESLint Rules Enforcement

  • Added jest/consistent-test-it rule to enforce test over it
  • Added no-restricted-globals rule to error on describe and it usage
  • Configured in superset-frontend/src/.eslintrc.json

2. Automated Migrations

  • Auto-migrated all it to test: 194 files updated via npm run lint-fix
  • Flattened simple describe blocks: 5 files with no hooks or complex setup safely migrated

3. Legacy Code Handling

  • Added eslint-disable comments with TODO markers to 243 files containing describe blocks
  • Marked 1,080 existing describe blocks with: // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
  • This allows existing code to work while preventing new usage

Migration Statistics

  • Total test files: 482
  • ittest migration: 100% complete (194 files)
  • describe blocks removed: 5 files (simple cases without hooks)
  • describe blocks with TODOs: 243 files (to be migrated gradually)
  • Files with hooks requiring manual migration: 198

Files Successfully Flattened

These files had simple describe blocks without hooks and were safely migrated:

  • utils/urlUtils.test.ts
  • explore/controlPanels/Separator.test.ts
  • components/ErrorMessage/ErrorAlert.test.tsx
  • components/ModalTitleWithIcon/ModalTitleWithIcon.test.tsx
  • components/MessageToasts/reducers.test.js

Benefits

  1. Enforces modern patterns: New tests cannot use describe/it (ESLint error)
  2. Preserves functionality: All existing tests continue to work
  3. Clear technical debt: TODO markers make legacy patterns visible
  4. Gradual migration path: Files can be updated as they're touched
  5. Better test readability: Flat structure is easier to understand and maintain

Testing Instructions

  1. Run tests to confirm everything still passes:

    npm run test
  2. Try adding a new describe block - it should trigger an ESLint error:

    describe('NewComponent', () => {  // ❌ ESLint error
      test('works', () => {});
    });
  3. New tests should use flat structure:

    test('NewComponent works', () => {  // ✅ Correct pattern
      // test implementation
    });

Additional Information

This migration follows React Testing Library and Jest best practices. The flat test structure:

  • Reduces nesting complexity
  • Makes test names more descriptive
  • Eliminates confusion about hook scopes
  • Improves test maintainability

The TODO markers allow us to track and gradually migrate the remaining describe blocks without breaking existing tests.

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]

rusackas and others added 4 commits September 26, 2025 00:23
…flat test()

- Add jest/consistent-test-it rule to enforce 'test' over 'it'
- Add no-restricted-globals warning for 'describe' and 'it'
- Following Kent C. Dodds' 'avoid nesting when testing' pattern
- Set as warnings for gradual migration strategy

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Applied jest/consistent-test-it rule with auto-fix
- Converted 194 test files from 'it' to 'test'
- Following Kent C. Dodds' testing best practices
- Part of migration away from nested describe blocks

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Safely migrated 5 test files with simple describe blocks
- Only flattened describe blocks without hooks or complex setup
- Preserved all test functionality and behavior
- Following 'avoid nesting when testing' pattern

Files migrated:
- utils/urlUtils.test.ts
- explore/controlPanels/Separator.test.ts
- components/ErrorMessage/ErrorAlert.test.tsx
- components/ModalTitleWithIcon/ModalTitleWithIcon.test.tsx
- components/MessageToasts/reducers.test.js

Remaining: 243 files with describe blocks require manual review
(198 have hooks, 45 have other complexity)

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
…e error on new ones

- Changed no-restricted-globals rule from 'warn' to 'error' for describe/it
- Added eslint-disable comments with TODO markers on all existing describe blocks
- This allows existing code to work while preventing new describe blocks
- Updated 243 test files with 1,080 describe blocks

The TODO markers make it clear these are legacy patterns that should be
migrated over time, while the error rule prevents new code from using
describe blocks.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Copy link

korbit-ai bot commented Sep 26, 2025

Korbit doesn't automatically review large (3000+ lines changed) pull requests such as this one. If you want me to review anyway, use /korbit-review.

@dosubot dosubot bot added change:frontend Requires changing the frontend frontend:refactor Related to refactoring the frontend labels Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend frontend:refactor Related to refactoring the frontend size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant