-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(protocol-designer): start of cypress happy path test for mix settings #17320
base: edge
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why this file was added? I fully agree with having fixtures, but this one feels like it was automatically generated by Cypress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skowalski08 , this is actually a smidge more severe, my apologies. It appears that you have created a cypress, fixtures, and support file within the Opentrons Directory independent of Protocol-Designer
See here https://github.com/Opentrons/opentrons/tree/a42308ca809c4a7389dbc423fd7bd20bbf5dacc9/cypress
@@ -0,0 +1,37 @@ | |||
/// <reference types="cypress" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go away from the triple slash, which is causing a linting error, but also might delete the rest of the commented out space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon, see earlier comment: https://github.com/Opentrons/opentrons/pull/17320/files#r1926035830
@@ -0,0 +1,262 @@ | |||
import { contains } from 'cypress/types/jquery' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this got auto-added (Vscode sometimes sneakily adds code to try and be helpful if). Can delete this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really thoughtful and thorough test coverage. Once we have alignment on some locators, fix a few bugs, and get Josh's likely much more thorough review it looks good to me!
@@ -0,0 +1,9 @@ | |||
import { defineConfig } from "cypress"; | |||
|
|||
export default defineConfig({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, /opentrons/protocol-designer/cypress.config.js I think is where we defined this. There are two places to go from here:
- Update cypress.config.js --> cypress.config.ts
- Leave it as is for now and delete cypress.config.ts
Actions.SelectTipHandling, | ||
UniversalActions.Snapshot, | ||
Actions.Continue, | ||
Verifications.PartTwoAsp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really impressed by how thorough your verifications were!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skowalski08 , this is actually a smidge more severe, my apologies. It appears that you have created a cypress, fixtures, and support file within the Opentrons Directory independent of Protocol-Designer
See here https://github.com/Opentrons/opentrons/tree/a42308ca809c4a7389dbc423fd7bd20bbf5dacc9/cypress
@@ -0,0 +1,37 @@ | |||
/// <reference types="cypress" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon, see earlier comment: https://github.com/Opentrons/opentrons/pull/17320/files#r1926035830
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,61 @@ | |||
import '../support/commands.ts'; // Importing the custom commands file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See example here without using the .ts extension
import '../support/commands' |
cy.contains(Content.OnceAtStartStep).click() | ||
break | ||
case Actions.AspirateFlowRate: | ||
cy.get(Locators.Aspirate).should('exist').should('be.visible').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should work, but I'd want to check with Devs how stable these locators are. I have a decent amount of them in my code as well so would be good to find out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cy.contains(locators.confirm).click() |
Looks like it's causing trouble with your home.cy.ts file. The cypress test likely will fail until this happens
Overview
Cypress happy path testing for mix settings. Still in progress, have test cases through Mix Settings Part 2 / 2 Aspirate settings.
Test Plan and Hands on Testing
Changelog
Review requests
Risk assessment