test: Clean up content library tests for code lists #14585
Conversation
|
Warning Rate limit exceeded@TomasEng has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request extends the public API and test utility structure in the frontend libraries. It adds exports for the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
solution/studio/designer
TomasEng
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14585 +/- ##
==========================================
+ Coverage 95.73% 95.74% +0.01%
==========================================
Files 1906 1908 +2
Lines 24844 24915 +71
Branches 2848 2848
==========================================
+ Hits 23785 23856 +71
Misses 799 799
Partials 260 260 ☔ View full report in Codecov by Sentry. |
TomasEng
added
team/studio-domain1
quality/testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
frontend/libs/studio-content-library/src/test-data/codeListDataList.ts (3)
10-29: Consider reducing code duplication in codeList1.The items in
codeList1have identical properties except for the value. Consider using a helper function or array mapping to generate these items.Example refactor:
-const codeList1: CodeList = [ - { - value: 'item1', - label: label1ResourceNb.id, - description: description1ResourceNb.id, - helpText: helpText1ResourceNb.id, - }, - { - value: 'item2', - label: label1ResourceNb.id, - description: description1ResourceNb.id, - helpText: helpText1ResourceNb.id, - }, - { - value: 'item3', - label: label1ResourceNb.id, - description: description1ResourceNb.id, - helpText: helpText1ResourceNb.id, - }, -]; +const createCodeListItem = (value: string) => ({ + value, + label: label1ResourceNb.id, + description: description1ResourceNb.id, + helpText: helpText1ResourceNb.id, +}); + +const codeList1: CodeList = ['item1', 'item2', 'item3'].map(createCodeListItem);
12-15: Consider using enums or constants for item values.The
valueproperties use magic strings ('item1', 'item2', etc.). Consider using an enum or constants to prevent typos and improve maintainability.Example:
enum CodeListItems { Item1 = 'item1', Item2 = 'item2', Item3 = 'item3', Item4 = 'item4', }Also applies to: 38-40, 42-44
1-53: Add documentation for test data purpose.Consider adding JSDoc comments to explain the purpose of these test data sets and how they should be used in tests.
Example:
/** * Test data for code lists used in CodeListPage tests. * codeList1: Complete code list with all optional fields * codeList2: Minimal code list with only required fields */frontend/libs/studio-content-library/src/test-data/textResources.ts (3)
4-15: Consider using an enum for text IDs.The text IDs could be better organized using an enum to provide better type safety and autocompletion.
-const label1Id = 'label1'; -const description1Id = 'description1'; -const helpText1Id = 'helpText1'; -const label2Id = 'label2'; -const description2Id = 'description2'; -const helpText2Id = 'helpText2'; -const label3Id = 'label3'; -const description3Id = 'description3'; -const helpText3Id = 'helpText3'; -const label4Id = 'label4'; -const description4Id = 'description4'; -const helpText4Id = 'helpText4'; +enum TextId { + Label1 = 'label1', + Description1 = 'description1', + HelpText1 = 'helpText1', + Label2 = 'label2', + Description2 = 'description2', + HelpText2 = 'helpText2', + Label3 = 'label3', + Description3 = 'description3', + HelpText3 = 'helpText3', + Label4 = 'label4', + Description4 = 'description4', + HelpText4 = 'helpText4', +}
16-39: Consider using a record for text values.The text values could be better organized using a record to group related translations and reduce repetition.
-const label1Nb = 'Ledetekst 1'; -const description1Nb = 'Beskrivelse 1'; -const helpText1Nb = 'Hjelpetekst 1'; -// ... more Norwegian text -const label1En = 'Label 1'; -const description1En = 'Description 1'; -const helpText1En = 'Help text 1'; -// ... more English text +const textValues = { + nb: { + label1: 'Ledetekst 1', + description1: 'Beskrivelse 1', + helpText1: 'Hjelpetekst 1', + // ... more Norwegian text + }, + en: { + label1: 'Label 1', + description1: 'Description 1', + helpText1: 'Help text 1', + // ... more English text + }, +} as const;
41-64: Consider using a factory function for text resources.The individual text resource exports could be simplified using a factory function to reduce repetition.
+const createTextResource = (id: TextId, value: string): TextResource => ({ id, value }); -export const label1ResourceNb: TextResource = { id: label1Id, value: label1Nb }; -export const label1ResourceEn: TextResource = { id: label1Id, value: label1En }; -// ... more text resources +export const label1ResourceNb = createTextResource(TextId.Label1, textValues.nb.label1); +export const label1ResourceEn = createTextResource(TextId.Label1, textValues.en.label1); // ... more text resourcesfrontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx (2)
29-158: Consider using test.each for similar test cases.Some test cases follow similar patterns and could be refactored using
test.eachto reduce code duplication.+const testCases = [ + { + name: 'renders the codeList heading', + setup: () => ({ + selector: { role: 'heading', name: textMock('app_content_library.code_lists.page_name') }, + }), + }, + { + name: 'renders a code list counter message', + setup: () => ({ + selector: { text: textMock('app_content_library.code_lists.code_lists_count_info_plural') }, + }), + }, +]; + +test.each(testCases)('$name', ({ setup }) => { + const { selector } = setup(); + renderCodeListPage(); + const element = selector.role + ? screen.getByRole(selector.role, { name: selector.name }) + : screen.getByText(selector.text); + expect(element).toBeInTheDocument(); +});
160-181: Consider adding return type for renderCodeListPage.The
renderCodeListPagehelper function should have an explicit return type for better type safety.-const renderCodeListPage = (props: Partial<CodeListPageProps> = {}): RenderResult => +const renderCodeListPage = (props: Partial<CodeListPageProps> = {}): RenderResult & { rerender: (ui: React.ReactElement) => void } =>Also, consider adding JSDoc comments to document the helper functions' purposes and parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/libs/studio-components/src/index.ts(1 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx(3 hunks)frontend/libs/studio-content-library/src/test-data/codeListDataList.ts(1 hunks)frontend/libs/studio-content-library/src/test-data/textResources.ts(1 hunks)frontend/libs/studio-content-library/src/types/CodeList.ts(1 hunks)frontend/libs/studio-content-library/src/types/CodeListData.ts(1 hunks)frontend/libs/studio-content-library/src/types/TextResource.ts(1 hunks)frontend/libs/studio-content-library/src/types/TextResources.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/libs/studio-content-library/src/types/CodeList.ts
- frontend/libs/studio-content-library/src/types/TextResource.ts
🔇 Additional comments (8)
frontend/libs/studio-content-library/src/types/TextResources.ts (1)
1-5: LGTM! Clean type definition.The
TextResourcestype definition is well-structured and provides good type safety for managing text resources across different languages.frontend/libs/studio-content-library/src/types/CodeListData.ts (1)
1-7: LGTM! Well-structured type definition.The
CodeListDatatype definition is clear and provides good flexibility with optional fields for data and error handling.frontend/libs/studio-components/src/index.ts (1)
6-6: LGTM! Clean type export.The
TextResourcetype export is correctly added and maintains consistency with existing export patterns.frontend/libs/studio-content-library/src/test-data/codeListDataList.ts (1)
36-45: Consider consistent structure between code lists.
codeList2items have inconsistent structure compared tocodeList1items. Consider documenting why this inconsistency exists if it's intentional for testing purposes.Is this structural difference intentional for testing different scenarios? If so, consider adding a comment explaining the purpose.
frontend/libs/studio-content-library/src/test-data/textResources.ts (2)
1-2: LGTM!The imports are correctly typed and properly scoped.
66-99: LGTM!The text resource arrays and final export are well-organized and properly typed.
frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx (2)
1-27: LGTM!The imports are well-organized, and the mock setup is clear and properly typed.
168-171: Consider using data-testid for accordion element.The comment indicates that accessing the accordion element through
parentElementis not recommended. Instead of waiting for the Design System upgrade, consider adding adata-testidattribute to the accordion element as a temporary solution.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the accordion element is already using data-testid in other files # Search for data-testid usage in accordion elements rg -i "data-testid.*accordion" # Search for the accordion component definition ast-grep --pattern 'const Accordion = ({ children }) => { $$$ }'Length of output: 386
Use
data-testidfor accordion element as in other parts of the codebase.
The search results confirmed that other accordion components (e.g., infrontend/packages/ux-editor-v3/src/containers/DesignView/PageAccordion/PageAccordion.tsxandfrontend/packages/ux-editor/src/containers/DesignView/PageAccordion/PageAccordion.tsx) already utilizedata-testidattributes for testing purposes. Adopting a similar approach in the CodeListPage test is a good temporary solution until the Design System upgrade is performed.
Description
As I am going to work in the
CodeListPagetest suite, I had to do some initial work to facilitate the implementation of the upcoming tests. This pull request contains the following improvements to theCodeListPagetests:test-data, to keep the same structure as instudio-components.defaultCodeListPagePropsin theafterEachhook. This was probably added because one of the tests manipulated the object for some reason. I removed that as well since it was not necessary to make the test work.toHaveAttributechecks.getByRolewhere possible.Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Tests