Charts: Add identity-obj-proxy for CSS module class assertions in tests (CHARTS-179)#47476
Charts: Add identity-obj-proxy for CSS module class assertions in tests (CHARTS-179)#47476
Conversation
…ts (CHARTS-179) Co-authored-by: adamwoodnz <1017872+adamwoodnz@users.noreply.github.com>
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Updated the PR description to follow the Jetpack PR template format (Proposed changes, Testing instructions, checklist, etc.). Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
identity-obj-proxy was added to the charts package to enable CSS module class assertions in Jest tests. Running pnpm install updated the lock file with the new dependency and its harmony-reflect peer. Made-with: Cursor
|
@claude review please |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Claude finished @adamwoodnz's task in 1m 59s —— View job PR Review
The implementation is correct and well-structured. Here are my observations: ✅ Correctness of the jest config approachThe key insight is that
This is the correct pattern. No ordering issue exists here because the two mechanisms don't compete. ✅ Test qualityThe four new tests in
These test the actual class application logic in 🔍 Minor nit:
|
There was a problem hiding this comment.
Pull request overview
Updates the Charts package’s Jest setup so SCSS CSS module imports resolve to identity-obj-proxy, enabling reliable toHaveClass() assertions against CSS module class keys in unit tests.
Changes:
- Add
identity-obj-proxy@3.0.0to Charts devDependencies. - Update Charts Jest config to map
*.module.scsstoidentity-obj-proxywhile leaving non-module SCSS on the existing asset stub. - Add new
BaseLegendtests asserting the correct overflow-related class is applied based ontextOverflowandmaxWidth.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/charts/tests/jest.config.cjs | Adds moduleNameMapper for *.module.scss → identity-obj-proxy to support class assertions. |
| projects/js-packages/charts/src/components/legend/test/legend.test.tsx | Adds four class assertion tests for legend-item-text--ellipsis / --wrap behavior. |
| projects/js-packages/charts/package.json | Adds identity-obj-proxy as a dev dependency for the charts package. |
| projects/js-packages/charts/changelog/charts-179-add-identity-obj-proxy-for-css-module-class-assertions | Adds a changelogger entry documenting the test tooling change. |
| pnpm-lock.yaml | Locks identity-obj-proxy (and transitive harmony-reflect) into the workspace lockfile. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
identity-obj-proxy was placed between @-prefixed packages, breaking the expected alphabetical order. Move it with other non-@ packages (esbuild-sass-plugin, jest, etc.) to satisfy the devDependencies sort. Made-with: Cursor
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
…roxy With identity-obj-proxy, CSS module imports resolve to class name strings instead of undefined. Update renderMainMetric and renderTooltip tests to expect the actual class names (main-metric, tooltip-wrapper) passed to the render props. Made-with: Cursor
Fixes CHARTS-179: Add identity-obj-proxy for CSS module class assertions in tests
Summary
Add identity-obj-proxy to the charts package so Jest tests can assert on CSS module class names (e.g.
toHaveClass('legend-item-text--ellipsis')) instead of relying on stub objects.Proposed changes:
identity-obj-proxy@3.0.0as a dev dependency to the charts packagetests/jest.config.cjsto map.module.scssimports toidentity-obj-proxy, enablingtoHaveClass()assertions on CSS module class names in tests. Plain.scssimports continue using the existing asset stub.legend.test.tsxasserting the correct CSS class (legend-item-text--ellipsisorlegend-item-text--wrap) is applied based on thetextOverflowprop and whethermaxWidthis setOther information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
projects/js-packages/chartspnpm test(orTZ=UTC jest --config=tests/jest.config.cjs)src/components/legend/test/legend.test.tsx:applies legend-item-text--ellipsis class when textOverflow is ellipsis and maxWidth is setapplies legend-item-text--wrap class when textOverflow is wrap and maxWidth is setdoes not apply overflow class when maxWidth is not set(ellipsis variant)does not apply overflow class when maxWidth is not set and textOverflow is wrapChangelog