Skip to content
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

feat(protocol-designer): Make createNew.ts more robust and use it to start testing protocol actions in PD #17156

Open
wants to merge 32 commits into
base: edge
Choose a base branch
from

Conversation

alexjoel42
Copy link
Contributor

Overview

These additions to the test case of transferSetting.js.cy are the start of providing greater test coverage to our transfer form.

At this moment, it only goes up to saving a liquid and then opening the transfer form.

Test Plan and Hands on Testing

These are changes exclusively to cypress. So it will be tested when folks run their code.

Changelog

  1. opentrons/protocol-designer/cypress/e2e/transferSettings.cy.js .
  2. opentrons/protocol-designer/cypress/support/commands.ts added some additional helper functions

Please add special attention to checking if this was the correct way to add liquid. Saving it took a bit of extra effort. AFAIK

When you submit a form, the browser sends a POST or GET request to the action URL specified in the

tag or the current page URL if no action is defined.
Cypress executes the test and observes the page reload, which can interfere with your test flow if you're not handling it properly.

So I tried to do the following

cy.contains('button', 'Add liquid').click()
                        cy.contains('button', 'Liquid').click()
                        cy.contains('button', 'Define a liquid').click()
                        cy.get('input[name="name"]') // Select the input with name="name"
                            .type('My liquid!')

                        cy.get('div[aria-label="ModalShell_ModalArea"]')
                            .find('form') // Target the form that wraps the button
                            .invoke('submit', (e) => {
                                e.preventDefault(); // Prevent default behavior
                            });

Review requests

@y3rsh @jerader

Risk assessment

Medium. This might take some extra time with developers to ensure that I'm not breaking existing testing behavior.

@alexjoel42 alexjoel42 requested review from a team as code owners December 19, 2024 20:09
@alexjoel42 alexjoel42 requested review from jerader and removed request for a team December 19, 2024 20:09
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!! a couple of initial pointers:

  1. we have rules for how to name our PRs, i think this one would be a test(protocol-designer): some description https://opentrons.slack.com/archives/CMGV77JP4/p1656442016461519
  2. you can run make check-js and make lint-js quiet=true to run the lint and check tests locally so you can see what is failing without having to push commits
  3. you'll probably want to rebase/merge edge into this branch so you're up to date! i merged the 8.2.2 release branch into edge earlier today

Will continue reviewing but lmk if you need help resolving any of the errors

@alexjoel42
Copy link
Contributor Author

It didn't seem to like opentrons/protocol-designer/cypress/support/commands.ts
Line 204

Cypress.Commands.add('chooseDeckSlot', (slot: string) => {
  // Define the deckSlots object with a Record type for valid keys
  const deckSlots: Record<
    | 'A1'
    | 'A2'
    | 'A3'
    | 'B1'
    | 'B2'
    | 'B3'
    | 'C1'
    | 'C2'
    | 'C3'
    | 'D1'
    | 'D2'
    | 'D3',
    () => void
  > = {
    A1: () => cy.contains('foreignObject[x="164"][y="107"]', 'Edit slot'),
    A2: () => cy.contains('foreignObject[x="164"][y="321"]', 'Edit slot'),
    A3: () => cy.contains('foreignObject[x="328"][y="321"]', 'Edit slot'),
    B1: () => cy.contains('foreignObject[x="0"][y="214"]', 'Edit slot'),
    B2: () => cy.contains('foreignObject[x="164"][y="214"]', 'Edit slot'),
    B3: () => cy.contains('foreignObject[x="328"][y="214"]', 'Edit slot'),
    C1: () => cy.contains('foreignObject[x="0"][y="107"]', 'Edit slot'),
    C2: () => cy.contains('foreignObject[x="164"][y="107"]', 'Edit slot'),
    C3: () => cy.contains('foreignObject[x="328"][y="107"]', 'Edit slot'),
    D1: () => cy.contains('foreignObject[x="0"][y="0"]', 'Edit slot'),
    D2: () => cy.contains('foreignObject[x="0"][y="0"]', 'Edit slot'),
    D3: () => cy.contains('foreignObject[x="328"][y="0"]', 'Edit slot'),
  }

Copy link
Member

@y3rsh y3rsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, please don't be discouraged by all the comments—you're doing an amazing job! 🎉

That said, we need to follow the framework established in protocol-designer/cypress/support/createNew.ts.

Key Points

  1. Commands Structure:

    • The commands file is intended to house only the top-level landing and navigation code.
  2. Starting Point for Tests:

    • createNew is our starting place for all tests that progress into the "create new" flow.
    • If needed, we can rename this for clarity.
  3. Example to Follow:

    • Refer to protocol-designer/cypress/e2e/createNew.cy.ts to see how to structure tests using this pattern:
      1. Create an array of steps.
      2. Run that array of steps with runCreateTest(steps).
  4. Actions and Verifications:

    • All app interactions fall into two categories:
      • Action: Perform an action in the UI.
      • Verification: Validate an expected outcome.
    • The implementation of actions and verifications should be centralized for workflows that align with tests.
    • For now, everything down the "create new" path should reside in createNew.ts.
  5. Readable Test Files:

    • The actual *.cy.ts files should read like a clear list of steps like createNew.cy.ts
  6. Scalability:

    • While createNew.ts may grow too large, we'll break it into smaller pieces when needed. For now, we'll consolidate everything there.

Edit - another bit of words to help cast the vision

*.cy.ts - the test files, define the list of steps we want to accomplish in a test. For tests that go down the create new path, we define the implementation of how cypress goes about acting or verifying in createNew. We define an action or a verification 1 time and then reuse it by including its step name in our list of steps. These are enums and not bare strings so that your IDE will prompt you with existing steps.

protocol-designer/cypress/e2e/createNew.cy.ts Outdated Show resolved Hide resolved
protocol-designer/cypress/e2e/import.cy.ts Outdated Show resolved Hide resolved
protocol-designer/cypress/e2e/import.cy.ts Outdated Show resolved Hide resolved
protocol-designer/cypress/e2e/migrations.cy.ts Outdated Show resolved Hide resolved
@@ -1,7 +1,7 @@
describe('The Settings Page', () => {
before(() => {
cy.visit('/')
cy.closeAnalyticsModal()
cy.contains('button', 'Confirm').click()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the command.

@@ -19,6 +18,7 @@ declare global {
left_pipette_selector: string,
right_pipette_selector: string
) => Cypress.Chainable<void>
chooseDeckSlot: (slot: string) => Cypress.Chainable<void>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove and put in createNew

@@ -101,6 +104,8 @@ Cypress.Commands.add('verifyCreateNewHeader', () => {

// Home Page
Cypress.Commands.add('verifyHomePage', () => {
// Todo re-add when Once 8.2.2 comes back in
cy.contains('button', 'Confirm').click()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you have the new command created, use it here.

Comment on lines +118 to +123
// cy.get('[data-testid="SettingsIconButton"]').click();
cy.getByTestId(locators.settingsDataTestid).click()
// ToDo re-add when 8.2.2 pushed to edge
// cy.get('[data-testid="analyticsToggle"] svg')
// .should('have.css', 'fill', 'rgb(0, 108, 250)')
cy.getByTestId(locators.settingsDataTestid).click()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command should have one job and that is to click the Create New button. Handling other blocking steps should be done separately.

Comment on lines +135 to +145
Cypress.Commands.add('robotSelection', (robotName: string) => {
if (robotName === 'Opentrons OT-2') {
cy.contains('label', locators.OT2_Home).should('be.visible').click()
} else {
// Just checking that the selection modal works
cy.contains('label', locators.OT2_Home).should('be.visible').click()
cy.contains('label', locators.Flex_Home).should('be.visible').click()
}
cy.contains('button', 'Confirm').should('be.visible').click()
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already in createNew

Comment on lines 204 to 244
Cypress.Commands.add('chooseDeckSlot', (slot: string) => {
// Define the deckSlots object with a Record type for valid keys
const deckSlots: Record<
| 'A1'
| 'A2'
| 'A3'
| 'B1'
| 'B2'
| 'B3'
| 'C1'
| 'C2'
| 'C3'
| 'D1'
| 'D2'
| 'D3',
() => Cypress.Chainable<JQuery<HTMLElement>>
> = {
A1: () => cy.contains('foreignObject[x="164"][y="107"]', 'Edit slot'),
A2: () => cy.contains('foreignObject[x="164"][y="321"]', 'Edit slot'),
A3: () => cy.contains('foreignObject[x="328"][y="321"]', 'Edit slot'),
B1: () => cy.contains('foreignObject[x="0"][y="214"]', 'Edit slot'),
B2: () => cy.contains('foreignObject[x="164"][y="214"]', 'Edit slot'),
B3: () => cy.contains('foreignObject[x="328"][y="214"]', 'Edit slot'),
C1: () => cy.contains('foreignObject[x="0"][y="107"]', 'Edit slot'),
C2: () => cy.contains('foreignObject[x="164"][y="107"]', 'Edit slot'),
C3: () => cy.contains('foreignObject[x="328"][y="107"]', 'Edit slot'),
D1: () => cy.contains('foreignObject[x="0"][y="0"]', 'Edit slot'),
D2: () => cy.contains('foreignObject[x="0"][y="0"]', 'Edit slot'),
D3: () => cy.contains('foreignObject[x="328"][y="0"]', 'Edit slot'),
}

// Type-safe slot action assignment
const slotAction = deckSlots[slot as keyof typeof deckSlots]

// Check if slotAction exists and call it, else throw an error
if (slotAction) {
slotAction() // Execute the Cypress command for the selected slot
} else {
throw new Error(`Slot ${slot} not found in deck slots.`)
}
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to createNew

@alexjoel42 alexjoel42 changed the title E2E transfer and command update feat(protocol-designer): revamp PD Cypress tests and add helper functions Dec 20, 2024
@alexjoel42 alexjoel42 changed the title feat(protocol-designer): revamp PD Cypress tests and add helper functions feat(protocol-designer): Make createNew.ts more robust and use it to start testing protocol actions in PD Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants