-
Notifications
You must be signed in to change notification settings - Fork 1
test: resolve element internals testing support issue #570
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
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@coderabbitai review this pr please |
|
@QuintonJason: I'll review the PR changes for you now. ✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes introduce a global test setup infrastructure with a mocked ElementInternals API. A new test setup module is created containing a MockElementInternals class that manages form-related internal state and validity checking. This setup is integrated into the Stencil test configuration via setupFilesAfterEnv, and the TypeScript configuration is updated to handle the new setup file. The setup module is marked for exclusion from coverage reporting. Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libs/core/src/utils/test/setup.ts (3)
12-15: Clarify @ts-expect-error comments.The comments state "Stored for mock completeness," but these properties are actively used in
setFormValue(lines 38-39). The@ts-expect-errordirectives appear unnecessary since the properties have valid types and are used correctly.Consider removing the
@ts-expect-errordirectives and updating the comments to accurately reflect the purpose of these properties:- // @ts-expect-error - Stored for mock completeness private _formValue: FormData | string | null = null; - // @ts-expect-error - Stored for mock completeness private _formState: FormData | string | null = null;
67-69: reportValidity implementation is simplified.The standard
ElementInternals.reportValidity()method should fire an "invalid" event when validation fails. This mock only returns the validity state without firing events. For basic unit testing, this simplified implementation may be sufficient, but be aware that tests relying on "invalid" event dispatch will not work correctly.If tests need to verify "invalid" event handling, consider enhancing the mock:
reportValidity(): boolean { + if (!this.validity.valid) { + // Note: Would need element reference to dispatch event properly + // For now, this mock doesn't support event dispatch + } return this.validity.valid; }
124-135: Console suppression is narrowly scoped.The error suppression only filters warnings containing "Property setFormValue was accessed on ElementInternals." Other ElementInternals-related warnings may still appear in test output. This targeted approach is acceptable, but consider expanding the filter if additional warnings emerge during testing.
If needed, you can make the filter more comprehensive:
if ( typeof args[0] === 'string' && - args[0].includes('Property setFormValue was accessed on ElementInternals') + (args[0].includes('ElementInternals') || args[0].includes('attachInternals')) ) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/core/src/utils/test/setup.ts(1 hunks)libs/core/stencil.config.ts(1 hunks)libs/core/tsconfig.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test-lint (22)
- GitHub Check: test-specs (18)
- GitHub Check: test-specs (20)
- GitHub Check: test-lint (18)
- GitHub Check: test-lint (20)
- GitHub Check: test-specs (22)
- GitHub Check: Cycode: Vulnerable Dependencies
🔇 Additional comments (4)
libs/core/tsconfig.json (1)
34-34: LGTM! Correct exclusion of test setup file.Excluding the test setup utility from the main TypeScript compilation is appropriate since it's only used as a side-effect module by the test runner.
libs/core/stencil.config.ts (1)
14-22: LGTM! Testing configuration is correctly structured.The
setupFilesAfterEnvproperty correctly references the global test setup file, and the coverage exclusion patterns appropriately exclude the setup file from coverage reporting.libs/core/src/utils/test/setup.ts (2)
34-40: LGTM! Correct setFormValue implementation.The method correctly handles both the value and optional state parameters, with appropriate fallback logic when state is undefined.
115-119: LGTM! Defensive polyfill implementation.The conditional check ensures
attachInternalsis only polyfilled when needed, and the type assertion is appropriate for the mock.
Description
Fixes #(issue)
Type of change
How Has This Been Tested?
Run the test suite and verify that the ElementInternal console.log message is not present
Test Configuration:
Checklist:
If not applicable, leave options unchecked.