-
Notifications
You must be signed in to change notification settings - Fork 39
refactor(atomic): migrate atomic-timeframe-facet from Stencil to Lit #6757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: fbeaudoincoveo <[email protected]>
Co-authored-by: fbeaudoincoveo <[email protected]>
β¦W API mocking Co-authored-by: fbeaudoincoveo <[email protected]>
β¦tests for atomic-timeframe-facet Co-authored-by: fbeaudoincoveo <[email protected]>
packages/atomic/src/components/search/facets/atomic-timeframe-facet/atomic-timeframe-facet.ts
Outdated
Show resolved
Hide resolved
...es/atomic/src/components/search/facets/atomic-timeframe-facet/atomic-timeframe-facet.spec.ts
Outdated
Show resolved
Hide resolved
...es/atomic/src/components/search/facets/atomic-timeframe-facet/atomic-timeframe-facet.spec.ts
Outdated
Show resolved
Hide resolved
...es/atomic/src/components/search/facets/atomic-timeframe-facet/atomic-timeframe-facet.spec.ts
Outdated
Show resolved
Hide resolved
...es/atomic/src/components/search/facets/atomic-timeframe-facet/atomic-timeframe-facet.spec.ts
Outdated
Show resolved
Hide resolved
packages/atomic/src/components/search/atomic-timeframe-facet/atomic-timeframe-facet.spec.ts
Outdated
Show resolved
Hide resolved
packages/atomic/src/components/search/atomic-timeframe-facet/atomic-timeframe-facet.spec.ts
Outdated
Show resolved
Hide resolved
packages/atomic/src/components/search/atomic-timeframe-facet/atomic-timeframe-facet.ts
Show resolved
Hide resolved
...tomic/src/components/search/facets/atomic-timeframe-facet/stencil-atomic-timeframe-facet.tsx
Show resolved
Hide resolved
packages/atomic/src/components/search/facets/atomic-timeframe-facet/atomic-timeframe-facet.ts
Outdated
Show resolved
Hide resolved
β¦ation - Move atomic-timeframe-facet directory from /facets to /search level - Delete stencil-atomic-timeframe-facet.tsx and atomic-timeframe-facet.pcss files - Update prop validation to use static readonly schema and only validate specific props (injectionDepth, headingLevel, min, max, sortCriteria) - Add biome-ignore comments for conditional bindStateToController decorators - Restructure test describe blocks: remove unnecessary wrappers, update naming conventions, combine nested describe blocks Co-authored-by: fbeaudoincoveo <[email protected]>
|
@copilot Can you update the tests now? |
β¦ameFacetCommon - Simplified tests to focus on component initialization and integration with TimeframeFacetCommon - Mock TimeframeFacetCommon to test delegation of rendering and lifecycle - Test component-specific logic: props validation, controller initialization, event handling - Remove tests for logic now handled by TimeframeFacetCommon - Reduced test file from ~645 lines to ~330 lines while maintaining essential coverage Co-authored-by: fbeaudoincoveo <[email protected]>
Updated in commit 35d8294. The tests have been refactored to work with the new TimeframeFacetCommon-based architecture. The tests now focus on:
Tests for facet-specific logic (value rendering, date formatting, etc.) are now handled by TimeframeFacetCommon's own test suite, eliminating duplication and reducing the test file from ~645 lines to ~330 lines. |
| }); | ||
| }); | ||
|
|
||
| describe('props', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please get rid of the superfluous describe wrappers.
See .github/instructions/tests-atomic.instructions.md
in particular:
**Nested describe blocks:** Use `describe` blocks to group related tests that share:
- **Same condition/state:** `describe('when <condition>', () => {...})` - Groups tests that share common setup or state
- **Same lifecycle callback:** `describe('#connectedCallback (when added to the DOM)', () => {...})` - Tests for component lifecycle methods
- **Same public method:** `describe('#methodName', () => {...})` - Tests for public methods or properties
**When to use nested describe:**
- Tests share setup logic (e.g., same `beforeEach` mock configuration)
- Multiple tests verify different aspects of the same condition
- Testing a method with multiple scenarios or edge cases
**When NOT to use nested describe:**
- Single test for a condition - include condition in `it()` statement instead
- Tests are unrelated despite similar names
- Over-nesting (avoid more than 2-3 levels deep)Apply to the entire spec file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in commit 514ae6a. Removed all superfluous describe wrappers following the testing guidelines:
- Removed single-test describe blocks (field, facetId, sortCriteria, injectionDepth, headingLevel)
- Flattened min/max prop tests (previously had 2 tests each in describe blocks)
- Removed #controllers, #interactions, and isCollapsed describe wrappers
- Flattened disconnectedCallback test to main level
All tests now follow the pattern: single test conditions are in the it() statement, not nested describe blocks. The test file structure is now cleaner with appropriate nesting (1-2 levels max).
β¦e-facet tests - Remove single-test describe wrappers for field, facetId, sortCriteria, injectionDepth, headingLevel - Remove describe wrappers for min/max that only had 2 tests each - Remove #controllers, #interactions, and isCollapsed describe wrappers - Flatten disconnectedCallback test into main describe block - All tests now follow the guideline: single test conditions go in it() statement, not nested describe Co-authored-by: fbeaudoincoveo <[email protected]>
β Checklist
.mdxfileindex.tsfile.https://coveord.atlassian.net/browse/KIT-4960
Description
Migrates
atomic-timeframe-facetfrom Stencil to Lit framework, aligning with the ongoing modernization effort across the Atomic package. The component has been refactored to leverage theTimeframeFacetCommonhelper class for improved code reuse and maintainability.Component Changes
TimeframeFacetCommonhelper@customElement,@bindings,@withTailwindStyles,@bindStateToController)disconnectedCallbackfor cleanup)@/*path aliasesTimeframeFacetCommon/facets/atomic-timeframe-facet/to/search/atomic-timeframe-facet/stencil-atomic-timeframe-facet.tsxandatomic-timeframe-facet.pcss)Testing
Test Refactoring
Tests were refactored to align with the component's new architecture using TimeframeFacetCommon and Atomic testing guidelines:
Test Structure:
it()statements rather than nested describe blocksWhat's Tested:
What's Not Tested (Delegated to TimeframeFacetCommon):
This approach reduces test duplication (~645 lines to ~290 lines) and ensures tests focus on the component's actual responsibilities rather than logic handled by the shared helper.
Documentation
Key Improvements
Use of TimeframeFacetCommon Helper
The component now delegates all facet logic to the
TimeframeFacetCommonhelper class, which provides:Enhanced Validation
Min/max properties now validate the YYYY-MM-DD format with regex patterns and provide helpful error messages when validation fails.
Example Usage
Original prompt
π¬ We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.