Refactor forms tests #1312
Refactor forms tests #1312
Conversation
|
Warning Rate limit exceeded@nmajor25 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 18 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. WalkthroughThe changes involve a significant refactoring of the test suite for patient action forms, focusing on replacing hardcoded identifiers with dynamic references. A new JavaScript module has been introduced to generate configuration objects for forms, while several test cases have been updated to enhance maintainability and accuracy. Additionally, a file containing static form definitions has been removed, and dynamic UUIDs are now used for resource identification to ensure uniqueness. Changes
Possibly related PRs
Poem
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 (
|
test/integration/forms/action.js
Outdated
| getFormResponse({ | ||
| id: '1', | ||
| id: uuid(), |
There was a problem hiding this comment.
Every getFormResponse() returns a form response with id: '11111'. Which was causing issues in this test.
RoundingWell Care Ops Frontend
|
||||||||||||||||||||||||||||||||||
| Project |
RoundingWell Care Ops Frontend
|
| Branch Review |
action-forms-refactor
|
| Run status |
|
| Run duration | 03m 06s |
| Commit |
|
| Committer | Nick Major |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
308
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
test/integration/forms/action.js • 1 failed test • care-ops-e2e
| Test | Artifacts | |
|---|---|---|
| Patient Action Form > refresh stale form |
Test Replay
Screenshots
|
|
Pull Request Test Coverage Report for Build 5cea99a6-a0bb-42e8-abb4-924948786202Details
💛 - Coveralls |
![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
Outside diff range, codebase verification and nitpick comments (1)
test/support/api/forms.js (1)
23-27: Missing Form IDs infxTestFormsThe form IDs '22222', '55555', '66666', '88888', and 'BBBBB' are not present in the
fxTestFormsarray as defined intest/fixtures/test/forms.json. This absence could lead to runtime errors or failed tests when these IDs are accessed. Please ensure that these IDs are included infxTestForms.
- File:
test/fixtures/test/forms.jsonAnalysis chain
Verify the existence of form IDs in
fxTestForms.Ensure that all form IDs used in the constants are present in the
fxTestFormsarray to avoid potential runtime errors.Run the following script to verify the presence of form IDs:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence of form IDs in `fxTestForms`. # Test: Search for the form IDs. Expect: All IDs should be present in the array. rg --type json -A 5 $'fxTestForms'Length of output: 33
Script:
#!/bin/bash # Description: Locate `fxTestForms` and verify the presence of form IDs in JavaScript files. # Step 1: Locate the definition of `fxTestForms` in JavaScript files. rg --type js 'fxTestForms' -A 10 # Step 2: Verify the presence of specific form IDs in `fxTestForms`. # IDs to check: '22222', '55555', '66666', '88888', 'BBBBB' rg --type js "'22222'|'55555'|'66666'|'88888'|'BBBBB'" -A 5Length of output: 17603
Script:
#!/bin/bash # Description: Locate the `fixtures/test/forms` file and verify the presence of form IDs. # Step 1: Locate the `fixtures/test/forms` file. fd 'forms' --type f # Step 2: Verify the presence of specific form IDs in the located file. # IDs to check: '22222', '55555', '66666', '88888', 'BBBBB' rg "'22222'|'55555'|'66666'|'88888'|'BBBBB'" $(fd 'forms' --type f)Length of output: 1557
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- test/integration/forms/action.js (71 hunks)
- test/support/api/forms.js (1 hunks)
Additional comments not posted (3)
test/support/api/forms.js (1)
23-27: New constants for form testing added.The addition of
testReadOnlyForm,testWidgetsForm,testPrefillForm,testSubmitHiddenForm, andtestReportFormenhances the modularity and flexibility of the testing framework. Each constant is properly initialized usinggetResourcewith specific IDs. Ensure that these IDs correspond to valid forms in thefxTestFormsarray.test/integration/forms/action.js (2)
2-2: Refactoring to use dynamic references and new imports.The introduction of
uuidfor generating unique identifiers and the import of new test form variables liketestForm,testReadOnlyForm, etc., replace static IDs previously used in the tests. This enhances the maintainability and flexibility of the test cases by allowing for more dynamic and reusable test scenarios.Also applies to: 15-15, 16-16
149-149: Verify the correctness and consistency of dynamic keys in local storage interactions.The use of dynamic keys based on the current clinician's ID for local storage interactions is a good practice. However, it is crucial to verify that these keys are constructed correctly and used consistently across different test cases to ensure accurate state management.
Run the following script to verify the consistency of dynamic keys:
Also applies to: 187-187, 269-269, 367-367
Verification successful
Dynamic key usage in local storage interactions is consistent and correct.
The dynamic keys in
test/integration/forms/action.jsare constructed usingcurrentClinician.id,testPatient.id,testForm.id, andtestAction.id, and this pattern is consistently applied within the file. The review comment's concern about consistency and correctness is addressed, as the keys are constructed and used correctly across the test cases in this file. No issues were found in the dynamic key usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of dynamic keys used in local storage interactions. # Test: Search for the usage of dynamic keys. Expect: Consistent construction and usage across test cases. rg --type javascript -A 5 $'localStorage'Length of output: 75
Script:
#!/bin/bash # Description: Verify the consistency of dynamic keys used in local storage interactions. # Test: Search for the usage of dynamic keys. Expect: Consistent construction and usage across test cases. rg --type js -A 5 $'localStorage'Length of output: 51564
nmajor25
changed the title
/integration/forms/action.js tests
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- test/integration/forms/form.js (19 hunks)
- test/integration/forms/patient.js (29 hunks)
- test/integration/forms/preview.js (1 hunks)
- test/support/api/forms.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/support/api/forms.js
Additional comments not posted (8)
test/integration/forms/preview.js (2)
6-6: Approved import statement for dynamic form ID handling.The import of
testFormis essential for replacing hardcoded form IDs with dynamic references, enhancing test flexibility.
11-16: Approved dynamic form ID usage in routing and visiting methods.The updates to
.routeFormand.visitmethods to usetestForm.idare correctly implemented, enhancing the test's adaptability to different form IDs. Ensure that the dynamic ID is consistently used across all test scenarios.Run the following script to verify the dynamic ID usage:
Verification successful
Dynamic ID usage is correctly implemented and consistent across tests.
The dynamic ID
testForm.idis used appropriately in the.routeFormand.visitmethods intest/integration/forms/preview.js, and its usage is consistent across other test files. This ensures the tests are adaptable to different form IDs. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the dynamic ID usage in routing and visiting methods. # Test: Search for the dynamic ID usage. Expect: Only occurrences of the dynamic ID. rg --type python -A 5 $'testForm.id'Length of output: 66
Script:
#!/bin/bash # Description: Verify the dynamic ID usage in routing and visiting methods. # Test: Search for the dynamic ID usage in JavaScript files. rg --type js -A 5 'testForm.id'Length of output: 35705
test/integration/forms/form.js (3)
11-17: Approved import statements for dynamic form references.The import of various test forms is essential for replacing hardcoded form IDs with dynamic references, enhancing the flexibility and maintainability of the tests.
Line range hint
42-130: Approved dynamic action and form ID usage in routing and visiting methods.The updates to
.routeFormByActionand.visitmethods to usetestForm.idandtestAction.idare correctly implemented, enhancing the test's adaptability to different action and form IDs. Ensure that the dynamic IDs are consistently used across all test scenarios.Run the following script to verify the dynamic ID usage:
Verification successful
Dynamic ID Usage Verified in Routing and Visiting Methods
The dynamic IDs
testForm.idandtestAction.idare correctly used across the codebase, including in the filetest/integration/forms/form.js, ensuring the tests adapt to different scenarios as intended. This confirms the flexibility and maintainability of the test suite.
- Files Verified:
test/integration/forms/form.js, among others.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the dynamic ID usage in routing and visiting methods. # Test: Search for the dynamic ID usage. Expect: Only occurrences of the dynamic IDs. rg --type python -A 5 $'testForm.id|testAction.id'Length of output: 82
Script:
#!/bin/bash # Description: Verify the dynamic ID usage in routing and visiting methods in JavaScript files. # Test: Search for the dynamic ID usage in JavaScript files. rg --type js -A 5 'testForm.id|testAction.id'Length of output: 49440
19-19: Approved variable renaming for clarity.The renaming of
patienttotestPatientclarifies its role in the test cases. Verify thattestPatientis used consistently across all test scenarios.Run the following script to verify the consistent usage of
testPatient:Verification successful
Consistent usage of
testPatientconfirmed.The renaming of
patienttotestPatienthas been consistently applied across the codebase, including intest/integration/forms/form.js. This supports the clarity and purpose of the variable in test scenarios. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent usage of `testPatient`. # Test: Search for the variable usage. Expect: Only occurrences of `testPatient`. rg --type python -A 5 $'testPatient'Length of output: 66
Script:
#!/bin/bash # Description: Verify the consistent usage of `testPatient` in JavaScript files. # Test: Search for the variable usage. Expect: Only occurrences of `testPatient`. rg --type js -A 5 $'testPatient'Length of output: 98892
test/integration/forms/patient.js (3)
7-13: Approved import statements for dynamic form references and JSON API handling.The import of various test forms and additional helper functions is essential for replacing hardcoded form IDs with dynamic references and for handling JSON API data, enhancing the flexibility and maintainability of the tests.
Line range hint
30-732: Approved dynamic patient, form, and clinician ID usage in routing and visiting methods.The updates to
.routeFormand.visitmethods to usetestForm.id,testPatient.id, andcurrentClinician.idare correctly implemented, enhancing the test's adaptability to different patient, form, and clinician IDs. Ensure that the dynamic IDs are consistently used across all test scenarios.Run the following script to verify the dynamic ID usage:
Verification successful
Dynamic ID Usage Verified in JavaScript Tests
The search results confirm that the dynamic IDs
testForm.id,testPatient.id, andcurrentClinician.idare used consistently across various test files. This indicates that the implementation correctly adapts to different patient, form, and clinician IDs, enhancing the test's flexibility and reliability.
- Files with dynamic ID usage:
test/integration/forms/patient.jstest/integration/patients/patient/patient-flow.jstest/integration/patients/patient/dashboard.jstest/integration/forms/action.js- And others as listed in the output.
The dynamic IDs are used in routing, visiting methods, and local storage keys, ensuring that the tests are adaptable to different scenarios.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the dynamic ID usage in routing and visiting methods. # Test: Search for the dynamic ID usage. Expect: Only occurrences of the dynamic IDs. rg --type python -A 5 $'testForm.id|testPatient.id|currentClinician.id'Length of output: 103
Script:
#!/bin/bash # Description: Verify the dynamic ID usage in routing and visiting methods. # Test: Search for the dynamic ID usage in JavaScript files. rg --type js -A 5 'testForm.id|testPatient.id|currentClinician.id'Length of output: 75411
18-19: Approved variable declarations for dynamic patient and clinician references.The declarations of
testPatientandcurrentClinicianreplace hardcoded IDs with dynamic references, enhancing test flexibility. Verify that these variables are used consistently across all test scenarios.Run the following script to verify the consistent usage of
testPatientandcurrentClinician:Verification successful
Consistent Usage of
testPatientandcurrentClinicianVerifiedThe variables
testPatientandcurrentClinicianare used consistently across various test scenarios, replacing hardcoded IDs with dynamic references. This enhances the flexibility and maintainability of the tests. No issues were found regarding their usage.
- Files where these variables are used include:
test/integration/forms/patient.jstest/integration/patients/sidebar/flow-sidebar.jstest/integration/patients/patient/patient-flow.js- And several others.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent usage of `testPatient` and `currentClinician`. # Test: Search for the variable usage. Expect: Only occurrences of `testPatient` and `currentClinician`. rg --type python -A 5 $'testPatient|currentClinician'Length of output: 85
Script:
#!/bin/bash # Description: Verify the consistent usage of `testPatient` and `currentClinician` in JavaScript files. # Test: Search for the variable usage. Expect: Only occurrences of `testPatient` and `currentClinician`. rg --type js -A 5 'testPatient|currentClinician'Length of output: 147371
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/integration/formservice/formservice.js (2 hunks)
Additional comments not posted (2)
test/integration/formservice/formservice.js (2)
7-9: Review of new importsThe new imports (
testReportForm,getFlow,getPatient) are aligned with the PR's objective to use dynamic references instead of hardcoded values. This change enhances the flexibility and maintainability of the tests.
164-198: Refactored test case: "action formservice latest response from action tags"The refactoring of this test case to use dynamic data (
testFlow,testPatient,testAction) significantly enhances its flexibility and maintainability. The use of functions likegetFlow,getPatient, andgetActionto dynamically generate test data is a robust approach that aligns well with the PR's objectives.
test/support/api/forms.js
Outdated
| @@ -20,6 +20,15 @@ export function getForms() { | |||
|
|
|||
| // Exporting only form needed for testing variance | |||
| export const testForm = getResource(_.find(fxTestForms, { id: '11111' })); | |||
| export const testReadOnlyForm = getResource(_.find(fxTestForms, { id: '22222' })); | |||
There was a problem hiding this comment.
I actually think we should probably remove 100% of these.
We should generate generic forms.. in collections instead of using tests.. probably with mostly no options. And the test specifics should largely move to the tests.
Although maybe it's easier to have a single test form export? If that's the case we should essentially do this:
const fxTestForms = [{ name: 'Test Form' }];
...
export const testForm = getForm({ name: 'Test Form' });
But we should remove all of the formId = from the routeFormX stuff. This maybe where having a default testForm or possibly two is useful.. But most of these exports are single use
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (5)
test/integration/patients/worklist/worklist.js (1)
Line range hint
1183-1748: Test case looks good but consider moving it to component tests.The
date filteringtest case extensively verifies the behavior of date filtering on the worklist page. It covers various scenarios such as relative dates, specific dates, and date ranges, ensuring that the worklist is filtered correctly based on the selected date filters.However, the test case is quite long and covers a lot of scenarios. To improve maintainability and readability, consider moving this test case to component-level tests specifically designed for testing the date filtering functionality.
test/integration/formservice/formservice.js (2)
165-172: Consider defining'foo-tag'as a constantTo enhance maintainability and avoid duplication, consider defining
'foo-tag'as a constant variable that can be reused throughout the test.Apply this diff to define
'foo-tag'as a constant:+ const prefillTag = 'foo-tag'; const testReportForm = getForm({ attributes: { options: { is_report: true, - prefill_action_tag: 'foo-tag', + prefill_action_tag: prefillTag, }, }, });And update the assertion at line 207:
- .should('contain', 'filter[action.tags]=foo-tag') + .should('contain', `filter[action.tags]=${ prefillTag }`)
174-184: Consider defining'prefill-latest-response'tag as a constantFor consistency and ease of future updates, consider defining
'prefill-latest-response'as a constant variable.Apply this diff to define the tag as a constant:
+ const prefillLatestResponseTag = 'prefill-latest-response'; const testAction = getAction({ attributes: { created_at: createdAt, - tags: ['prefill-latest-response'], + tags: [prefillLatestResponseTag], }, relationships: { 'form': getRelationship(testReportForm), 'flow': getRelationship(testFlow), 'patient': getRelationship(testPatient), }, });test/integration/forms/patient.js (2)
Line range hint
142-148: Replace hardcoded form response ID with dynamic valueThe test currently uses a hardcoded form response ID
'12345'. To enhance flexibility and align with the refactoring efforts of replacing hardcoded identifiers with dynamic references, consider using a dynamically generated ID from the test data.Apply this diff to replace the hardcoded ID with a dynamic value:
+ const testFormResponse = getFormResponse(); - body: { data: getFormResponse({ id: '12345' }) }, + body: { data: testFormResponse },And update the assertion accordingly:
- cy.get('iframe').should('have.attr', 'src', '/formapp/12345'); + cy.get('iframe').should('have.attr', 'src', `/formapp/${ testFormResponse.id }`);
Line range hint
142-148: Replace all hardcoded form response IDs in testsThere are multiple instances where the hardcoded form response ID
'12345'is used across different tests. To maintain consistency and align with the goal of replacing hardcoded identifiers, consider updating all these instances to use a dynamic ID.For example, apply this change in each relevant test:
+ const testFormResponse = getFormResponse(); - body: { data: getFormResponse({ id: '12345' }) }, + body: { data: testFormResponse },And update subsequent assertions:
- cy.get('iframe').should('have.attr', 'src', '/formapp/12345'); + cy.get('iframe').should('have.attr', 'src', `/formapp/${ testFormResponse.id }`);Also applies to: 212-215, 301-304, 776-776
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- test/fixtures/config/forms.js (1 hunks)
- test/fixtures/test/forms.json (0 hunks)
- test/integration/forms/action.js (66 hunks)
- test/integration/forms/form.js (19 hunks)
- test/integration/forms/patient.js (29 hunks)
- test/integration/forms/preview.js (1 hunks)
- test/integration/formservice/formservice.js (2 hunks)
- test/integration/globals/app-nav.js (4 hunks)
- test/integration/outreach/outreach.js (5 hunks)
- test/integration/patients/patient/archive.js (1 hunks)
- test/integration/patients/patient/patient-flow.js (2 hunks)
- test/integration/patients/patient/sidebar.js (10 hunks)
- test/integration/patients/worklist/schedule.js (3 hunks)
- test/integration/patients/worklist/worklist.js (3 hunks)
- test/support/api/form-responses.js (2 hunks)
- test/support/api/forms.js (1 hunks)
Files not reviewed due to no reviewable changes (1)
- test/fixtures/test/forms.json
Additional comments not posted (107)
test/fixtures/config/forms.js (1)
1-9: LGTM!The exported function generates a well-structured configuration object for testing purposes. The use of the
fakerlibrary ensures unique and randomized data, which is a good practice for creating robust tests. Theidproperty being a UUID and thenameproperty combining buzzwords and nouns are effective choices for generating realistic test data. Theoptionsproperty being an empty object allows for future extensibility.The code is clean, follows good practices, and serves its intended purpose well.
test/support/api/forms.js (5)
4-5: LGTM!The updated import path for test forms aligns with the refactoring objective and improves the organization of test data.
11-11: LGTM!The modifications to
getFormandgetFormsfunctions to utilizefxSampleFormsalign with the refactoring efforts and streamline the retrieval of form data for testing purposes.Also applies to: 17-17
21-23: LGTM!The modification to
testFormexport simplifies the retrieval of a specific test form and aligns with the suggestion from the past review comments to export a single test form.
29-29: LGTM!The streamlining of form data handling in the
routeForms,routeForm, androuteFormByActionfunctions enhances clarity and reduces redundancy. Directly utilizing thegetFormfunction simplifies the retrieval of form data for testing purposes. These changes align with the suggestion from the past review comments to remove theformIdfrom therouteFormXfunctions.Also applies to: 36-38, 46-48
41-41: LGTM!Using the
datavariable directly in the intercepted response ofrouteFormandrouteFormByActionfunctions simplifies the code and reduces unnecessary complexity. Setting theincludedproperty to an empty array ensures a consistent response structure.Also applies to: 51-51
test/support/api/form-responses.js (2)
3-3: LGTM!Importing the
v4function from theuuidlibrary and aliasing it asuuidis a good practice for generating unique identifiers in the code.
24-24: Great improvement!Replacing the hardcoded
idvalue with a dynamically generated UUID using theuuid()function is an excellent change. It ensures that each resource instance has a unique identifier, preventing potential ID collisions and improving the overall integrity of the data being handled.This change enhances the robustness and reliability of the
getFormResponsefunction, which is crucial for applications that rely on accurate resource management.test/integration/forms/preview.js (4)
4-4: LGTM!The import statement for
testFormis syntactically correct and aligns with the PR objective of using fixture IDs directly.
9-13: LGTM!The modification to the
.routeFormfunction to assigntestFormtofx.datais syntactically correct and ensures that the correct form data is used during routing. This change aligns with the PR objective of using fixture IDs directly.
18-18: LGTM!Changing the form ID in the URL from a hardcoded value ('11111') to
testForm.idimproves flexibility and maintainability. This change aligns with the PR objective of using fixture IDs directly.
1-3: LGTM!The remaining code in the file is syntactically correct and covers various test cases for the form preview functionality. The test cases ensure the expected behavior of the form preview.
Also applies to: 5-8, 14-17, 19-114
test/integration/patients/patient/archive.js (1)
355-355: LGTM!The URL assertion has been updated to dynamically include the
testForm.idinstead of a hardcoded value. This improves the flexibility of the test by ensuring it checks for the correct URL based on the specific form being tested.test/integration/outreach/outreach.js (8)
Line range hint
11-86: LGTM!The test case for the opt-in success scenario is comprehensive and covers the expected behavior. The test case interacts with the UI, makes assertions on the request body, and verifies the success message. No changes are required.
Line range hint
88-129: LGTM!The test case for the opt-in error scenario is comprehensive and covers the expected behavior. The test case interacts with the UI, mocks the error response, and verifies the error message. No changes are required.
157-161: Approve the changes torouteFormByAction.The changes made to the
routeFormByActionfunction call are appropriate. AssigningtestFormtofx.dataensures that a specific test form is used for the verification success scenario. This change aligns with the test case's purpose and does not introduce any issues.
Line range hint
301-319: LGTM!The test case for the user verification scenario with an API error while creating a new code is focused and covers the expected behavior. The test case mocks the error response and verifies the error message. No changes are required.
Line range hint
321-382: LGTM!The test case for the user verification scenario when the user enters an invalid code is focused and covers the expected behavior. The test case mocks the error response, interacts with the UI, and verifies the error state. No changes are required.
467-471: Approve the changes torouteFormByAction.The changes made to the
routeFormByActionfunction call are appropriate. AssigningtestFormtofx.dataensures that a specific test form is used for the form submission scenario. This change aligns with the test case's purpose and does not introduce any issues.
Line range hint
574-580: LGTM!The assertions made on the form response data are consistent with the test case's purpose. The test case verifies the relationships between the form response, action, and form, as well as the values of the form fields and patient data. The assertions align with the form fields set up in the
routeFormActionFieldsfunction. No changes are required.
608-619: Approve the changes torouteFormByAction.The changes made to the
routeFormByActionfunction call are appropriate. Using thegetFormfunction to create a read-only form with specific attributes ensures that the test case uses a read-only form for verification. This change aligns with the test case's purpose and does not introduce any issues.test/integration/globals/app-nav.js (6)
11-11: LGTM!The import statement for
testFormis syntactically correct and aligns with the provided list of alterations. It is likely used in the test cases.
1026-1027: LGTM!Setting the
form_idandsubmit_textproperties of thepatient_creation_formsetting usingtestForm.idaligns with the objective of replacing hardcoded values with dynamic references. This change enhances the flexibility and maintainability of the test code.
1104-1108: LGTM!Setting the response body of the form route to
testFormaligns with the objective of replacing hardcoded values with dynamic references. This change enhances the flexibility and maintainability of the test code.
1126-1126: LGTM!Using
testForm.idin the assertion text aligns with the objective of replacing hardcoded values with dynamic references. This change enhances the flexibility and maintainability of the test code.
1132-1132: LGTM!Using
testForm.idin the URL assertion aligns with the objective of replacing hardcoded values with dynamic references. This change enhances the flexibility and maintainability of the test code.
Line range hint
1-1135: Excellent test coverage and structure!The test file
app-nav.jscontains well-structured test cases that cover a wide range of scenarios related to app navigation. The test cases use appropriate assertions to verify the expected behavior and handle different configurations and settings, ensuring comprehensive test coverage.The use of dynamic references (
testForm.id) instead of hardcoded values enhances the maintainability of the test code. The test cases follow a consistent pattern and adhere to best practices for writing integration tests.Overall, the test file demonstrates a high level of quality and thoroughness in testing the app navigation functionality.
test/integration/patients/worklist/schedule.js (8)
Line range hint
46-46: LGTM!The
contextfunction is correctly used to define the test suite for the schedule page. The test suite name accurately describes the feature being tested, and the multiplespecifyblocks cover various test scenarios.
Line range hint
48-289: LGTM!The test case thoroughly verifies the display of the schedule page. It sets up realistic test data, routes necessary API requests, and makes comprehensive assertions on the rendered page. The test case also interacts with the page to verify the expected behavior. The test coverage for the display of the schedule page looks good.
Line range hint
291-358: LGTM!The test case properly verifies the behavior when the maximum list count is reached on the schedule page. It sets up a sufficient number of test actions, verifies the display of the "maximum list count reached" message, and ensures that the message updates correctly based on the search results. The test coverage for this scenario looks good.
Line range hint
360-600: LGTM!The test case comprehensively verifies the filtering functionality on the schedule page. It covers the owner filter and various date filter scenarios, ensuring that the selected filters are stored correctly and the API requests are updated with the appropriate filter parameters. The test case interacts with the filter components and verifies the expected behavior. The test coverage for the schedule filtering functionality looks good.
Line range hint
602-611: LGTM!The test case correctly verifies the behavior for a restricted employee on the schedule page. It sets up a test clinician with a restricted role, visits the schedule page, and ensures that the owner filter is not displayed. The test coverage for this scenario looks good.
Line range hint
613-760: LGTM!The test case thoroughly verifies the bulk edit functionality on the schedule page. It covers various scenarios such as action selection, updating action attributes (due date and owner), and verifying the API requests and success/error messages. The test case also ensures that the bulk edit cancel button and clear selection functionality work as expected. The test coverage for the bulk edit feature looks comprehensive.
Line range hint
762-779: LGTM!The test case correctly verifies the behavior when the schedule is empty. It routes an empty actions API response, visits the schedule page, and ensures that the empty state is displayed correctly. It checks for the empty action count region, the presence of the "No Scheduled Actions" message, and the disabled state of the select all button. The test coverage for the empty schedule scenario looks good.
Line range hint
781-1000: LGTM!The test case comprehensively verifies the "find in list" functionality on the schedule page. It covers various scenarios such as searching for non-existent items, searching for specific terms, and verifying the search result count and displayed actions. The test case also ensures that the search state is maintained when navigating away and returning back to the schedule page, and verifies the behavior of the search clear button. The test coverage for the "find in list" feature looks thorough.
test/integration/patients/patient/patient-flow.js (2)
13-13: LGTM!The removal of the
getFormimport and directly usingtestForminstead simplifies the code.
1285-1285: LGTM!Including
testFormin thefx.includedarray ensures its availability in the response payload, aligning with the change to directly usetestForm.test/integration/patients/worklist/worklist.js (13)
Line range hint
41-280: Test case looks good!The
flow listtest case thoroughly verifies the behavior of the flow list on the worklist page. It covers important scenarios such as filtering, sorting, selecting flows, and navigating to the flow and patient pages. The assertions made in the test are appropriate and ensure the expected functionality.
Line range hint
282-324: Test case looks good!The
done flow listtest case verifies the behavior of the done flow list on the worklist page. It ensures that done flows are displayed correctly based on the filter parameters and can be moved back to the in-progress state. The assertions made in the test are appropriate and cover the expected functionality.
Line range hint
326-827: Test case looks good!The
action listtest case thoroughly verifies the behavior of the action list on the worklist page. It covers important scenarios such as filtering, sorting, updating action attributes, and navigating to the action and patient pages. The assertions made in the test are appropriate and ensure the expected functionality.
Line range hint
829-849: Test case looks good!The
actions on a done-flow listtest case verifies that action metadata is not displayed for done flows on the worklist page. The test setup and assertions are appropriate and ensure the expected behavior.
Line range hint
851-961: Test case looks good!The
maximum list count reachedtest case verifies the behavior when the maximum list count is reached on the worklist page. It ensures that an appropriate message is displayed to indicate that only a subset of results is shown and that filtering the list updates the message accordingly. The test setup and assertions are comprehensive and cover the expected functionality.
Line range hint
963-969: Test case looks good!The
non-existent worklisttest case verifies that navigating to a non-existent worklist URL displays a 404 page. The test is straightforward and ensures the expected behavior.
Line range hint
971-1100: Test case looks good!The
clinician filteringtest case thoroughly verifies the behavior of clinician filtering on the worklist page. It ensures that clinicians are filtered based on their role and team, and the worklist is updated accordingly. The test setup and assertions cover different scenarios and ensure the expected functionality.
Line range hint
1102-1180: Test case looks good!The
owner filteringtest case thoroughly verifies the behavior of owner filtering on the worklist page. It ensures that owners are filtered based on their team and the "No Owner" option, and the worklist is updated accordingly. The test setup and assertions cover different scenarios and ensure the expected functionality.
Line range hint
1750-1765: Test case looks good!The
restricted employeetest case verifies that the owner filter region is empty for a restricted employee on the worklist page. The test setup and assertions are appropriate and ensure the expected behavior for a restricted employee.
Line range hint
1767-1786: Test case looks good!The
restricted employee - shared bytest case verifies the behavior for a restricted employee on the "Shared By" worklist. It ensures that the owner filter region is present but the owner toggle region is empty. The test setup and assertions are appropriate and cover the expected behavior for a restricted employee on the "Shared By" worklist.
Line range hint
1788-1907: Test case looks good!The
flow sortingtest case thoroughly verifies the behavior of flow sorting on the worklist page. It covers different sorting options such as updated date, created date, and patient name, ensuring that the flows are sorted correctly based on the selected option. The test setup and assertions are comprehensive and ensure the expected functionality.
Line range hint
1909-1961: Test case looks good!The
flow sorting - patienttest case verifies the behavior of flow sorting based on the patient's last name. It ensures that flows are sorted correctly in ascending or descending order of the patient's last name. The test setup and assertions are appropriate and cover the expected functionality.
Line range hint
1963-2031: Test case looks good!The
flow sorting alphabetical - patient fieldtest case verifies the behavior of flow sorting based on a patient field value alphabetically. It ensures that flows are sorted correctly in ascending or descending alphabetical order of the patient field value. The test setup and assertions are appropriate and cover the expected functionality.test/integration/formservice/formservice.js (6)
5-7: Imports of test data helpers are appropriateThe added imports for
getForm,getFlow, andgetPatientare necessary for creating test data and improve the test setup.
162-163: Initialization of test data with helper functionsUsing
getFlow()andgetPatient()to generate test data aligns with the refactoring goals and enhances test maintainability.
187-191: Correctly routingrouteFormByActionwith test dataThe
routeFormByActionis properly set up withtestReportForm, ensuring the test uses the correct form data.
196-196: Properly assigningtestActiondata inrouteActionThe test action data is correctly assigned with
fx.data = testAction, ensuring the mocked route uses the intended action.
201-201: Navigating to the correct URL using dynamictestAction.idThe test navigates to the correct URL using
testAction.id, replacing hardcoded values with dynamic references.
208-209: Assertions use dynamic IDs for flow and patientThe assertions correctly verify the URL parameters using
testFlow.idandtestPatient.id, ensuring the test reflects dynamic data.test/integration/patients/patient/sidebar.js (9)
13-13: Import statement added correctlyThe import of
getFormandtestFormfrom'support/api/forms'is appropriate and necessary for the refactored tests.
34-45: Definition oftestScriptReducerFormis accurateThe
testScriptReducerFormis properly defined with the requiredattributesandoptions, includingcontextandreducers. This enhances the flexibility and maintainability of the test code.
52-55: Routing updated to usetestScriptReducerFormSetting
fx.datatotestScriptReducerFormensures that the correct form is used in the route. This aligns with the refactoring objective of using dynamic references.
101-101: Usage oftestForm.idinformWidgetis appropriateThe
form_idfor theformWidgetis correctly set totestForm.id, reflecting the intended form reference.
110-110:formModalWidgetcorrectly referencestestScriptReducerForm.idUsing
testScriptReducerForm.idfor theformModalWidgetensures that this widget interacts with the appropriate form as defined in the new test setup.
120-120:formModalWidgetSmallcorrectly usestestForm.idThe
form_idis appropriately set totestForm.idfor theformModalWidgetSmall, maintaining consistency in form references.
131-131:formModalWidgetLargecorrectly usestestForm.idSetting
form_idtotestForm.idfor theformModalWidgetLargeis correct and consistent with the use oftestFormin similar widgets.
212-212: Test assertion correctly verifiestestScriptReducerForm.idin URLConfirming that the URL contains
testScriptReducerForm.idensures that navigation and routing are functioning as expected with the new form references.
393-393: URL verification uses dynamictestForm.idThe test accurately checks the URL using the dynamic
testForm.id, which aligns with the refactoring goal of replacing hardcoded IDs.test/integration/forms/form.js (36)
11-11: ImportinggetFormandtestFormcorrectlyThe
getFormandtestFormare properly imported from'support/api/forms', ensuring that the necessary functions are available for the tests.
13-13: InitializingtestPatient
testPatientis correctly initialized usinggetPatient(), providing a consistent test patient for the test cases.
17-17: Generating unique IDs usinguuidThe
idfield is correctly set usinguuid(\resource:field:${ name }`, testPatient.id), ensuring that the patient field IDs are unique and associated with thetestPatient`.
36-40: Setting uptestActionwithtestFormrelationshipThe
testActionobject is properly created with a relationship totestForm, which is essential for testing the form actions.
74-77: AssigningtestActionto fixture dataThe fixture's data is correctly set to
testAction, ensuring that the test action is used in subsequent steps.
78-79: Routing form by action usingtestFormThe
.routeFormByActionfunction appropriately assignstestFormtofx.data, ensuring that the form data is correctly loaded during the test.
107-107: Dynamic clinician retrieval with correct team IDThe custom script correctly uses
getClinicians({ teamId: ${ teamCoordinator.id } });to fetch clinicians associated with the team coordinator, enhancing the accuracy of the test.
128-128: Visiting the correct patient action form URLThe
.visit()function constructs the URL usingtestAction.idandtestForm.id, ensuring that the test navigates to the intended patient action form.
163-168: Setting uptestActionfor directory testsThe
testActionis correctly initialized with a relationship totestForm, preparing the test environment for directory-related form tests.
180-180: AssigningtestActionto fixture dataFixture data is set to
testAction, ensuring that the action data is properly utilized in the test.
184-184: Correct routing with_.identityandtestForm.idUsing
_.identitywithtestForm.idin.routeFormByAction()ensures that the correct form data is returned without alterations, maintaining test integrity.
235-235: Visiting the appropriate form URL in directory testsThe
.visit()function correctly navigates usingtestAction.idandtestForm.id, ensuring accurate test execution for directory functionalities.
287-287: AssigningtestPatientto fixture dataThe fixture data is set to
testPatient, maintaining consistency in patient data across tests.
295-295: Intercepting GET request for patient fieldbarThe
.intercept()function captures GET requests to/api/patients/${ testPatient.id }/fields/bar, enabling simulation of error responses for testing error handling.
300-300: Intercepting PATCH request for patient fieldfooBy intercepting the PATCH request to update
foo, the test can verify the application's handling of successful patient field updates.
304-304: Intercepting PATCH request for patient fieldbarwith error simulationIntercepting the PATCH request to
/api/patients/${ testPatient.id }/fields/barand simulating a 400 error allows testing of error handling during patient field updates.
383-387: Setting up form fixture withtestFormThe form fixture is correctly assigned
testForm, and thefxobject is properly returned, ensuring that form data is accurately loaded for the test.
389-389: Navigating to the correct patient form URLThe
.visit()function navigates to/patient/${ testPatient.id }/form/${ testForm.id }, ensuring the test targets the right patient form.
448-450: Asserting correct patient field IDThe test correctly asserts that
data.idmatches the expected UUID, verifying that patient field operations use the correct identifiers.
475-475: Validating request body data for patient fieldbarThe assertion ensures that the
request.body.data.idis correctly generated, confirming that updates to patient fieldbarare accurately formed.
497-497: ReusingtestPatientfixture for ICD code testsConsistently using
testPatientensures that the ICD code tests have a reliable patient context.
585-589: AssigningtestFormto form fixture in ICD code testsThe form fixture is properly set to
testForm, ensuring that the ICD code tests utilize the correct form.
648-660: Creating test form with custom scripts and reducersThe
testScriptReducerFormincorporates customcontextandreducers, effectively testing the form's capability to execute custom scripts and manipulate form submission data.
666-669: AssigningtestPatientto fixture data in script reducer testsSetting
fx.datatotestPatientmaintains patient data consistency in tests involving script reducers.
670-671: Setting form fixture totestScriptReducerFormThe form fixture is correctly set to
testScriptReducerForm, ensuring that tests execute with the intended form containing custom scripts.
695-696: Intentional syntax error in reducers for error handling testsThe
reducersarray contains a syntax error'syntaxError(\'foo;'. Verify that this is intentionally included to test error handling mechanisms and ensure the test appropriately captures and asserts the expected error behavior.
703-706: AssigningtestPatientto fixture data in reducer error testsThe fixture data is set to
testPatient, providing a consistent context for the reducer error tests.
707-708: Setting form fixture totestReducerErrorFormAssigning
testReducerErrorFormto the form fixture ensures that the test evaluates the form with the intentional syntax error in reducers.
745-749: Intentional syntax error inbeforeSubmitfunctionThe
beforeSubmitscript includes a syntax error'syntaxError(\'foo;'. Confirm that this is deliberate for testing error handling during form submission and that the test accurately verifies the application's response.
755-758: AssigningtestPatientto fixture data inbeforeSubmiterror testsFixture data is properly set to
testPatient, maintaining consistency in the test environment.
759-760: Setting form fixture totestBeforeSubmitErrorFormThe form fixture is correctly assigned
testBeforeSubmitErrorForm, which contains the intentional syntax error in thebeforeSubmitscript.
809-814: Intentional syntax error insubmitReducersfor error handlingThe
submitReducersarray includes a syntax error'syntaxError(\'foo;'. Verify that this is intended for testing error handling during form submission reducers and that the test properly asserts the expected application behavior.
821-824: AssigningtestPatientto fixture data insubmitReducerserror testsSetting
fx.datatotestPatientmaintains a consistent patient context for the tests.
825-826: Setting form fixture totestSubmitReducerErrorFormAssigning
testSubmitReducerErrorFormto the form fixture ensures that the tests evaluate the form with the intentional syntax error insubmitReducers.
885-888: AssigningtestPatientto fixture data in duplicate form services testsUsing
testPatientensures consistency in patient data when testing duplicate form services.
889-890: Setting form fixture totestFormin duplicate services testsAssigning
testFormto the form fixture ensures that the correct form is used during the duplicate services tests.test/integration/forms/patient.js (2)
148-148: Ensure consistency in form response ID usageAfter updating the form response ID to be dynamic, remember to replace all instances where
'12345'is used. This ensures consistency throughout the test and prevents potential mismatches.Apply this diff to update the assertion:
- cy.get('iframe').should('have.attr', 'src', '/formapp/12345'); + cy.get('iframe').should('have.attr', 'src', `/formapp/${ testFormResponse.id }`);Likely invalid or redundant comment.
931-949: EnsuretestSubmitHiddenFormis properly utilizedIn the 'hidden submit button' test, you define
testSubmitHiddenFormwith thesubmit_hiddenoption set totrue. Verify that this form is correctly used in the test and that all references to form IDs are updated to usetestSubmitHiddenForm.id.Run the following script to check for any hardcoded form IDs:
test/integration/forms/action.js (4)
14-16: Imports updated appropriatelyThe new imports for
getForm,testForm,getFlow, andgetWorkspacePatientare correctly added and support the refactored tests.
31-32: Definition ofcurrentClinicianis appropriateInitializing
currentClinicianenhances test accuracy by using the current clinician's data.
33-40: Properly definingtestReadOnlyFormThe
testReadOnlyFormis correctly configured with theread_onlyoption, which is essential for testing read-only form scenarios.
41-48: Properly definingtestSubmitHiddenFormThe
testSubmitHiddenFormis appropriately set up with thesubmit_hiddenoption to test forms with hidden submit buttons.
| import fxTestForms from 'fixtures/test/forms'; | ||
| import fxTestForms from 'fixtures/collections/forms'; | ||
|
|
||
| const fxSampleForms = _.rest(fxTestForms, 2); |
There was a problem hiding this comment.
Consider generating generic forms for testing.
The introduction of fxSampleForms focuses on a subset of forms for testing. However, as mentioned in the past review comments, it might be beneficial to generate generic forms for testing instead of relying on hardcoded forms. This approach would provide more flexibility and maintainability in the long run.
| .routeLatestFormResponse() | ||
| .visit('/patient-action/1/form/11111') | ||
| .visit(`/patient-action/1/form/${ testForm.id }`) |
There was a problem hiding this comment.
Replace hardcoded action ID '1' with testAction.id in URLs
The code uses a hardcoded action ID '1' in the .visit() and .should('not.contain') methods. For consistency and to prevent potential issues if the action ID changes, consider replacing '1' with testAction.id.
Apply this diff to fix the issue:
- .visit(`/patient-action/1/form/${ testForm.id }`)
+ .visit(`/patient-action/${ testAction.id }/form/${ testForm.id }`)- .should('not.contain', `/patient-action/1/form/${ testForm.id }`);
+ .should('not.contain', `/patient-action/${ testAction.id }/form/${ testForm.id }`);Also applies to: 75-75
There was a problem hiding this comment.
I think maybe we should make these ids something more obvious.. like deletedActionId which we could use in the intercept route.. and is just a deletedActionId = uuid()
There was a problem hiding this comment.
@paulfalgout, that's a great idea. Defining a deletedActionId using uuid() will make the test more explicit and avoid hardcoding IDs. Here's how we can update the code:
// At the beginning of the test
const deletedActionId = uuid();
// Use deletedActionId in the intercept route
.intercept('GET', `/api/actions/${ deletedActionId }`, {
statusCode: 410,
body: { errors },
})
.as('routeActionError')
// Update the visit and assertion methods
.visit(`/patient-action/${ deletedActionId }/form/${ testForm.id }`)
.wait('@routePatientByAction')
.wait('@routeActionError');
cy
.get('.alert-box__body')
.should('contain', 'The Action you requested does not exist.');
cy
.url()
.should('not.contain', `/patient-action/${ deletedActionId }/form/${ testForm.id }`);This approach improves clarity by explicitly indicating that the action ID represents a deleted action and ensures we're using a unique ID in the test.
Learnings added
Learnt from: paulfalgout
PR: RoundingWell/care-ops-frontend#1312
File: test/integration/forms/action.js:65-65
Timestamp: 2024-09-19T23:36:16.826Z
Learning: In tests involving deleted or non-existent actions, define a `deletedActionId` using `uuid()` and use it in place of hardcoded IDs or `testAction.id` to improve clarity and avoid potential conflicts.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
nmajor25
force-pushed
the
action-forms-refactor
branch
from
38d7f06
to
2be2798
Compare
Configure unique form options in the tests, instead of fixtures
nmajor25
force-pushed
the
action-forms-refactor
branch
from
2be2798
to
e337e36
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
test/support/api/forms.js (1)
6-6: Ensure adequate testing coverage with the subset of forms.The introduction of
fxSampleFormsfocuses on a subset of forms for testing, which can help streamline the testing process and improve maintainability. However, it's important to ensure that this subset of forms provides adequate coverage for different form scenarios to maintain the overall quality of the testing framework.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- test/fixtures/config/forms.js (1 hunks)
- test/fixtures/test/forms.json (0 hunks)
- test/integration/forms/action.js (66 hunks)
- test/integration/forms/form.js (19 hunks)
- test/integration/forms/patient.js (29 hunks)
- test/integration/forms/preview.js (1 hunks)
- test/integration/formservice/formservice.js (2 hunks)
- test/integration/globals/app-nav.js (4 hunks)
- test/integration/outreach/outreach.js (5 hunks)
- test/integration/patients/patient/archive.js (1 hunks)
- test/integration/patients/patient/patient-flow.js (2 hunks)
- test/integration/patients/patient/sidebar.js (10 hunks)
- test/integration/patients/worklist/schedule.js (3 hunks)
- test/integration/patients/worklist/worklist.js (3 hunks)
- test/support/api/form-responses.js (2 hunks)
- test/support/api/forms.js (1 hunks)
Files not reviewed due to no reviewable changes (1)
- test/fixtures/test/forms.json
Files skipped from review as they are similar to previous changes (11)
- test/fixtures/config/forms.js
- test/integration/forms/form.js
- test/integration/forms/preview.js
- test/integration/formservice/formservice.js
- test/integration/globals/app-nav.js
- test/integration/outreach/outreach.js
- test/integration/patients/patient/archive.js
- test/integration/patients/patient/patient-flow.js
- test/integration/patients/patient/sidebar.js
- test/integration/patients/worklist/schedule.js
- test/integration/patients/worklist/worklist.js
Additional comments not posted (28)
test/support/api/forms.js (4)
4-5: Consider generating generic forms for testing.The import path for test forms has been updated, indicating a reorganization of the test data structure. However, as mentioned in the past review comments, it might be beneficial to generate generic forms for testing instead of relying on hardcoded forms. This approach would provide more flexibility and maintainability in the long run.
11-11: LGTM! The changes align with the PR objectives.The modifications to the
getFormandgetFormsfunctions, as well as the update to thetestFormexport, align with the overall objective of the PR to focus on a specific subset of forms for testing. These changes simplify the retrieval of form data and improve code clarity.However, it's important to ensure that the subset of forms used in these functions still provides adequate coverage for different form scenarios to maintain the overall quality of the testing framework.
Also applies to: 17-17, 21-23
29-29: The streamlining of form data handling improves code quality.The changes made to the
routeForms,routeForm, androuteFormByActionCypress commands significantly improve code clarity, maintainability, and consistency in handling form data during testing. The removal of the cloning and modification logic simplifies the command definitions, while the direct utilization of thegetFormfunction ensures a consistent approach to retrieving form data across the commands.These modifications align with the overall objective of the PR to enhance the testing framework and contribute to better code quality.
Also applies to: 36-38, 41-41
46-48: LGTM! The update torouteFormByActionis consistent with other changes.The modification to the
routeFormByActionCypress command, which now directly utilizes thegetFormfunction, is consistent with the changes made to the other Cypress commands in this file. This update improves code clarity and ensures a consistent approach to retrieving form data across the commands.The change aligns with the overall objective of the PR to enhance the testing framework and contribute to better code quality.
Also applies to: 51-51
test/support/api/form-responses.js (2)
3-3: LGTM!The import statement for the
uuidlibrary is correctly implemented, enabling the generation of unique identifiers for resources.
24-24: Great improvement!Replacing the hardcoded
idvalue with a dynamically generated UUID using theuuid()function ensures unique identifiers for resources. This change enhances data integrity, prevents ID collisions, and improves the accuracy of the test suite.test/integration/forms/patient.js (11)
37-41: Great job refactoring the test to use dynamic references!The changes made to use
testPatientandtestForminstead of hardcoded values improve the maintainability and reusability of the test. The verification of the request payload sent during form submission is also a good addition to ensure the form data is submitted as expected.Also applies to: 60-64, 148-153
177-181: The refactoring of the auto-save functionality test looks good!Using dynamic references like
currentClinician.id,testPatient.id, andtestForm.idto construct the local storage key ensures uniqueness and accuracy. Verifying the auto-saved data in local storage is a great way to confirm that the functionality is working as expected.Also applies to: 186-189, 212-214
242-247: The test for restoring draft form submissions looks great!Using dynamic references to set and retrieve the draft data from local storage ensures that the draft is associated with the correct clinician, patient, and form. Verifying that the draft data is restored correctly when the form is loaded is a good way to confirm that the functionality is working as expected.
Also applies to: 250-254, 272-276
301-306: The test for restoring stored form submissions looks great!Using dynamic references to set and retrieve the stored submission data from local storage ensures that the data is associated with the correct clinician, patient, and form. Verifying that the stored submission data is restored correctly when the form is loaded is a good way to confirm that the functionality is working as expected.
Also applies to: 309-313, 331-335
360-365: The test for discarding stored form submissions looks great!Using dynamic references to set the stored submission data in local storage ensures that the data is associated with the correct clinician, patient, and form. Verifying that the stored submission data is discarded when the user clicks the discard button is a good way to confirm that the functionality is working as expected.
Also applies to: 368-372, 375-378, 391-393
Line range hint
429-434: The test for the read-only functionality of a form looks great!Using a dynamic reference to set the read-only form data in local storage ensures that the correct form is being tested. Verifying that the form is displayed as read-only and the submit button is disabled is a good way to confirm that the functionality is working as expected.
Also applies to: 438-441, 442-446, 460-464
488-489: The test for storing the expanded state of the form in local storage looks great!Using a dynamic reference to set and retrieve the expanded state ensures that it is associated with the correct clinician. Verifying that the expanded state is stored and retrieved correctly is a good way to confirm that the functionality is working as expected.
Also applies to: 492-495, 496-500, 504-508, 533-536, 543-546
552-561: The test for the form header widgets functionality looks great!Using a dynamic reference to create a test form with specific widget options ensures that the correct form configuration is being tested. Verifying that the form header widgets are displayed correctly based on the patient data is a good way to confirm that the functionality is working as expected.
Also applies to: 564-568, 598-607, 615-623
641-642: The test for the "Submit and Go Back" button functionality looks great!Using dynamic references to set and retrieve the save button type ensures that it is associated with the correct clinician. Verifying that the save button type is stored and retrieved correctly, and the user is redirected to the patient dashboard after submission is a good way to confirm that the functionality is working as expected.
Also applies to: 645-649, 668-671, 672-677, 716-719, 739-742, 776-781, 786-787
791-795: The test for the error handling of the "Submit and Go Back" button functionality looks great!Using dynamic references to set up the test form and patient data ensures that the correct form and patient are being tested. Verifying that the error message is displayed correctly when there is an error in the form response is a good way to confirm that the error handling is working as expected.
Also applies to: 800-803, 804-810
931-938: The test for the hidden submit button functionality looks great!Using a dynamic reference to create a test form with the submit button hidden ensures that the correct form configuration is being tested. Verifying that the submit button is not displayed when the
submit_hiddenoption is set totrueis a good way to confirm that the functionality is working as expected.Also applies to: 940-944, 949-954
test/integration/forms/action.js (11)
Line range hint
24-32: LGTM!The
contextfunction andbeforeEachhook are set up correctly to define the test suite and common routes for patient action forms.
Line range hint
49-76: Test case looks good!The test case correctly mocks a 410 error response for the deleted action and verifies the error message and URL.
Line range hint
78-127: Test case looks good!The test case correctly sets up the test data, mocks the API routes, and verifies that the form can be updated.
Line range hint
129-207: Test case looks good!The test case correctly sets up the test data, mocks the API routes, fills out the form, and verifies the stored submission and auto-save behavior.
Line range hint
209-277: Test case looks good!The test case correctly sets up the test data, mocks the API routes, and verifies that the stored submission is restored and submitted.
Line range hint
279-371: Test case looks good!The test case correctly sets up the test data, mocks the API routes, and verifies that the form draft is restored and submitted.
Line range hint
373-502: Test case looks good!The test case correctly sets up the test data, mocks the API routes, and verifies that the stored submission is discarded and a new submission is created.
Line range hint
504-585: Test case looks good!The test case correctly sets up the test data, mocks the API routes, and verifies that the form is prefilled with the latest submission data by flow.
Line range hint
587-676: Test case looks good!The test case correctly sets up the test data, mocks the API routes, and verifies that the form is prefilled with the latest submission data from another form.
Line range hint
678-772: Test case looks good!The test case correctly sets up the test data, mocks the API routes, and verifies that the form is prefilled with the latest submission data based on the action tag.
Line range hint
774-832: Test case looks good!The test case correctly sets up the test data, mocks the API routes, and verifies that the form is populated with the response data and can be updated.
| export default () => { | ||
| return { | ||
| id: faker.datatype.uuid(), | ||
| name: `${ faker.company.bsBuzz() } ${ faker.company.catchPhraseNoun() }`, |
There was a problem hiding this comment.
I think we need to add the created_at updated_at submitted_at timestamps here
| .routeLatestFormResponse() | ||
| .visit('/patient-action/1/form/11111') | ||
| .visit(`/patient-action/1/form/${ testForm.id }`) |
There was a problem hiding this comment.
I think maybe we should make these ids something more obvious.. like deletedActionId which we could use in the intercept route.. and is just a deletedActionId = uuid()
|
|
||
| return fx; | ||
| }) | ||
| .routeFormByAction(_.identity, '11111') | ||
| .routeFormByAction(_.identity, testForm.id) |
There was a problem hiding this comment.
I kinda think we could remove the 2nd argument.. it's a strange pattern now.. also we can probably default routeFormByAction to return the testForm and make everything else the exception
There was a problem hiding this comment.
actually it looks like you did remove it, so this isn't doing anything.. no need to pass the _.identitys
| .routeFormByAction(fx => { | ||
| fx.data = getForm({ | ||
| attributes: { | ||
| name: 'Read Only Test Form', |
| fx.included.push( | ||
| getForm({ id: '11111', attributes: { name: 'Test Form' } }), | ||
| ); | ||
| fx.included.push(testForm); |
There was a problem hiding this comment.
I think this could be removed.. we don't include forms on this endpoint
| import fxTestForms from 'fixtures/test/forms'; | ||
| import fxTestForms from 'fixtures/collections/forms'; | ||
|
|
||
| const fxSampleForms = _.rest(fxTestForms, 2); |
There was a problem hiding this comment.
since we're only using fxTextForms[0] this should be _.rest(..., 1)
| cy | ||
| .intercept('GET', '/api/forms', { | ||
| body: mutator({ | ||
| data: getResource(fxForms, TYPE), | ||
| data: getResource(fxTestForms, TYPE), |
There was a problem hiding this comment.
I think this should be data: [testForm, ...getResource(fxSampleForms, TYPE)],
There was a problem hiding this comment.
otherwise this the forms url will contain the testForm.id with different attributes than loaded elsewhere.
| const resource = getResource(_.sample(fxTestForms), TYPE); | ||
|
|
||
| data = _.extend({ id: uuid() }, data); | ||
| const resource = getResource(_.sample(fxSampleForms), TYPE); |
There was a problem hiding this comment.
we don't really use a random form.. hardly ever.. I think this could be:
export function getForm(data) {
return mergeJsonApi(testForm, data);
}
Changes to match the patterns in the recent refactoring we've done on other tests.
Use the generated
uuidids where we can. And reference fixture id's directly when possible (example:'11111'=>testForm.id).Summary by CodeRabbit
New Features
Bug Fixes
Refactor