Add Cypress Test Suite for Sample Test Request Workflow #8977
Conversation
❌ Deploy Preview for care-ohc failed.
|
|
|
||
| it("should request a new sample test", () => { | ||
| sampleTestPage.visitPatientPage(); | ||
| sampleTestPage.visitPatientDashboardPage(); |
There was a problem hiding this comment.
| sampleTestPage.visitPatientDashboardPage(); | |
| patientPage.visitPatient(patientName); |
- reuse existing component, and use them in your test by importing the function
- always try to click on specific patient card, rather than random card
- keep all the input const in the test file itself
…926/add-sample-test
| it("should request a new sample test", () => { | ||
| sampleTestPage.visitPatientPage(); | ||
| sampleTestPage.visitPatientDashboardPage(); | ||
| sampleTestPage.visitSampleRequestPage(); |
There was a problem hiding this comment.
- before this line, reuse the function to click patient details function
- after that only add this line, where you are clicking the request sample request page button
- make sure once the sample request page is loaded, add a cy.intercept and verify the API response to ensure the page is fully loaded before your next line of test is executed
| sampleTestPage.selectSampleType(sampleTestType); | ||
| sampleTestPage.selectIcmrCategory(icmrCategory); | ||
| sampleTestPage.typeIcmrLabel(icmrLabel); | ||
|
|
There was a problem hiding this comment.
- remove unwanted spaces
- input values to all the fields in the form not only the mandatory columns
| sampleTestPage.typeIcmrLabel(icmrLabel); | ||
|
|
||
| // Submit the form | ||
| sampleTestPage.submitForm(); |
There was a problem hiding this comment.
| sampleTestPage.submitForm(); | |
| cy.submitButton("Confirm your request to send sample for testing") |
- always use the reusable commands present in commands.ts file
| sampleTestPage.submitForm(); | ||
|
|
||
| // Check for sample request notification and history | ||
| sampleTestPage.clickOnNotification(); |
There was a problem hiding this comment.
| sampleTestPage.clickOnNotification(); | |
| cy.verifyNotification("Sample test created successfully"); |
WalkthroughThis pull request introduces a new Cypress test specification for requesting a sample test, encapsulated in Changes
Assessment against linked issues
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (8)
cypress/e2e/sample_test_spec/SampleTestRequest.cy.ts (1)
1-8: Consider improving test data management and type safety.
- Move test data to a separate fixture file for better maintainability and reusability.
- Add TypeScript type annotations for the constants.
import { SampleTestPage } from "pageobject/Sample/SampleTestCreate"; +import testData from '../fixtures/sampleTest.json'; describe("Sample Test", () => { const sampleTestPage = new SampleTestPage(); - const sampleTestType = "BA/ETA", - icmrCategory = "Cat 0", - icmrLabel = "Test Icmr Label"; + const sampleTestType: string = testData.sampleTestType; + const icmrCategory: string = testData.icmrCategory; + const icmrLabel: string = testData.icmrLabel;cypress/pageobject/Sample/SampleTestCreate.ts (2)
1-4: Add TypeScript type annotations and improve documentation.Consider adding type annotations and converting the inline comment to JSDoc format for better TypeScript integration and documentation.
export class SampleTestPage { - sampleTestReportHistory = 0; - patientName = ""; // to search for patient in sample page + private sampleTestReportHistory: number = 0; + /** Patient name used for searching in the sample page */ + private patientName: string = "";
1-79: Consider architectural improvements for better maintainability and debugging.While the Page Object Model implementation is good, consider these architectural improvements:
- Add logging for better debugging capabilities
- Create custom Cypress commands for common operations
- Implement retry strategies for flaky operations
- Consider splitting into smaller, more focused classes (e.g., separate navigation, form handling, and verification)
src/components/Patient/SampleTest.tsx (3)
230-230: Consider adding validation and improving accessibility for ICMR Category.While the
id="icmr-category"addition is good for testing, consider these improvements:
- Add validation in
validateForm()to ensure a valid category is selected.- Move the ICMR categories explanation to a separate component or tooltip for better maintainability and accessibility.
256-260: Consider adding format validation for ICMR Label.While the field has required validation, consider adding:
- Pattern validation to ensure the ICMR label follows the expected format
- Helper text to guide users on the expected format
Example implementation:
<TextFormField {...field("icmr_label", "ICMR Label")} required id="icmr-label" + pattern="[A-Z0-9-]+" + helperText="Enter the ICMR label in uppercase with numbers and hyphens only" />
Line range hint
1-326: Consider architectural improvements for better maintainability.While the changes for testing are good, consider these architectural improvements:
- Extract form validation logic into a custom hook for reusability
- Move ICMR categories to a configuration file
- Consider using a form library like React Hook Form or Formik for better form handling
src/components/Patient/SampleViewAdmin.tsx (1)
349-349: LGTM! Consider consistent ID naming.The addition of
id="search_patient_name"improves test automation capabilities. For consistency across the codebase, consider using either kebab-case (likesample-card) or snake_case (like current) for all IDs.src/components/Patient/PatientInfoCard.tsx (1)
Line range hint
1-673: Consider breaking down the component for better maintainability.The
PatientInfoCardcomponent handles multiple responsibilities and could benefit from being split into smaller, more focused components. Consider:
- Extracting modal-related logic into separate components
- Creating a dedicated component for the patient info section
- Moving the dropdown menu options into a separate component
- Consolidating modal state management using a reducer
This would improve code maintainability, readability, and testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
cypress/e2e/sample_test_spec/SampleTestRequest.cy.ts(1 hunks)cypress/pageobject/Sample/SampleTestCreate.ts(1 hunks)src/components/Patient/PatientHome.tsx(1 hunks)src/components/Patient/PatientInfoCard.tsx(1 hunks)src/components/Patient/SampleTest.tsx(4 hunks)src/components/Patient/SampleTestCard.tsx(1 hunks)src/components/Patient/SampleViewAdmin.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Patient/SampleTestCard.tsx
🔇 Additional comments (4)
src/components/Patient/SampleTest.tsx (2)
215-215: LGTM! Verify test coverage for sample type selection.
The addition of id="sample-type" enhances test targeting. The field is properly integrated with form state and validation.
Let's verify the test coverage:
✅ Verification successful
The sample-type field is properly tested in the Cypress test suite
The added id="sample-type" attribute is being utilized effectively in the test suite:
- Implemented in the page object pattern (
SampleTestCreate.ts) - Used in E2E test flow (
SampleTestRequest.cy.ts) - Properly validated with visibility checks before interaction
- Integrated into the complete form submission workflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases targeting the sample-type field
rg -l 'sample-type' cypress/
Length of output: 585
Script:
#!/bin/bash
# Let's search more broadly for any test files related to sample testing
fd . cypress/ -e ts -e js | xargs rg -l "sample.*test" -i
# Also search specifically for the sample type selector in test files
rg -A 5 "#sample-type" cypress/
Length of output: 738
Script:
#!/bin/bash
# Let's check the actual test implementation to verify the sample type testing
rg -A 10 "selectSampleType" cypress/e2e/sample_test_spec/SampleTestRequest.cy.ts
Length of output: 436
322-326: LGTM! Verify test coverage for form submission.
The addition of id="sample-test-submit-btn" enhances test targeting. The submission logic is comprehensive.
Let's verify the test coverage:
src/components/Patient/SampleViewAdmin.tsx (1)
149-149: LGTM! Good test automation support.
The addition of id="sample-card" improves test automation capabilities by making the sample card element easily identifiable in Cypress tests.
src/components/Patient/PatientInfoCard.tsx (1)
258-258: LGTM! Adding span wrapper improves accessibility.
The addition of a span element with id="patient-name" enhances DOM accessibility and makes it easier to target the patient's name for testing or styling purposes.
| it("should verify sample request on sample page", () => { | ||
| sampleTestPage.visitSamplePage(); | ||
| sampleTestPage.searchPatientSample(sampleTestPage.patientName); | ||
| sampleTestPage.patientSampleMustExist(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test independence and add explicit assertions.
The test case appears to depend on the previous test's state and lacks proper assertions.
it("should verify sample request on sample page", () => {
+ cy.intercept('GET', '/api/v1/sample/*').as('getSamples');
sampleTestPage.visitSamplePage();
+ cy.wait('@getSamples');
+ const expectedPatient = "John Doe"; // Move to test data
- sampleTestPage.searchPatientSample(sampleTestPage.patientName);
+ sampleTestPage.searchPatientSample(expectedPatient);
+ cy.get('[data-testid="search-results"]').should('be.visible');
- sampleTestPage.patientSampleMustExist();
+ cy.get('[data-testid="patient-sample"]')
+ .should('contain', expectedPatient)
+ .and('contain', sampleTestType);
});Consider making this test independent by setting up its own test data, rather than relying on the previous test case.
Committable suggestion skipped: line range outside the PR's diff.
| before(() => { | ||
| cy.loginByApi("devdistrictadmin", "Coronasafe@123"); | ||
| cy.saveLocalStorage(); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| cy.restoreLocalStorage(); | ||
| cy.clearLocalStorage(/filters--.+/); | ||
| }); |
There was a problem hiding this comment.
Move credentials to environment variables or test configuration.
Hardcoded credentials in test files pose a security risk and make credential rotation difficult.
before(() => {
- cy.loginByApi("devdistrictadmin", "Coronasafe@123");
+ cy.loginByApi(Cypress.env('ADMIN_USERNAME'), Cypress.env('ADMIN_PASSWORD'));
cy.saveLocalStorage();
});📝 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.
| before(() => { | |
| cy.loginByApi("devdistrictadmin", "Coronasafe@123"); | |
| cy.saveLocalStorage(); | |
| }); | |
| beforeEach(() => { | |
| cy.restoreLocalStorage(); | |
| cy.clearLocalStorage(/filters--.+/); | |
| }); | |
| before(() => { | |
| cy.loginByApi(Cypress.env('ADMIN_USERNAME'), Cypress.env('ADMIN_PASSWORD')); | |
| cy.saveLocalStorage(); | |
| }); | |
| beforeEach(() => { | |
| cy.restoreLocalStorage(); | |
| cy.clearLocalStorage(/filters--.+/); | |
| }); |
| it("should request a new sample test", () => { | ||
| sampleTestPage.visitPatientPage(); | ||
| sampleTestPage.visitPatientDashboardPage(); | ||
| sampleTestPage.visitSampleRequestPage(); | ||
|
|
||
| // Fill form fields | ||
| sampleTestPage.selectSampleType(sampleTestType); | ||
| sampleTestPage.selectIcmrCategory(icmrCategory); | ||
| sampleTestPage.typeIcmrLabel(icmrLabel); | ||
|
|
||
| // Submit the form | ||
| sampleTestPage.submitForm(); | ||
|
|
||
| // Check for sample request notification and history | ||
| sampleTestPage.clickOnNotification(); | ||
| sampleTestPage.checkRequestHistory(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test reliability with API intercepts and assertions.
The test needs improvements in several areas:
- Add API intercepts to verify page loads
- Add assertions between steps
- Use specific patient selection instead of random selection
it("should request a new sample test", () => {
+ cy.intercept('GET', '/api/v1/patient/*').as('getPatient');
sampleTestPage.visitPatientPage();
- sampleTestPage.visitPatientDashboardPage();
+ cy.wait('@getPatient');
+
+ const patientName = "John Doe"; // Move to test data
+ sampleTestPage.visitPatient(patientName);
+ cy.contains(patientName).should('be.visible');
+ cy.intercept('GET', '/api/v1/sample_test/*').as('getSampleTest');
sampleTestPage.visitSampleRequestPage();
+ cy.wait('@getSampleTest');
// Fill form fields
sampleTestPage.selectSampleType(sampleTestType);
+ cy.get('[data-testid="sample-type"]').should('have.value', sampleTestType);
sampleTestPage.selectIcmrCategory(icmrCategory);
+ cy.get('[data-testid="icmr-category"]').should('have.value', icmrCategory);
sampleTestPage.typeIcmrLabel(icmrLabel);
+ cy.get('[data-testid="icmr-label"]').should('have.value', icmrLabel);
// Submit the form
- sampleTestPage.submitForm();
+ cy.submitButton("Confirm your request to send sample for testing");
// Check for sample request notification and history
- sampleTestPage.clickOnNotification();
+ cy.verifyNotification("Sample test created successfully");
sampleTestPage.checkRequestHistory();
});Committable suggestion skipped: line range outside the PR's diff.
| selectSampleType(option: string) { | ||
| cy.get("#sample-type").should("be.visible").click(); | ||
| cy.get("[role='option']").contains(option).click(); | ||
| } | ||
|
|
||
| selectIcmrCategory(option: string) { | ||
| cy.get("#icmr-category").should("be.visible").click(); | ||
| cy.get("[role='option']").contains(option).click(); | ||
| } | ||
|
|
||
| typeIcmrLabel(label: string) { | ||
| cy.get("#icmr-label").should("be.visible").type(label); | ||
| } | ||
|
|
||
| submitForm() { | ||
| cy.get("#sample-test-submit-btn").scrollIntoView(); | ||
| cy.get("#sample-test-submit-btn").should("be.visible").click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling to form interactions.
The form interaction methods need improved validation and error handling:
- Input parameters should be validated
- Form submission should verify form validity
- Error handling for failed interactions is missing
- selectSampleType(option: string) {
+ selectSampleType(option: string): void {
+ if (!option) {
+ throw new Error('Sample type option is required');
+ }
cy.get("#sample-type").should("be.visible").click();
- cy.get("[role='option']").contains(option).click();
+ cy.get("[role='option']")
+ .contains(option)
+ .should("exist")
+ .should("be.visible")
+ .click();
}
submitForm() {
+ // Verify form validity before submission
+ cy.get('form')
+ .invoke('get', 0)
+ .then($form => {
+ expect($form.checkValidity()).to.be.true;
+ });
cy.get("#sample-test-submit-btn").scrollIntoView();
cy.get("#sample-test-submit-btn")
.should("be.visible")
+ .should("be.enabled")
.click();
}Committable suggestion skipped: line range outside the PR's diff.
| visitSampleRequestPage() { | ||
| cy.get("body").then(($body) => { | ||
| if ($body.find("#sample-test-history").length > 0) { | ||
| cy.get("#sample-test-history") | ||
| .its("length") | ||
| .then((count) => { | ||
| this.sampleTestReportHistory = count; | ||
| }); | ||
| } else { | ||
| // Set to 0 if the element does not exist | ||
| this.sampleTestReportHistory = 0; | ||
| } | ||
| }); | ||
| cy.get("#sample-request-btn").scrollIntoView(); | ||
| cy.get("#sample-request-btn").should("be.visible").click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify sample history count logic and add error handling.
The current implementation of sample history counting is overly complex and could be simplified.
visitSampleRequestPage() {
- cy.get("body").then(($body) => {
- if ($body.find("#sample-test-history").length > 0) {
- cy.get("#sample-test-history")
- .its("length")
- .then((count) => {
- this.sampleTestReportHistory = count;
- });
- } else {
- // Set to 0 if the element does not exist
- this.sampleTestReportHistory = 0;
- }
- });
+ // Get sample history count, defaulting to 0 if not found
+ cy.get("#sample-test-history")
+ .then($elements => {
+ this.sampleTestReportHistory = $elements.length;
+ })
+ .catch(() => {
+ this.sampleTestReportHistory = 0;
+ });
+
+ // Verify sample request button exists before clicking
cy.get("#sample-request-btn").scrollIntoView();
- cy.get("#sample-request-btn").should("be.visible").click();
+ cy.get("#sample-request-btn")
+ .should("be.visible")
+ .should("be.enabled")
+ .click();
}📝 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.
| visitSampleRequestPage() { | |
| cy.get("body").then(($body) => { | |
| if ($body.find("#sample-test-history").length > 0) { | |
| cy.get("#sample-test-history") | |
| .its("length") | |
| .then((count) => { | |
| this.sampleTestReportHistory = count; | |
| }); | |
| } else { | |
| // Set to 0 if the element does not exist | |
| this.sampleTestReportHistory = 0; | |
| } | |
| }); | |
| cy.get("#sample-request-btn").scrollIntoView(); | |
| cy.get("#sample-request-btn").should("be.visible").click(); | |
| } | |
| visitSampleRequestPage() { | |
| // Get sample history count, defaulting to 0 if not found | |
| cy.get("#sample-test-history") | |
| .then($elements => { | |
| this.sampleTestReportHistory = $elements.length; | |
| }) | |
| .catch(() => { | |
| this.sampleTestReportHistory = 0; | |
| }); | |
| // Verify sample request button exists before clicking | |
| cy.get("#sample-request-btn").scrollIntoView(); | |
| cy.get("#sample-request-btn") | |
| .should("be.visible") | |
| .should("be.enabled") | |
| .click(); | |
| } |
| visitPatientDashboardPage() { | ||
| cy.get(".patient-stable-ring").first().scrollIntoView(); | ||
| cy.get(".patient-stable-ring").should("be.visible").first().click(); | ||
| cy.get("#patient-name") | ||
| .invoke("text") | ||
| .then((patientName) => { | ||
| this.patientName = patientName.trim(); | ||
| }); | ||
| cy.get("#patient-details").should("be.visible").click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve selector reliability and error handling in patient dashboard navigation.
The current implementation has potential reliability issues:
- The
.patient-stable-ringselector is brittle and could break with UI changes - No error handling for cases where patient details are not found
visitPatientDashboardPage() {
- cy.get(".patient-stable-ring").first().scrollIntoView();
- cy.get(".patient-stable-ring").should("be.visible").first().click();
+ cy.get('[data-testid="patient-card"]')
+ .first()
+ .should("exist")
+ .scrollIntoView()
+ .should("be.visible")
+ .click();
cy.get("#patient-name")
.invoke("text")
.then((patientName) => {
this.patientName = patientName.trim();
+ // Verify patient name was actually extracted
+ expect(this.patientName).to.not.be.empty;
});
cy.get("#patient-details").should("be.visible").click();
}📝 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.
| visitPatientDashboardPage() { | |
| cy.get(".patient-stable-ring").first().scrollIntoView(); | |
| cy.get(".patient-stable-ring").should("be.visible").first().click(); | |
| cy.get("#patient-name") | |
| .invoke("text") | |
| .then((patientName) => { | |
| this.patientName = patientName.trim(); | |
| }); | |
| cy.get("#patient-details").should("be.visible").click(); | |
| } | |
| visitPatientDashboardPage() { | |
| cy.get('[data-testid="patient-card"]') | |
| .first() | |
| .should("exist") | |
| .scrollIntoView() | |
| .should("be.visible") | |
| .click(); | |
| cy.get("#patient-name") | |
| .invoke("text") | |
| .then((patientName) => { | |
| this.patientName = patientName.trim(); | |
| // Verify patient name was actually extracted | |
| expect(this.patientName).to.not.be.empty; | |
| }); | |
| cy.get("#patient-details").should("be.visible").click(); | |
| } |
| clickOnNotification() { | ||
| cy.get(".pnotify-container").should("be.visible").click(); | ||
| } | ||
|
|
||
| checkRequestHistory() { | ||
| cy.get("#sample-test-history").scrollIntoView(); | ||
| cy.get("#sample-test-history").should( | ||
| "have.length", | ||
| this.sampleTestReportHistory + 1, | ||
| ); | ||
| } | ||
|
|
||
| visitSamplePage() { | ||
| cy.awaitUrl("/sample"); | ||
| } | ||
|
|
||
| searchPatientSample() { | ||
| cy.get("#search_patient_name").should("be.visible").type(this.patientName); | ||
| } | ||
|
|
||
| patientSampleMustExist() { | ||
| cy.get("#sample-card").should("be.visible"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance verification methods with better error handling and timeouts.
The verification methods need improvements in reliability and error handling:
- Notification handling should be more robust
- Sample history verification should handle edge cases
- Timeouts should be configured for async operations
clickOnNotification() {
- cy.get(".pnotify-container").should("be.visible").click();
+ cy.get(".pnotify-container", { timeout: 10000 })
+ .should("be.visible")
+ .should("not.be.disabled")
+ .click();
}
checkRequestHistory() {
cy.get("#sample-test-history").scrollIntoView();
- cy.get("#sample-test-history").should(
- "have.length",
- this.sampleTestReportHistory + 1,
- );
+ cy.get("#sample-test-history", { timeout: 10000 })
+ .should("exist")
+ .then($elements => {
+ const currentCount = $elements.length;
+ expect(currentCount).to.equal(
+ this.sampleTestReportHistory + 1,
+ `Expected sample history count to increase by 1 from ${this.sampleTestReportHistory} to ${this.sampleTestReportHistory + 1}, but found ${currentCount}`
+ );
+ });
}
patientSampleMustExist() {
- cy.get("#sample-card").should("be.visible");
+ cy.get("#sample-card", { timeout: 10000 })
+ .should("exist")
+ .should("be.visible")
+ .then($card => {
+ expect($card).to.contain(this.patientName);
+ });
}📝 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.
| clickOnNotification() { | |
| cy.get(".pnotify-container").should("be.visible").click(); | |
| } | |
| checkRequestHistory() { | |
| cy.get("#sample-test-history").scrollIntoView(); | |
| cy.get("#sample-test-history").should( | |
| "have.length", | |
| this.sampleTestReportHistory + 1, | |
| ); | |
| } | |
| visitSamplePage() { | |
| cy.awaitUrl("/sample"); | |
| } | |
| searchPatientSample() { | |
| cy.get("#search_patient_name").should("be.visible").type(this.patientName); | |
| } | |
| patientSampleMustExist() { | |
| cy.get("#sample-card").should("be.visible"); | |
| } | |
| clickOnNotification() { | |
| cy.get(".pnotify-container", { timeout: 10000 }) | |
| .should("be.visible") | |
| .should("not.be.disabled") | |
| .click(); | |
| } | |
| checkRequestHistory() { | |
| cy.get("#sample-test-history").scrollIntoView(); | |
| cy.get("#sample-test-history", { timeout: 10000 }) | |
| .should("exist") | |
| .then($elements => { | |
| const currentCount = $elements.length; | |
| expect(currentCount).to.equal( | |
| this.sampleTestReportHistory + 1, | |
| `Expected sample history count to increase by 1 from ${this.sampleTestReportHistory} to ${this.sampleTestReportHistory + 1}, but found ${currentCount}` | |
| ); | |
| }); | |
| } | |
| visitSamplePage() { | |
| cy.awaitUrl("/sample"); | |
| } | |
| searchPatientSample() { | |
| cy.get("#search_patient_name").should("be.visible").type(this.patientName); | |
| } | |
| patientSampleMustExist() { | |
| cy.get("#sample-card", { timeout: 10000 }) | |
| .should("exist") | |
| .should("be.visible") | |
| .then($card => { | |
| expect($card).to.contain(this.patientName); | |
| }); | |
| } |
| @@ -1370,6 +1370,7 @@ export const PatientHome = (props: any) => { | |||
| ) | |||
| } | |||
| authorizeFor={NonReadOnlyUsers} | |||
| id="sample-request-btn" | |||
There was a problem hiding this comment.
💡 Codebase verification
Both instances of "Request Sample Test" should have test identifiers for consistency
The codebase has two instances of "Request Sample Test":
- One in a
ButtonV2component that correctly has theid="sample-request-btn"attribute - Another in a
<p>tag that lacks a test identifier
For consistent test automation, consider adding a test identifier to the <p> tag instance as well.
🔗 Analysis chain
LGTM! Test identifier added for Cypress automation.
The addition of the id attribute for the sample test request button aligns well with the PR objectives and follows good testing practices.
Let's verify if similar buttons in the desktop view also need test identifiers:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other sample test related buttons in the file
rg --type tsx "Request Sample Test" -B 2 -A 2
Length of output: 75
Script:
#!/bin/bash
# Search for other sample test related buttons in the file, using ripgrep without file type filter
rg "Request Sample Test" -B 2 -A 2
Length of output: 856
Script:
#!/bin/bash
# Let's check if these other instances have test identifiers
rg -B 5 -A 5 "Request Sample Test" src/components/Patient/PatientHome.tsx
Length of output: 909
|
|
||
| // Check for sample request notification and history | ||
| sampleTestPage.clickOnNotification(); | ||
| sampleTestPage.checkRequestHistory(); |
There was a problem hiding this comment.
| sampleTestPage.checkRequestHistory(); | |
| sampleTestPage.checkRequestHistory(); |
- this should verify the content on the card present in the patient details page, where status , sample type and result is displayed.
- we have reusable component for verifying content presence, refer to commands.ts file
| sampleTestPage.checkRequestHistory(); | ||
| }); | ||
|
|
||
| it("should verify sample request on sample page", () => { |
There was a problem hiding this comment.
| it("should verify sample request on sample page", () => { | |
| it("should verify sample request on sample page", () => { |
- this should be part of the first test , not a different test
|
|
||
| it("should verify sample request on sample page", () => { | ||
| sampleTestPage.visitSamplePage(); | ||
| sampleTestPage.searchPatientSample(sampleTestPage.patientName); |
There was a problem hiding this comment.
| sampleTestPage.searchPatientSample(sampleTestPage.patientName); | |
| sampleTestPage.searchPatientSample(sampleTestPage.patientName); |
- check for the patient card presence, and verify the patient name in the card
- then click on the sample details button, and verify all the inputted details are properly present in the sample details page
- add a cy.intercept to ensure that the API status to ensure the page is fully loaded
| cy.awaitUrl("/patients"); | ||
| } | ||
|
|
||
| visitPatientDashboardPage() { |
There was a problem hiding this comment.
| visitPatientDashboardPage() { | |
| visitPatientDashboardPage() { |
- always try to use proper id's and not class name
- remove all unwanted function as a lot of changes requested
| cy.get("#search_patient_name").should("be.visible").type(this.patientName); | ||
| } | ||
|
|
||
| patientSampleMustExist() { |
There was a problem hiding this comment.
| patientSampleMustExist() { | |
| patientSampleMustExist() { |
- never add random card presence check, if you want to check a card of a patient, always check for the card with patient name present in it
| icmrLabel = "Test Icmr Label"; | ||
|
|
||
| before(() => { | ||
| cy.loginByApi("devdistrictadmin", "Coronasafe@123"); |
There was a problem hiding this comment.
| cy.loginByApi("devdistrictadmin", "Coronasafe@123"); | |
| loginPage.loginAsDisctrictAdmin(); |
|
…926/add-sample-test
…are_fe into issues/8926/add-sample-test
|
Hey @nihal467 can you please verify requested changes |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (8)
src/components/Patient/SampleDetails.tsx (2)
359-361: LGTM! Consider extending ID coverageThe
etiology_identifiedID is well-implemented. For completeness, consider adding similar IDs to other test-relevant fields like sample type, SARI status, and travel history sections to maintain consistent testability across the component.
Line range hint
302-361: Well-structured implementation for test automationThe addition of ID attributes follows a consistent pattern and aligns well with the PR's objective of implementing Cypress tests. The identifiers are semantic, well-placed within their respective conditional blocks, and maintain the existing component logic.
A few suggestions for enhancing the test automation setup:
- Consider documenting these test IDs in a central location for better maintainability
- Consider adding a data-testid attribute alongside id for separation of concerns between styling/accessibility and testing
cypress/e2e/sample_test_spec/SampleTestRequest.cy.ts (2)
61-65: Add assertions to verify the request history content.To ensure that the sample test request is correctly reflected in the request history, add explicit assertions to verify the displayed content.
Example additions:
// Check the updated request history sampleTestPage.interceptSampleTestReq(); sampleTestPage.verifySampleTestReq(); sampleTestPage.checkRequestHistory(fastTrackReason); + sampleTestPage.verifyRequestHistoryContent({ + sampleType: sampleTestType, + diagnosis, + fastTrackReason, + });
69-81: Enhance test reliability by adding explicit assertions for sample test details.To improve the reliability of the test, add assertions after navigating to the sample test details page to verify that all entered details are correctly displayed.
Example additions:
sampleTestPage.searchPatientSample(patientName); sampleTestPage.interceptGetSampleTestReq(); sampleTestPage.verifyGetSampleTestReq(); sampleTestPage.verifyPatientName(patientName); sampleTestPage.clickOnSampleDetailsBtn(); sampleTestPage.verifyGetSampleTestReq(); sampleTestPage.verifyPatientTestDetails( patientName, fastTrackReason, diagnosis, differentialDiagnosis, etiologyIdentified, ); + // Assertions to confirm displayed details + cy.get('[data-testid="sample-type"]').should('contain', sampleTestType); + cy.get('[data-testid="diagnosis"]').should('contain', diagnosis); + cy.get('[data-testid="fast-track-reason"]').should('contain', fastTrackReason); + // Add additional assertions as necessarycypress/pageobject/Sample/SampleTestCreate.ts (4)
56-61: Parameterize hardcoded values incheckRequestHistoryfor flexibilityCurrently, the method uses hardcoded strings like "Request Submitted" and "ba/eta". Consider passing these as parameters to make the method more reusable and adaptable to future changes.
Apply this diff to parameterize the values:
checkRequestHistory(fastTrack: string) { + const status = "Request Submitted"; + const testType = "ba/eta"; + const result = "Awaiting"; cy.verifyContentPresence("#sample-test-status", [status]); cy.verifyContentPresence("#sample-test-type", [testType]); cy.verifyContentPresence("#sample-test-fast-track", [fastTrack]); cy.verifyContentPresence("#sample-test-result", [result]); }Or update the method signature to accept these as parameters:
- checkRequestHistory(fastTrack: string) { + checkRequestHistory(fastTrack: string, status: string, testType: string, result: string) { cy.verifyContentPresence("#sample-test-status", [status]); cy.verifyContentPresence("#sample-test-type", [testType]); cy.verifyContentPresence("#sample-test-fast-track", [fastTrack]); cy.verifyContentPresence("#sample-test-result", [result]); }
75-88: Parameterize hardcoded doctor name inverifyPatientTestDetailsThe doctor's name "Dr John Doe" is hardcoded. To enhance flexibility and accommodate different doctors, consider making it a parameter.
Apply this diff:
verifyPatientTestDetails( patientName: string, fastTrackReason: string, diagnosis: string, differentialDiagnosis: string, etiologyIdentified: string, + doctorName: string, ) { cy.verifyContentPresence("#patient_name", [patientName]); cy.verifyContentPresence("#fast_track_reason", [fastTrackReason]); - cy.verifyContentPresence("#doctor_name", ["Dr John Doe"]); + cy.verifyContentPresence("#doctor_name", [doctorName]); cy.verifyContentPresence("#diagnosis", [diagnosis]); cy.verifyContentPresence("#diff_diagnosis", [differentialDiagnosis]); cy.verifyContentPresence("#etiology_identified", [etiologyIdentified]); }Update the calls to this method to pass the
doctorNameparameter accordingly.
95-96: Add timeout tocy.waitforverifyPatientDetailsResponseTo prevent indefinite waiting in case the API response is delayed or fails, consider adding a timeout to
cy.wait.Apply this diff:
verifyPatientDetailsResponse() { - cy.wait("@patientDetails").its("response.statusCode").should("eq", 200); + cy.wait("@patientDetails", { timeout: 10000 }) + .its("response.statusCode") + .should("eq", 200); }Repeat similar adjustments for other methods waiting on intercepts.
108-114: Consolidate API intercepts into a single method for maintainabilityTo reduce code duplication and improve maintainability, consider creating a generic method for intercepting API calls and verifying their responses.
Example implementation:
interceptAndVerifyApi(alias: string, method: string, urlPattern: string, statusCode: number = 200) { cy.intercept(method, urlPattern).as(alias); cy.wait(`@${alias}`, { timeout: 10000 }) .its("response.statusCode") .should("eq", statusCode); }You can then replace the existing methods with calls to this generic method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
cypress/e2e/sample_test_spec/SampleTestRequest.cy.ts(1 hunks)cypress/pageobject/Sample/SampleTestCreate.ts(1 hunks)src/components/Patient/SampleDetails.tsx(2 hunks)src/components/Patient/SampleTest.tsx(5 hunks)src/components/Patient/SampleTestCard.tsx(5 hunks)src/components/Patient/SampleViewAdmin.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Patient/SampleTest.tsx
- src/components/Patient/SampleTestCard.tsx
- src/components/Patient/SampleViewAdmin.tsx
🔇 Additional comments (5)
src/components/Patient/SampleDetails.tsx (5)
302-302: LGTM! Semantic ID for patient name
The addition of the patient_name ID attribute follows good practices for test automation.
327-327: LGTM! Appropriate ID for fast track reason
The fast_track_reason ID is well-placed within the conditional rendering block.
335-337: LGTM! Well-structured doctor name identifier
The doctor_name ID is correctly implemented while preserving the name transformation logic.
343-343: LGTM! Clear diagnosis identifier
The diagnosis ID is appropriately placed for test automation.
351-351: LGTM! Proper differential diagnosis identifier
The diff_diagnosis ID follows the established pattern for test automation.
| "The patient has a high risk of complications and requires immediate testing."; | ||
|
|
||
| before(() => { | ||
| loginPage.loginAsDisctrictAdmin(); |
There was a problem hiding this comment.
Fix the typo in method name loginAsDisctrictAdmin.
The method name loginAsDisctrictAdmin contains a typo. It should be loginAsDistrictAdmin.
Apply this diff to correct the typo:
- loginPage.loginAsDisctrictAdmin();
+ loginPage.loginAsDistrictAdmin();📝 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.
| loginPage.loginAsDisctrictAdmin(); | |
| loginPage.loginAsDistrictAdmin(); |
| cy.awaitUrl("/sample"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add API intercept to ensure the sample page data is loaded before proceeding.
To make sure that the sample page data is fully loaded before performing actions like searching, add a cy.intercept and wait for the response.
Apply this diff to include the intercept:
// Ensure sample page API is loaded before proceeding
+ cy.intercept('GET', '/api/v1/sample/*').as('getSamples');
cy.awaitUrl("/sample");
+ cy.wait('@getSamples');📝 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.
| cy.awaitUrl("/sample"); | |
| // Ensure sample page API is loaded before proceeding | |
| cy.intercept('GET', '/api/v1/sample/*').as('getSamples'); | |
| cy.awaitUrl("/sample"); | |
| cy.wait('@getSamples'); |
| sampleTestPage.verifyPatientDetailsResponse(); | ||
|
|
||
| // Ensure sample request API is loaded | ||
| sampleTestPage.visitSampleRequestPage(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add API intercept to ensure the sample request page has fully loaded.
To ensure that the sample request page is fully loaded before interacting with it, consider adding a cy.intercept for the sample request API and waiting for its response.
Apply this diff to implement the intercept:
// Ensure sample request API is loaded
+ sampleTestPage.interceptSampleRequestAPI();
sampleTestPage.visitSampleRequestPage();
+ sampleTestPage.waitForSampleRequestResponse();If these methods are not available, you can use:
// Ensure sample request API is loaded
+ cy.intercept('GET', '/api/v1/sample/*').as('getSampleRequest');
sampleTestPage.visitSampleRequestPage();
+ cy.wait('@getSampleRequest');Committable suggestion skipped: line range outside the PR's diff.
| selectSampleType(option: string) { | ||
| cy.clickAndSelectOption("#sample-type", option); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for parameters in interaction methods
To enhance robustness, consider adding input validation to ensure that the option parameter is defined before using it in selectSampleType.
Apply this diff to add input validation:
selectSampleType(option: string) {
+ if (!option) {
+ throw new Error('Option is required for selectSampleType');
+ }
cy.clickAndSelectOption("#sample-type", option);
}📝 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.
| selectSampleType(option: string) { | |
| cy.clickAndSelectOption("#sample-type", option); | |
| } | |
| selectSampleType(option: string) { | |
| if (!option) { | |
| throw new Error('Option is required for selectSampleType'); | |
| } | |
| cy.clickAndSelectOption("#sample-type", option); | |
| } |
| fillDoctorName(value: string) { | ||
| cy.get("#doctor_name").should("be.visible").type(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate input in fillDoctorName
To prevent errors, check that value is provided before typing.
Apply this diff:
fillDoctorName(value: string) {
+ if (!value) {
+ throw new Error('Doctor name is required for fillDoctorName');
+ }
cy.get("#doctor_name").should("be.visible").type(value);
}📝 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.
| fillDoctorName(value: string) { | |
| cy.get("#doctor_name").should("be.visible").type(value); | |
| } | |
| fillDoctorName(value: string) { | |
| if (!value) { | |
| throw new Error('Doctor name is required for fillDoctorName'); | |
| } | |
| cy.get("#doctor_name").should("be.visible").type(value); | |
| } |
| fillDiagnosis(value: string) { | ||
| cy.get("#diagnosis").should("be.visible").type(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate value parameter in fillDiagnosis
Add a check to ensure value is provided.
Apply this diff:
fillDiagnosis(value: string) {
+ if (!value) {
+ throw new Error('Diagnosis value is required for fillDiagnosis');
+ }
cy.get("#diagnosis").should("be.visible").type(value);
}📝 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.
| fillDiagnosis(value: string) { | |
| cy.get("#diagnosis").should("be.visible").type(value); | |
| } | |
| fillDiagnosis(value: string) { | |
| if (!value) { | |
| throw new Error('Diagnosis value is required for fillDiagnosis'); | |
| } | |
| cy.get("#diagnosis").should("be.visible").type(value); | |
| } |
| fillIcmrLabel(label: string) { | ||
| cy.get("#icmr-label").should("be.visible").type(label); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate input parameter before using in fillIcmrLabel
Ensure the label parameter is not empty to avoid unexpected behavior.
Apply this diff:
fillIcmrLabel(label: string) {
+ if (!label) {
+ throw new Error('Label is required for fillIcmrLabel');
+ }
cy.get("#icmr-label").should("be.visible").type(label);
}📝 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.
| fillIcmrLabel(label: string) { | |
| cy.get("#icmr-label").should("be.visible").type(label); | |
| } | |
| fillIcmrLabel(label: string) { | |
| if (!label) { | |
| throw new Error('Label is required for fillIcmrLabel'); | |
| } | |
| cy.get("#icmr-label").should("be.visible").type(label); | |
| } |
| fillEtiology(value: string) { | ||
| cy.get("#etiology_identified").should("be.visible").type(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to fillEtiology
Check that value is not empty before typing.
Apply this diff:
fillEtiology(value: string) {
+ if (!value) {
+ throw new Error('Etiology value is required for fillEtiology');
+ }
cy.get("#etiology_identified").should("be.visible").type(value);
}📝 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.
| fillEtiology(value: string) { | |
| cy.get("#etiology_identified").should("be.visible").type(value); | |
| } | |
| fillEtiology(value: string) { | |
| if (!value) { | |
| throw new Error('Etiology value is required for fillEtiology'); | |
| } | |
| cy.get("#etiology_identified").should("be.visible").type(value); | |
| } |
| fillDiffDiagnosis(value: string) { | ||
| cy.get("#diff_diagnosis").should("be.visible").type(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate value parameter in fillDiffDiagnosis
Ensure value is provided to prevent potential errors.
Apply this diff:
fillDiffDiagnosis(value: string) {
+ if (!value) {
+ throw new Error('Differential diagnosis value is required for fillDiffDiagnosis');
+ }
cy.get("#diff_diagnosis").should("be.visible").type(value);
}📝 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.
| fillDiffDiagnosis(value: string) { | |
| cy.get("#diff_diagnosis").should("be.visible").type(value); | |
| } | |
| fillDiffDiagnosis(value: string) { | |
| if (!value) { | |
| throw new Error('Differential diagnosis value is required for fillDiffDiagnosis'); | |
| } | |
| cy.get("#diff_diagnosis").should("be.visible").type(value); | |
| } |
| fillFastTrackReason(value: string) { | ||
| cy.get("#is_fast_track").should("be.visible").check(); | ||
| cy.get("#fast_track").should("be.visible").type(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to fillFastTrackReason
Validate the value parameter to ensure the method functions correctly.
Apply this diff:
fillFastTrackReason(value: string) {
+ if (!value) {
+ throw new Error('Value is required for fillFastTrackReason');
+ }
cy.get("#is_fast_track").should("be.visible").check();
cy.get("#fast_track").should("be.visible").type(value);
}📝 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.
| fillFastTrackReason(value: string) { | |
| cy.get("#is_fast_track").should("be.visible").check(); | |
| cy.get("#fast_track").should("be.visible").type(value); | |
| } | |
| fillFastTrackReason(value: string) { | |
| if (!value) { | |
| throw new Error('Value is required for fillFastTrackReason'); | |
| } | |
| cy.get("#is_fast_track").should("be.visible").check(); | |
| cy.get("#fast_track").should("be.visible").type(value); | |
| } |
nihal467
added
needs testing
needs review
and removed
test failed
changes required
labels
| // Ensure patient list API is loaded before proceeding | ||
| cy.awaitUrl("/patients"); | ||
| patientPage.visitPatient(patientName); | ||
| cy.verifyAndClickElement("#patient-details", "Patient Details"); |
There was a problem hiding this comment.
| cy.verifyAndClickElement("#patient-details", "Patient Details"); | |
| cy.verifyAndClickElement("#patient-details", "Patient Details"); |
- reusable the clickPatientDetails() function in the PatientConsultation.ts
| cy.awaitUrl("/patients"); | ||
| patientPage.visitPatient(patientName); | ||
| cy.verifyAndClickElement("#patient-details", "Patient Details"); | ||
| sampleTestPage.interceptPatientDetailsAPI(); |
There was a problem hiding this comment.
| sampleTestPage.interceptPatientDetailsAPI(); | |
| sampleTestPage.interceptPatientDetailsAPI(); |
- what are you verifying here, in between the intercept trigger and response verification, there is no action been executed
| sampleTestPage.verifyPatientDetailsResponse(); | ||
|
|
||
| // Ensure sample request API is loaded | ||
| sampleTestPage.visitSampleRequestPage(); |
There was a problem hiding this comment.
like the ai review suggested, there is API verification is missing
|
|
||
| // Ensure sample request API is loaded | ||
| sampleTestPage.visitSampleRequestPage(); | ||
|
|
There was a problem hiding this comment.
- As mentioned in the last review to remove unused line, why are you still keeping the space
| } | ||
|
|
||
| checkRequestHistory(fastTrack: string) { | ||
| cy.verifyContentPresence("#sample-test-status", ["Request Submitted"]); |
There was a problem hiding this comment.
| cy.verifyContentPresence("#sample-test-status", ["Request Submitted"]); | |
| cy.verifyContentPresence("#sample-test-status", ["Request Submitted"]); |
- why are keeping these hardcoded value inside the function, keep it in the test file, make it reusable, checkrequesthistory function will not be having same content all the time
| cy.verifyNotification("Sample test created successfully"); | ||
|
|
||
| // Check the updated request history | ||
| sampleTestPage.interceptSampleTestReq(); |
There was a problem hiding this comment.
| sampleTestPage.interceptSampleTestReq(); | |
| sampleTestPage.interceptSampleTestReq(); |
- as mention on above comment, there is no action after the intercept is triggered, make necessary changes, personally understand why intercept are being used before implementing it
| ) { | ||
| cy.verifyContentPresence("#patient_name", [patientName]); | ||
| cy.verifyContentPresence("#fast_track_reason", [fastTrackReason]); | ||
| cy.verifyContentPresence("#doctor_name", ["Dr John Doe"]); |
There was a problem hiding this comment.
| cy.verifyContentPresence("#doctor_name", ["Dr John Doe"]); | |
| cy.verifyContentPresence("#doctor_name", ["Dr John Doe"]); |
- dont use hardcoded values, its not necessary next time someone using this function will have patient card of dr john
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
cypress/e2e/sample_test_spec/SampleTestRequest.cy.ts (1)
36-87: Consider breaking down the test into smaller, focused test cases.The current test case is quite long and tests multiple aspects of the workflow. Consider splitting it into smaller, more focused test cases for better maintainability and easier debugging.
Suggested test structure:
- Test sample request form submission:
it("should submit sample test request form successfully", () => { // Navigate to form // Fill form // Submit and verify });
- Test sample request history:
it("should display sample request in history", () => { // Create sample request in before hook // Verify history });
- Test sample search and details:
it("should display sample details correctly", () => { // Create sample request in before hook // Search and verify details });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cypress/e2e/sample_test_spec/SampleTestRequest.cy.ts(1 hunks)cypress/pageobject/Sample/SampleTestCreate.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/pageobject/Sample/SampleTestCreate.ts
| const patientName = "Dummy Patient 11"; | ||
| const sampleTestType = "BA/ETA"; | ||
| const icmrCategory = "Cat 0"; | ||
| const icmrLabel = "Test Icmr Label"; | ||
| const doctorName = "Dr John Doe"; | ||
| const atypicalDetails = "Patient showing unusual symptoms"; | ||
| const diagnosis = "Suspected respiratory infection"; | ||
| const etiologyIdentified = "Bacterial infection suspected"; | ||
| const differentialDiagnosis = "Possibly a viral infection"; | ||
| const fastTrackReason = | ||
| "The patient has a high risk of complications and requires immediate testing."; | ||
| const sampleTestStatus = "Request Submitted"; | ||
| const expectedSampleTestType = "ba/eta"; | ||
| const sampleTestResult = "Awaiting"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move test data to fixtures for better maintainability.
Consider moving test data constants to a fixture file for better reusability and maintainability.
Create a new file cypress/fixtures/sampleTest.json:
{
"patient": {
"name": "Dummy Patient 11",
"doctorName": "Dr John Doe"
},
"sampleTest": {
"type": "BA/ETA",
"icmrCategory": "Cat 0",
"icmrLabel": "Test Icmr Label",
"status": "Request Submitted",
"result": "Awaiting"
},
"clinicalDetails": {
"atypicalDetails": "Patient showing unusual symptoms",
"diagnosis": "Suspected respiratory infection",
"etiologyIdentified": "Bacterial infection suspected",
"differentialDiagnosis": "Possibly a viral infection",
"fastTrackReason": "The patient has a high risk of complications and requires immediate testing."
}
}Then import and use it in the test:
let testData: any;
before(() => {
cy.fixture('sampleTest').then((data) => {
testData = data;
});
});| sampleTestPage.interceptPatientDetailsAPI(); | ||
| patientConsultationPage.clickPatientDetails(); | ||
| sampleTestPage.verifyPatientDetailsResponse(); |
There was a problem hiding this comment.
Improve API intercept handling for patient details.
The intercept is triggered but there's no explicit wait for the response before proceeding.
Apply this diff:
- sampleTestPage.interceptPatientDetailsAPI();
patientConsultationPage.clickPatientDetails();
- sampleTestPage.verifyPatientDetailsResponse();
+ cy.intercept('GET', '/api/v1/patient/*/details').as('patientDetails');
+ patientConsultationPage.clickPatientDetails();
+ cy.wait('@patientDetails').then((interception) => {
+ expect(interception.response?.statusCode).to.equal(200);
+ });Committable suggestion skipped: line range outside the PR's diff.
| cy.awaitUrl("/sample"); | ||
| sampleTestPage.interceptGetSampleTestReq(); | ||
| sampleTestPage.searchPatientSample(patientName); | ||
| sampleTestPage.verifyGetSampleTestReq(); | ||
| sampleTestPage.verifyPatientName(patientName); | ||
| sampleTestPage.interceptGetSampleTestReq(); | ||
| sampleTestPage.clickOnSampleDetailsBtn(); | ||
| sampleTestPage.verifyGetSampleTestReq(); |
There was a problem hiding this comment.
Improve sample page navigation and verification.
Multiple API intercepts are used without proper waiting, which could cause race conditions.
Apply this diff:
cy.awaitUrl("/sample");
- sampleTestPage.interceptGetSampleTestReq();
+ cy.intercept('GET', '/api/v1/sample_test/*').as('getSampleTest');
sampleTestPage.searchPatientSample(patientName);
- sampleTestPage.verifyGetSampleTestReq();
+ cy.wait('@getSampleTest');
sampleTestPage.verifyPatientName(patientName);
- sampleTestPage.interceptGetSampleTestReq();
+ cy.intercept('GET', '/api/v1/sample_test/*/details').as('getSampleDetails');
sampleTestPage.clickOnSampleDetailsBtn();
- sampleTestPage.verifyGetSampleTestReq();
+ cy.wait('@getSampleDetails');📝 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.
| cy.awaitUrl("/sample"); | |
| sampleTestPage.interceptGetSampleTestReq(); | |
| sampleTestPage.searchPatientSample(patientName); | |
| sampleTestPage.verifyGetSampleTestReq(); | |
| sampleTestPage.verifyPatientName(patientName); | |
| sampleTestPage.interceptGetSampleTestReq(); | |
| sampleTestPage.clickOnSampleDetailsBtn(); | |
| sampleTestPage.verifyGetSampleTestReq(); | |
| cy.awaitUrl("/sample"); | |
| cy.intercept('GET', '/api/v1/sample_test/*').as('getSampleTest'); | |
| sampleTestPage.searchPatientSample(patientName); | |
| cy.wait('@getSampleTest'); | |
| sampleTestPage.verifyPatientName(patientName); | |
| cy.intercept('GET', '/api/v1/sample_test/*/details').as('getSampleDetails'); | |
| sampleTestPage.clickOnSampleDetailsBtn(); | |
| cy.wait('@getSampleDetails'); |
| cy.submitButton("Confirm your request to send sample for testing"); | ||
| sampleTestPage.interceptSampleTestReq(); | ||
| cy.verifyNotification("Sample test created successfully"); | ||
| sampleTestPage.verifySampleTestReq(); |
There was a problem hiding this comment.
Fix sample test request API verification.
The intercept is placed after the action that triggers the API call, which could lead to flaky tests.
Apply this diff:
+ cy.intercept('POST', '/api/v1/sample_test/').as('createSampleTest');
cy.submitButton("Confirm your request to send sample for testing");
- sampleTestPage.interceptSampleTestReq();
cy.verifyNotification("Sample test created successfully");
- sampleTestPage.verifySampleTestReq();
+ cy.wait('@createSampleTest').then((interception) => {
+ expect(interception.response?.statusCode).to.equal(201);
+ });📝 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.
| cy.submitButton("Confirm your request to send sample for testing"); | |
| sampleTestPage.interceptSampleTestReq(); | |
| cy.verifyNotification("Sample test created successfully"); | |
| sampleTestPage.verifySampleTestReq(); | |
| cy.intercept('POST', '/api/v1/sample_test/').as('createSampleTest'); | |
| cy.submitButton("Confirm your request to send sample for testing"); | |
| cy.verifyNotification("Sample test created successfully"); | |
| cy.wait('@createSampleTest').then((interception) => { | |
| expect(interception.response?.statusCode).to.equal(201); | |
| }); |
Proposed Changes
SampleTestPageclass with methods to automate sample request creation and verification.@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
idattributes for various form fields and components across the patient management interface.Bug Fixes
These updates enhance user experience and ensure robust functionality within the sample test management system.