Skip to content

Extend no-module-scope-test-setup to flag top-level describe scope#1087

Open
craigmarker wants to merge 10 commits intomainfrom
craig/eslint-describe-scope-v2
Open

Extend no-module-scope-test-setup to flag top-level describe scope#1087
craigmarker wants to merge 10 commits intomainfrom
craig/eslint-describe-scope-v2

Conversation

@craigmarker
Copy link
Copy Markdown
Contributor

@craigmarker craigmarker commented Apr 10, 2026

The outermost describe() in a test file is just structural — variables declared there are shared across all tests, same as module scope. Nested describes, however, are semantic groups (e.g. "disabled phases") where shared state is intentional and encouraged.

Rule changes:

  • Flag vars at top-level describe scope (no parent describe)
  • Allow vars at nested describe scope (semantic grouping)
  • Recognize function boundaries as valid scopes (fixes false positive on component definitions like FormWrapper)
  • Add beforeEach + render guidance to error message
  • Add ADR-style markdown documenting the rule's design decisions

Fix all 64 violations across 18 test files by inlining data as props, converting shared objects to factory functions, or moving setup into beforeEach where it feeds side-effectful operations like render().

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

What changed?

Why?

How did you test it?

Potential risks

Release notes

Documentation Changes

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026 •
edited
Loading

JavaScript Coverage Report

Total Coverage: 88.68%

Coverage Policy:

  • Baseline (existing code): ≥78.74% (current coverage)
  • New/changed code: ≥90% ✅ STRICTLY ENFORCED
Coverage Details 
$(cat coverage/coverage-summary.json 2>/dev/null | node -p "const c=require('fs').readFileSync(0,'utf8'); JSON.stringify(JSON.parse(c).total, null, 2)" || echo "Coverage summary not available")

View detailed HTML report in artifacts

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026 •
edited
Loading

📊 Combined Coverage Report

Component Coverage Baseline New Code Status
🐍 Python N/A% (no coverage data) ≥70% ≥90% ✅ ⏭️
🐹 Go N/A% (no coverage data) ≥60% ≥90% ✅ ⏭️
📜 JavaScript N/A% (no coverage data) ≥78.74% ≥90% ✅ ⏭️

Coverage Policy

Baseline (Existing Code):

  • Python: ≥70% (current coverage)
  • Go: ≥60% (current coverage)
  • JavaScript: ≥78.74% (current coverage)

New/Changed Code: ≥90% STRICTLY ENFORCED

Long-term Goal: Maintain and improve coverage baselines

📝 Coverage Guidelines
  • All new code must include tests
  • Coverage should not decrease from the base branch
  • Use # pragma: no cover sparingly for truly untestable code
  • Aim for meaningful tests, not just coverage numbers
🔗 Quick Links

@craigmarker craigmarker force-pushed the craig/eslint-describe-scope-v2 branch 5 times, most recently from 8b66822 to f6e4627 Compare April 10, 2026 21:05
craigmarker and others added 4 commits April 10, 2026 15:25
@craigmarker @claude
Extend no-module-scope-test-setup to flag top-level describe scope
c0e81a6 
The outermost describe() in a test file is just structural — variables
declared there are shared across all tests, same as module scope.
Nested describes, however, are semantic groups (e.g. "disabled phases")
where shared state is intentional and encouraged.

Rule changes:
- Flag vars at top-level describe scope (no parent describe)
- Allow vars at nested describe scope (semantic grouping)
- Recognize function boundaries as valid scopes (fixes false positive
  on component definitions like FormWrapper)
- Add beforeEach + render guidance to error message
- Add ADR-style markdown documenting the rule's design decisions

Fix all 64 violations across 18 test files by inlining data as props,
converting shared objects to factory functions, or moving setup into
beforeEach where it feeds side-effectful operations like render().

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@craigmarker @claude
Revert 4 test files in favor of eslint-disable + follow-up restructure
2c8ba15 
select-field, radio-field, categorical-filter, and
use-interpolation-resolver have >5 tests sharing the same data.
Mechanically inlining is worse than restructuring into nested describes
that share state semantically. Disable the rule for now and track the
restructure as a follow-up issue.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@craigmarker @claude
Inline test data where factory functions added no value
89cadaf 
Five files had zero-arg factory functions that were just `defaultProps`
with extra steps — no overrides, no variations between tests. Replace
with inlined props so each test declares exactly what it needs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@craigmarker @claude
Fix formatting and unused imports
e45984c 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@craigmarker craigmarker force-pushed the craig/eslint-describe-scope-v2 branch from 21e3794 to e45984c Compare April 10, 2026 22:25
@craigmarker @claude
Fix lint violations introduced by describe-scope rule extension
38b3330 
The no-module-scope-test-setup rule now flags describe-scope setup,
which caught violations in six test files.

Four files had only import ordering issues, fixed via eslint --fix.

multi-cell.tests.tsx had a mockRecord object at describe scope; inlined
directly into the record prop in each render call.

retry-cell.tests.tsx has shared mocks and a render helper that are
legitimately structured at describe scope (same pattern as
datetime-filter.test.tsx). Suppress with eslint-disable for now;
restructure in a follow-up.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
craigmarker
craigmarker commented Apr 10, 2026
View reviewed changes
javascript/packages/core/components/row/__tests__/row.test.tsx
};

render(<Row items={mockItems} overrides={overrides} />);
render(<Row items={[{ id: 'name', label: 'Name' }]} overrides={overrides} />);
Copy link
Copy Markdown
Contributor Author

@craigmarker craigmarker Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great example of a shared fixture being used for a test that doesn't need all that comes with the fixture.

craigmarker and others added 5 commits April 10, 2026 16:09
@craigmarker
Fix Prettier formatting in multi-cell.tests.tsx
7b223f1
@craigmarker @claude
Inline test data in table-active-filter-tag-list.test.tsx
0f3f013 
buildFilterableColumns() always returned the same base array with no
parameters. Each test that needed a variation did its own .map() or
spread after calling it. Removed the factory and inlined each test's
data directly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@craigmarker @claude
Inline intermediate variables and fix incomplete factory defaults
4d892ef 
- Inline `objectValue` in get-cell-value-for-column.test.ts: single-use
  variable referenced in both createMockRow and toHaveBeenCalledWith; deep
  equality makes the variable unnecessary
- Add `isDetailViewLoading: false` default to buildProps() factory and
  accept Partial overrides param in detail-view-table-page.test.tsx
- Inline `runtimeProps` in table-view-adapter.test.ts: threaded through
  the function call then referenced in assertion; toEqual uses deep equality

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@craigmarker @claude
Document factory function pattern in no-module-scope-test-setup
aef3a7f 
Add factory function guidance to both the lint error message and the
markdown doc. The error message points to the doc with a single line;
the doc covers the full pattern with component and plain-function
examples, plus the rule against spreading factory results and adding
props separately.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@craigmarker @claude
Clean up JSDoc and add targeted inline comments in no-module-scope-te…
f16c35c 
…st-setup

Remove JSDoc from internal helpers that narrate what the code already says
(getCalleeName, hasParentDescribe, isStandaloneFunction). Trim the opening
restatement from the isModuleScope JSDoc. Add inline comments at the three
non-obvious spots: the MemberExpression branch in getCalleeName, the
standalone function check in isModuleScope, and the !hasParentDescribe
inversion.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@craigmarker