[WIP] - Added automated tests with cypress for Tenants form

[asirvadAbrahamVarghese]

CP4AIOPS-16943
Pr that adds cypress tests for Tenants form(Settings -> Application Settings ->Access Control -> Tenants)

• Edit parent tenant
• Add project to parent tenant
• Manage quotas with parent tenant
• Add, edit and delete child tenant
• Add project to child tenant
• Manage quotas with child tenant

@miq-bot assign @elsamaryv
@miq-bot add-label cypress
@miq-bot add-label test

[asirvadAbrahamVarghese]

Sporadic server-related failures were noticed here as well:

A couple of API requests are made right after logging in, prior to dashboard rendering:

So I just tried adding a fixed wait using cy.wait immediately after login, before moving to the menu, which seems to help stabilize the test with fewer sporadic failures
CI Runs with wait:
attempt-1

attempt-2

CI run without wait

[asirvadAbrahamVarghese]

Need a second opinion here -

This test currently bundles add project and manage quotas with both parent and child tenants, which makes it quite large. Would it be better to break out add project and manage quotas into standalone tests?

[asirvadAbrahamVarghese]

TODOs:

• Use commands for form elements selection: Add custom commands for selecting form elements #9537 ✅
• Switch to direct API calls for cleanup, if the implementation under [WIP] - Added automated tests with cypress for Namespace form #9517 is approved
• Add more detailed component testing if considered required per [WIP] - Added automated tests with cypress for Namespace form #9517 ✅

[jrafanie]

RE: the sporadic test failure... see #9515 (comment) (if we can bypass saving session in the API and some of the /dashboard GETs, most of the contention causing stale session writes should be removed. 🤞

[asirvadAbrahamVarghese]

Related issue on tenant deletion: #9512

[jrafanie]

This quotasResetOperation function combines the navigate to the form, editing it, clicking a button and validating the flash/result. I think these would read better if the tests read like this:

  cy.getFormFooterButtonByType(resetButton, resetButtonType).should(
    'be.disabled'
  );

  // Editing the quota table
  editQuotasTable(quotaName);

  cy.getFormFooterButtonByType(resetButton, resetButtonType)
    .should('be.enabled')
    .click();
  cy.expect_flash(flashTypeWarning, flashMessageOperationReset);
  cancelOperation();

[asirvadAbrahamVarghese]

Inlined the logic and removed the quotasResetOperation method for improved readability - here & here

[jrafanie]

Same as above, I need to scroll to the function to understand what's being done and then asserted.

I don't mind repeating the same code if it better describes the test:

  editQuotasTable(quotaName);
  // Saving the form
  cy.getFormFooterButtonByType(saveButton, submitButtonType).click();
  cy.expect_flash(flashTypeSuccess, flashMessageSaved);

[asirvadAbrahamVarghese]

Removed the updateQuotas method & inlined the logic for improved readability - here & here

[jrafanie]

not sure if this is worthy of a separate test. It's also called on line 441.

[asirvadAbrahamVarghese]

Here we cancel the form and also assert the flash message, but on 441 the form is cancelled after checking an inline error, and no flash message needs to be asserted so I've handled that with a flag making the method more versatile(see).

[jrafanie]

Thanks for the clarification. Basically, if we have a single step test, we should verify if it's really within a larger workflow. It's ok to unit test in cypress but I think something like cancel would be better suited towards filling out the minimum needed for a form but then hitting cancel. Or mix it with "click add child tenant", "cancel", click it again, fill out the form and save/add.

[asirvadAbrahamVarghese]

Makes sense, mixed it with "add & delete child tenant" test - see

[jrafanie]

Similar to above, this function is doing more than one thing. I think it would read better as a test:

verify reset disabled
edit form
verify reset enabled / any other validations

[asirvadAbrahamVarghese]

Done here & here

[jrafanie]

Similar here... single after call... wondering if we could have a problem if the test were run out of order in a specific order.

[jrafanie]

oh wait, is after an alias for afterEach?

[asirvadAbrahamVarghese]

Since the suite 'Validate Add Project function' has only one test, using after or afterEach effectively results in the same behaviour.

oh wait, is after an alias for afterEach?

not really after and afterEach are different hooks
after: Runs once after all tests in a describe block have completed
afterEach: Runs after each individual test in a describe block

[jrafanie]

Ok, that's what is confusing me. In ruby rspec, there's after/after(:each)/after(:all). after is an alias of after(:each) because it's generally a really bad idea to use after(:all) as that implies there's state contamination that you are using.

Be very careful with before/after (:all).

[jrafanie]

https://docs.cypress.io/app/core-concepts/best-practices#Using-after-Or-afterEach-Hooks

[asirvadAbrahamVarghese]

Switched to afterEach in all cases except this one, which is tied to issue #9512, tagging with TODO for followup.

[jrafanie]

#9523 is merged so #9512 should be fixed.

[asirvadAbrahamVarghese]

Thanks, switched to afterEach

[jrafanie]

we call cancelForm(); in several other places... I'm not sure it's worth testing on it's own here. Maybe it can be tested in the test above: validateFormElements. In other words, when validating the add child tenant form elements, I'm thinking the cancel button is part of a way to test the form.

[asirvadAbrahamVarghese]

Moving that into the test above 'Validate Add child tenant form elements' (see)
I avoided placing that inside validateFormElements because it's used in other tests where cancel validation isn’t needed & adding a flag to control it would make it less clean

[asirvadAbrahamVarghese]

This is now moved along with "Add & delete child tenant" test, as discussed above

[jrafanie]

Do we care about the getFormLabelByInputId? We only check that the element is in the dom and has that id. We don't check the value elsewhere. If we care, we should check the id has a value or remove it entirely. Thoughts?

[asirvadAbrahamVarghese]

Updated to check the label text: see

but I am also thinking to use .contains in getFormLabelByInputId as we did with getFormFooterButtonByType
It might make sense to use .contains when a specific text is passed, and .get otherwise, as the label is already targeted using the linked element’s ID (for attribute), which should uniquely identify it.

Cypress.Commands.add(
  'getFormLabelByForAttributeAndText',
  ({ forValue, labelText }) => {
    if (forValue) {
      return labelText
        ? cy.contains(
            `#main-content .bx--form label[for="${forValue}"]`,
            labelText
          )
        : cy.get(`#main-content .bx--form label[for="${forValue}"]`);
    }
    cy.logAndThrowError('forValue is required');
  }
);

Can be done as part of #9598

[jrafanie]

Same as above

[asirvadAbrahamVarghese]

Updated to check the label text: see