[WIP] - Schedule form cypress refactor

[asirvadAbrahamVarghese]

PR to refactor schedule form cypress test

• Use form element selector commands
• Replace accordionItem with selectAccordionItem command
• Other minor code improvements

@miq-bot assign @GilbertCherrie
@miq-bot add-label cypress
@miq-bot add-label test
@miq-bot add-label refactoring

[jrafanie]

we're red based on the prior change on master... as soon as that's fixed, this should be ready for re-review.

[jrafanie]

Close / open after revert of #9505 via #9553

[jrafanie]

saveButton is really saveButtonText and not the type. It feels like these arguments are backwards.

I would expect the method to be named and called like this:

cy.getFormFooterButtonByTypeWithText('submit', SAVE_BUTTON_TEXT)

If you don't provide a SAVE_BUTTON_TEXT, it just looking for the form button by type.

Perhaps you could have getFormFooterButtonByType and getFormFooterButtonByTypeWithText and have getFormFooterButtonByType call getFormFooterButtonByTypeWithText with no text... and have the latter function default to not checking the button text if not provided. This can be cleaned up later.

Feel free to interpret my ideas in a different way. Ultimately, the args seem backwards so that's the problem I see here.

[asirvadAbrahamVarghese]

It feels like these arguments are backwards.

As it stands, 'type' defaults to 'button', putting it first forces us to provide it in every usage effectively nullifying the benefit of having a default value:
cy. getFormFooterButtonByType('button', 'Cancel');
cy. getFormFooterButtonByType('button', 'Reset');
To actually use the default value, we'd have to call the command in an awkward way like this:
cy. getFormFooterButtonByType(undefined, 'Save');

If the goal is to improve readability, I’d prefer passing an object like the one below, the keys clearly indicate what each value represents:

Cypress.Commands.add(
  'getFormFooterButtonByType',
  ({ buttonType = 'button', buttonName } = {}) =>
    cy.contains(
      `#main-content .bx--btn-set button[type="${buttonType}"]`,
      buttonName
    )
);

or we could go with the more meaningful name you suggested:

Cypress.Commands.add(
  'getFormFooterButtonByTypeWithText',
  ({ buttonType = 'button', buttonName } = {}) =>
    cy.contains(
      `#main-content .bx--btn-set button[type="${buttonType}"]`,
      buttonName
    )
);

Perhaps you could have getFormFooterButtonByType and getFormFooterButtonByTypeWithText and have getFormFooterButtonByType call getFormFooterButtonByTypeWithText with no text... and have the latter function default to not checking the button text if not provided.

We created this button selector based on an earlier discussion to use .contains, since it automatically retries the query. Sorry, if we need to select by button type, we'll have to use cy.get() instead because .contains() command expects:
cy.contains(selector, content)
or
cy.contains(content)

So when no name is passed, this is how the command gets executed:
cy.contains(#main-content .bx--btn-set button[type="${type}"])
which is telling Cypress to look for an element anywhere in the DOM that contains the text #main-content .bx--btn-set button[type="button"] and won’t match anything:

Based on the suggestion, the ideal way to split this into two commands would be as shown below, one using cy.get() when no button text is given:

// First command that uses .contains to match button if button text is provided:
Cypress.Commands.add(
  'getFormFooterButtonByTypeWithText',
  ({ buttonType = 'button', buttonName } = {}) =>
    cy.contains(
      `#main-content .bx--btn-set button[type="${buttonType}"]`,
      buttonName
    )
);

// Second one that calls the above if buttonName is provided:
Cypress.Commands.add(
  'getFormFooterButtonByType',
  ({ buttonType = 'button', buttonName } = {}) => {
    if (buttonName) {
      return cy.getFormFooterButtonByTypeWithText({ buttonType, buttonName });
    }
    return cy.get(`#main-content .bx--btn-set button[type="${buttonType}"]`);
  }
);

If we want to do this, personally, I’d prefer a single implementation that handles both:

Cypress.Commands.add(
  'getFormFooterButton',
  ({ buttonType = 'button', buttonName } = {}) =>
    buttonName
      ? cy.contains(
          `#main-content .bx--btn-set button[type="${buttonType}"]`,
          buttonName
        )
      : cy.get(`#main-content .bx--btn-set button[type="${buttonType}"]`)
);

The issue with the above implementation 🔝 is that when no button text is provided, and there are multiple buttons of the same type in the DOM, Cypress will throw an error when chaining further actions like .click() due to multiple matches. So selecting buttons by type alone can be unreliable:

Ultimately, I believe the best approach is to require button text and throw an error when it’s missing:

Cypress.Commands.add(
  'getFormFooterButtonByTypeWithText',
  ({ buttonType = 'button', buttonName } = {}) => {
    if (buttonName) {
      return cy.contains(
        `#main-content .bx--btn-set button[type="${buttonType}"]`,
        buttonName
      );
    }
    cy.logAndThrowError('buttonName is required');
  }
);

This can be cleaned up later.

It's best to finalize method and argument names early on, before they’re used widely across tests, changing them later would likely require widespread updates across test suites

[jrafanie]

Thanks for the deep dive. I agree with your conclusion after you described it. I do recall having issues where multiple dom elements match so maybe it is best to require the button text and if the type isn't button to provide both the type and the text of the button.

I'm not sure how much we'd have to change to do this.

It's best to finalize method and argument names early on, before they’re used widely across tests, changing them later would likely require widespread updates across test suites

I agree. The difficulty is making sure we've seen enough examples to be able to extract patterns. It's ok to say "we need more information or examples to refactor it". Waiting too long, yes, is also bad. It's a fine line.

[asirvadAbrahamVarghese]

Opened a PR to include this change 🔝 and applied similar changes to other selectors: #9598

[jrafanie]

Do we still need the force: true? We did in the past when the debug UI mode would show debug warnings that covered DOM elements on the screen as the default for cypress is to not interact with covered elements.

[jrafanie]

Oh, I forgot to say, we don't do the debug warnings with CYPRESS Env variable set to true... which is the default in CI.

[asirvadAbrahamVarghese]

Thanks, I wanted to follow up on this but missed it

we don't do the debug warnings with CYPRESS Env variable set to true... which is the default in CI.

yep, I’m using the same in local

Problem:

Even though the checkbox was fully visible, Cypress kept throwing a "covered by another element" error

But was available in the DOM so force:true worked

The problem here was that the input type checkbox was visually hidden with style(and is covered by the label element) and user interacts with label's ::before pseudo-element

Resolution:

so firing the click on the label is what we want here see

[jrafanie]

From a flow perspective, would it make sense to have addSchedule() return the value created, INITIAL_SCHEDULE_NAME, so it's clearer that we're clicking what was just created?

[jrafanie]

in other words:

schedule_name = addSchedule();
...
clickScheduleItem(schedule_name);
...

[asirvadAbrahamVarghese]

We can't directly return values using standard JavaScript return statements because Cypress commands are asynchronous and operate on a command queue
Instead we can return a cy.then() at the end to return the schedule name which will ensure all previous Cypress commands complete before returning, also allows for a clean, chainable usage pattern with .then(), likeaddSchedule().then((createdScheduleName) => {... - see

[jrafanie]

same as above...

[asirvadAbrahamVarghese]

ac04e8b Done in this commit

[asirvadAbrahamVarghese]

alternatively, we can also make use of the onApiResponse callback we have in interceptApi command