test: functional tests for TPRM module #2355
Conversation
WalkthroughThe changes introduce significant UI and test improvements for third-party risk management and compliance assessment features. Updates include restructuring Svelte components for better accessibility and testability, enhancing evidence handling logic, and expanding end-to-end test coverage with new fixtures, test data, and detailed TPRM workflow tests. No core business logic is altered. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Svelte)
participant Server (Actions)
participant DB
User->>UI (Svelte): Submit evidence form
UI (Svelte)->>Server (Actions): createEvidence
Server (Actions)->>DB: Save new evidence
DB-->>Server (Actions): Evidence object
Server (Actions)-->>UI (Svelte): { form, newEvidence }
UI (Svelte)->>UI (Svelte): Find requirementAssessment by newEvidence
UI (Svelte)->>UI (Svelte): Update evidences array and form data
UI (Svelte)-->>User: Updated evidence list displayed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
frontend/tests/functional/detailed/tprm.test.ts (2)
10-16: Extract the hard-coded email to a constant.The email 'third-party@tests.com' is used multiple times throughout the test. Consider extracting it to a constant for better maintainability.
+const THIRD_PARTY_EMAIL = 'third-party@tests.com'; + const entityAssessment = { name: 'Test entity assessment', perimeter: vars.perimeterName, create_audit: true, framework: vars.questionnaire.name, - representatives: 'third-party@tests.com' + representatives: THIRD_PARTY_EMAIL };
40-44: Consider a more robust solution for the autocomplete issue.Creating extra items to avoid autocomplete conflicts is a fragile workaround that could break if the autocomplete behavior changes. Consider alternative approaches like using unique timestamps or waiting for specific UI states.
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.svelte (1)
82-82: Consider renaming the internal variable for consistency.The form property changed from
createdEvidencetonewEvidence, but the internal variable is still namedcreatedEvidence. This inconsistency could be confusing.- let createdEvidence = $derived(form?.newEvidence); + let newEvidence = $derived(form?.newEvidence); run(() => { - if (createdEvidence) { + if (newEvidence) { const requirementAssessment = requirementAssessments.find( - (ra) => ra.id === createdEvidence.requirement_assessments[0] + (ra) => ra.id === newEvidence.requirement_assessments[0] ); if (requirementAssessment) { requirementAssessment.evidences.push({ - str: createdEvidence.name, - id: createdEvidence.id + str: newEvidence.name, + id: newEvidence.id }); - requirementAssessment?.updateForm?.data?.evidences?.push(createdEvidence.id); - createdEvidence = undefined; + requirementAssessment?.updateForm?.data?.evidences?.push(newEvidence.id); + newEvidence = undefined; addedEvidence += 1; } } });Also applies to: 176-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/src/routes/(app)/(internal)/analytics/tprm/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/analytics/tprm/FlippableCard.svelte(1 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte(2 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.server.ts(1 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.svelte(4 hunks)frontend/tests/functional/detailed/tprm.test.ts(1 hunks)frontend/tests/utils/login-page.ts(1 hunks)frontend/tests/utils/test-data.ts(2 hunks)frontend/tests/utils/test-utils.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.server.ts (1)
frontend/src/lib/utils/actions.ts (1)
nestedWriteFormAction(176-196)
⏰ 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). (8)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.server.ts (1)
116-117: LGTM! Evidence creation response structure improved.The refactoring to await the
nestedWriteFormActionresult and explicitly return both the form and the newly created evidence makes the API more predictable and easier to consume from the frontend. The property extraction fromresult.form.message.objectaligns with the corresponding frontend changes that expectform?.newEvidence.frontend/src/routes/(app)/(internal)/analytics/tprm/FlippableCard.svelte (6)
51-57: LGTM! Improved card structure and accessibility.The restructuring of the outer container to directly manage flip rotation with proper CSS transforms and the addition of
role="listitem"enhances both the visual behavior and accessibility of the component.
64-87: Good testability improvements with proper accessibility.The addition of
data-testid="flip-button-front"and properaria-labelmakes the flip button easily testable and accessible. The simplified SVG structure without nested tags is cleaner.
93-106: Excellent testability enhancements for key content elements.Adding
data-testidattributes to provider name and conclusion badge will facilitate reliable UI testing. These elements are likely key interaction points in the TPRM workflows.
122-128: Fix: Corrected layout class and added testability.The change from flex layout to
inline-blockfor the baseline container is more appropriate for this content, and the addition ofdata-testid="baseline"supports testing requirements.
196-220: Consistent back-face implementation with testability.The back face flip button maintains consistency with the front face implementation, including proper accessibility attributes and test IDs. The SVG simplification is also applied consistently.
234-236: Comprehensive test coverage for back-face content.Adding
data-testidattributes to completion percentage, reviewers, and observation text ensures all key information on the card's back face is testable. This supports the comprehensive TPRM testing mentioned in the PR objectives.Also applies to: 250-252, 262-264
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (3)
539-539: Good testability addition for action plan navigation.The
data-testid="action-plan-button"attribute enables reliable testing of the action plan functionality, which is likely a key workflow in compliance assessments.
549-549: Excellent test coverage for mode switching buttons.Adding test IDs to both flash-mode and table-mode buttons ensures that the different viewing modes can be reliably tested in E2E scenarios. These are core navigation elements for compliance assessment workflows.
Also applies to: 557-559
565-567: Comprehensive testability for key compliance actions.The test IDs for apply-mapping and sync-to-actions buttons cover important compliance management operations. These additions support thorough testing of the TPRM module functionality mentioned in the PR objectives.
Also applies to: 573-573
frontend/tests/utils/login-page.ts (1)
82-84: LGTM! Enhanced flexibility for welcome screen handling.The addition of an optional
urlparameter with a sensible default maintains backward compatibility while enabling flexible URL pattern matching for different user types. This supports the third-party authentication flows mentioned in the test enhancements.frontend/tests/utils/test-data.ts (2)
28-33: Good addition of third-party user test data.The
thirdPartyUserobject follows the same structure as the existinguserobject and provides necessary credentials for testing third-party representative workflows in the TPRM module.
560-564: Appropriate questionnaire test data for CMMC framework.The
questionnaireobject with CMMC 2.0 data complements the existing framework test data and supports the expanded compliance assessment testing scenarios.frontend/src/routes/(app)/(internal)/analytics/tprm/+page.svelte (1)
14-24: LGTM! Good accessibility and testability improvements.The addition of
role="list"improves semantic HTML for screen readers, and thedata-testidattributes enable better test targeting. The consolidated styling maintains the same visual appearance while simplifying the DOM structure.frontend/tests/functional/detailed/tprm.test.ts (1)
178-221: Well-structured test for password setting flow.Good implementation of the email verification flow with proper page navigation, password setting, and session cleanup. The logout step prevents potential session conflicts.
frontend/tests/utils/test-utils.ts (2)
339-386: Well-structured page fixtures following established patterns.The new TPRM-related page fixtures are properly implemented with appropriate URLs, titles, and form field definitions. Good consistency with existing fixture patterns.
804-818: Consistent implementation of entities page configuration.The entities page configuration follows the established pattern with proper build and editParams objects.
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.svelte (1)
327-338: Good semantic HTML improvements.The change from generic div/span elements to ul/li structure improves semantic HTML, making the content more accessible to screen readers. The h3 headers also provide better document structure.
| {/if} | ||
|
|
||
| <form | ||
| class="flex flex-col space-y-2 items-center justify-evenly w-full tale-mode-form" |
There was a problem hiding this comment.
Fix typo in CSS class name.
The class name "tale-mode-form" appears to be a typo.
- class="flex flex-col space-y-2 items-center justify-evenly w-full tale-mode-form"
+ class="flex flex-col space-y-2 items-center justify-evenly w-full table-mode-form"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class="flex flex-col space-y-2 items-center justify-evenly w-full tale-mode-form" | |
| class="flex flex-col space-y-2 items-center justify-evenly w-full table-mode-form" |
🤖 Prompt for AI Agents
In
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.svelte
at line 458, correct the typo in the CSS class name "tale-mode-form" to the
intended "table-mode-form" to ensure proper styling is applied.
| placeholder="" | ||
| class="input w-full" | ||
| bind:value={requirementAssessment.observation} | ||
| data-testid="form-input-ovservation" |
There was a problem hiding this comment.
Fix typo in data-testid attribute.
- data-testid="form-input-ovservation"
+ data-testid="form-input-observation"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data-testid="form-input-ovservation" | |
| data-testid="form-input-observation" |
🤖 Prompt for AI Agents
In
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.svelte
at line 639, there is a typo in the data-testid attribute
"form-input-ovservation". Correct the typo by changing "ovservation" to
"observation" to ensure the attribute is spelled correctly.
| await expect(assessableRequirements).not.toHaveCount(0); | ||
| await assessableRequirements.first().locator('.text-base').first().click(); | ||
| await assessableRequirements | ||
| .first() | ||
| .locator('li:nth-child(2) > .control > .p-1 > label:nth-child(2) > .text-base') | ||
| .first() | ||
| .click(); | ||
| await assessableRequirements | ||
| .first() | ||
| .locator('li:nth-child(3) > .control > .p-1 > label > .text-base') | ||
| .first() | ||
| .click(); | ||
| await assessableRequirements | ||
| .first() | ||
| .locator('li:nth-child(4) > .control > .p-1 > label > .text-base') | ||
| .first() | ||
| .click(); | ||
| await assessableRequirements | ||
| .first() | ||
| .locator('li:nth-child(5) > .control > .p-1 > label:nth-child(3) > .text-base') | ||
| .first() | ||
| .click(); | ||
| await assessableRequirements | ||
| .first() | ||
| .locator('li:nth-child(6) > .control > .p-1 > label:nth-child(3) > .text-base') | ||
| .first() | ||
| .click(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fragile selectors may break with UI changes.
The test uses very specific CSS selectors with nth-child and class combinations that are likely to break if the UI structure changes. Consider using more robust selectors based on data-testid attributes or semantic roles.
Example of more robust selector:
- await assessableRequirements
- .first()
- .locator('li:nth-child(2) > .control > .p-1 > label:nth-child(2) > .text-base')
- .first()
- .click();
+ await assessableRequirements
+ .first()
+ .getByTestId('assessment-option-2')
+ .click();🤖 Prompt for AI Agents
In frontend/tests/functional/detailed/tprm.test.ts around lines 245 to 271, the
test uses fragile CSS selectors with nth-child and class combinations that can
easily break with UI changes. Replace these selectors with more robust ones
using data-testid attributes or semantic roles to improve test stability. Update
the locator calls to target elements by these stable attributes instead of
relying on complex CSS paths.
| test.afterAll('cleanup', async ({ browser }) => { | ||
| const page = await browser.newPage(); | ||
| const loginPage = new LoginPage(page); | ||
| const foldersPage = new PageContent(page, '/folders', 'Domains'); | ||
|
|
||
| await loginPage.goto(); | ||
| await loginPage.login(); | ||
| await foldersPage.goto(); | ||
|
|
||
| await foldersPage.deleteItemButton(vars.folderName).click(); | ||
| await expect(foldersPage.deletePromptConfirmTextField()).toBeVisible(); | ||
| await foldersPage.deletePromptConfirmTextField().fill(m.yes()); | ||
| await foldersPage.deletePromptConfirmButton().click(); | ||
|
|
||
| await foldersPage.deleteItemButton(vars.folderName + ' foo').click(); | ||
| await expect(foldersPage.deletePromptConfirmTextField()).toBeVisible(); | ||
| await foldersPage.deletePromptConfirmTextField().fill(m.yes()); | ||
| await foldersPage.deletePromptConfirmButton().click(); | ||
|
|
||
| await expect(foldersPage.getRow(vars.folderName)).not.toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Incomplete cleanup may leave test artifacts.
The cleanup only removes folders but not other entities created during the test (perimeters, entities, solutions, representatives, etc.). This could cause test pollution.
Consider extending the cleanup to remove all created test data:
test.afterAll('cleanup', async ({ browser }) => {
const page = await browser.newPage();
const loginPage = new LoginPage(page);
const foldersPage = new PageContent(page, '/folders', 'Domains');
+ const entitiesPage = new PageContent(page, '/entities', 'Entities');
+ // Add other page objects as needed
await loginPage.goto();
await loginPage.login();
+
+ // Clean up entities first (due to dependencies)
+ await entitiesPage.goto();
+ await entitiesPage.deleteItemButton(testObjectsData.entitiesPage.build.name).click();
+ // ... confirm deletion
+
+ // Then clean up folders
await foldersPage.goto();
// ... existing folder cleanup code
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/tests/functional/detailed/tprm.test.ts around lines 333 to 353, the
cleanup function only deletes folders but does not remove other test entities
like perimeters, entities, solutions, and representatives, which can cause test
pollution. Extend the cleanup by adding steps to delete all these additional
entities created during the test, ensuring each deletion waits for confirmation
prompts and verifies removal to fully clean the test environment.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
data-testidattributes across components to facilitate more robust automated testing.