-
Notifications
You must be signed in to change notification settings - Fork 3
Setup tests for different utility functions and components #40
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
Conversation
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.
Pull Request Overview
This PR sets up comprehensive tests for various utility functions and UI components while refactoring some core modules to improve code consistency and behavior. Key changes include:
- Consolidation and export refactoring of utility functions in object-property.tsx.
- Addition of extensive test suites for data widgets, data plugins, and data core functionalities.
- Minor improvements in error messages, plugin hook resolution, and test setup.
Reviewed Changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/data-widgets/lib/components/object-property.tsx | Consolidated lodash-es imports and updated export declarations. |
packages/data-widgets/lib/components/object-property.test.tsx | Added tests for utility functions from object-property.tsx. |
packages/data-widgets/lib/components/elements.tsx | Made the label prop optional in the ArrayFieldset component. |
packages/data-widgets/lib/components/array-fieldset.elements.test.tsx | Added tests for the ArrayFieldset component. |
packages/data-plugins/lib/utils.ts | Corrected parameter type and fixed a mapping in fieldIf. |
packages/data-plugins/lib/utils.test.ts | Added tests for utility functions in the data-plugins package. |
packages/data-core/lib/schema/schema.test.ts | Added tests covering different schema-to-form transformations. |
packages/data-core/lib/plugin-utils/resolve.ts | Changed hook target lookup to use a copy of plugins. |
packages/data-core/lib/plugin-utils/resolve.test.ts | Added tests for plugin resolution and hook application. |
packages/data-core/lib/context/plugin-config.tsx | Updated error message for provider context. |
packages/data-core/lib/config/config.test.ts | Added tests for merging plugin configurations. |
packages/data-core/lib/components/widget-renderer.ts | Removed duplicate imports and updated widget error messages. |
packages/data-core/lib/components/widget-renderer.test.ts | Added tests for the WidgetRenderer, including error handling. |
packages/data-core/lib/components/plugin-box.tsx | Modified ErrorBox usage by adding a data-testid attribute. |
packages/data-core/lib/components/plugin-box.test.tsx | Introduced tests for PluginBox behavior in various schema cases. |
jest.config.ts | Configured moduleNameMapper for lodash-es and test setup files. |
jest-setup.ts | Added a Jest setup file for testing-library integration. |
Files not reviewed (3)
- package.json: Language not supported
- packages/data-core/lib/components/snapshots/plugin-box.test.tsx.snap: Language not supported
- packages/data-widgets/lib/components/snapshots/array-fieldset.elements.test.tsx.snap: Language not supported
Comments suppressed due to low confidence (3)
packages/data-core/lib/components/plugin-box.test.tsx:32
- Remove the '.only' modifier from this test to ensure that all tests run during the test suite execution.
it.only('renders ErrorBox when editSchema is null', () => {
packages/data-widgets/lib/components/object-property.tsx:49
- [nitpick] Consider removing the commented-out boolean inference block if it is no longer needed to reduce clutter and improve code clarity.
// if (typeof value === 'boolean') {
packages/data-core/lib/plugin-utils/resolve.ts:72
- [nitpick] Review whether using 'pluginsCopy.find' instead of 'plugins.find' aligns with the intended hook resolution behavior and does not affect plugin ordering.
const plTarget = pluginsCopy.find((p) => p.name === hook.name);
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.
Pull Request Overview
This PR sets up tests for various utility functions and components while also refactoring some exports and adjusting configuration settings.
- Consolidated multiple lodash imports into a single statement and exported some internal functions for testing.
- Updated tests for utility functions, widget components, and plugin components to match the new public APIs.
- Adjusted Jest configuration by mapping "lodash-es" to "lodash" and adding a global test setup file.
Reviewed Changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/data-widgets/lib/components/object-property.tsx | Combined multiple lodash imports and exposed internal functions for testing. |
packages/data-widgets/lib/components/elements.tsx | Made the "label" prop for ArrayFieldset optional. |
packages/data-plugins/lib/utils.ts | Altered the property mapping in fieldIf and updated tuple2Object typing. |
packages/data-core/lib/plugin-utils/resolve.ts | Modified hook target resolution to use a copied plugins list. |
packages/data-core/lib/context/plugin-config.tsx | Updated error message string to reflect updated provider naming. |
packages/data-core/lib/components/widget-renderer.tsx | Removed a duplicate import and renamed error boundary prop for clarity. |
jest.config.ts | Added moduleNameMapper for "lodash-es" and configured setupFilesAfterEnv. |
Files not reviewed (3)
- package.json: Language not supported
- packages/data-core/lib/components/snapshots/plugin-box.test.tsx.snap: Language not supported
- packages/data-widgets/lib/components/snapshots/array-fieldset.elements.test.tsx.snap: Language not supported
Comments suppressed due to low confidence (2)
packages/data-core/lib/plugin-utils/resolve.ts:72
- Using pluginsCopy.find instead of plugins.find could change the hook resolution behavior; please confirm that this modification is intentional and does not adversely affect hook ordering or plugin targeting.
const plTarget = pluginsCopy.find((p) => p.name === hook.name);
jest.config.ts:93
- [nitpick] Mapping 'lodash-es' to 'lodash' in Jest configuration may have unintended side effects if the ES module functionalities are required; please ensure this change is acceptable for the test environment.
moduleNameMapper: { 'lodash-es': 'lodash' },
Ensures plugins are built during the CI checks.
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.
These tests are very elaborate. I understand what half of them mean and those make sense to me.
Looking forward to seeing how this serves us in CI.
@@ -0,0 +1,78 @@ | |||
import { mod, getArrayLabel, toNumber, castArray } from './index'; |
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.
Just wondering - do we really need to test these generic functions? 🤷
I would hope that whatever mod
function we use, it is tested elsewhere.
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.
Ahahah. Yeah, they are very much not necessary, but those were autogenerated by copilot and I just thought why not leave then... 😅
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.
Yeah, let's maybe take those out then, no? Less code to push around and we save the world a few kb of noise. 😄
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.
First time I see someone ask to remove tests 😅
But yeah, they're a bit too much
@j08lue Done! |
No description provided.