Skip to content

Conversation

asirvadAbrahamVarghese
Copy link
Contributor

PR that refactors Cypress tests for Collect Logs edit form also include review comments changes from #9508

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

@asirvadAbrahamVarghese asirvadAbrahamVarghese requested a review from a team as a code owner July 9, 2025 13:52
@asirvadAbrahamVarghese asirvadAbrahamVarghese force-pushed the edit-collect-logs-form-cypress-refactor branch from df8d9be to b6be458 Compare July 10, 2025 05:38
@asirvadAbrahamVarghese
Copy link
Contributor Author

asirvadAbrahamVarghese commented Jul 10, 2025

This is also prone to sporadic server-related failures, as we’ve seen with the schedules form:
image

@asirvadAbrahamVarghese
Copy link
Contributor Author

asirvadAbrahamVarghese commented Jul 10, 2025

comment - When #9504 gets merged, we can use the helper function here.
comment - Note, checking for "saved" in the message is probably all we need.
comment - Also, we can merge this now and open a new PR to update to the helper function cy.expectFlash('success', 'saved')

comment - Note, the hardcoded 2 in these intercepts here and the tree select feel brittle to me. I wonder if we need to be this precise with the intercepts. Can we use glob pattern matching or are there similar POSTs that occur that we have to avoid intercept/waiting on?

@jrafanie
Copy link
Member

This is also prone to sporadic server-related failures, as we’ve seen with the schedules form:

sorry, I can't read the error in the screenshot. Is that the session issue losing the tree info? undefined method to_sym for nil or undefined method split for nil: #9515?

@asirvadAbrahamVarghese
Copy link
Contributor Author

sorry, I can't read the error in the screenshot. Is that the session issue losing the tree info? undefined method to_sym for nil or undefined method split for nil

Yes a similar one, Data undefined method downcase' for nil [ops/accordion_select] (replaced the screenshot)

@asirvadAbrahamVarghese asirvadAbrahamVarghese force-pushed the edit-collect-logs-form-cypress-refactor branch from b6be458 to a5142fd Compare July 16, 2025 06:08
@miq-bot
Copy link
Member

miq-bot commented Jul 16, 2025

Checked commits asirvadAbrahamVarghese/manageiq-ui-classic@1d5b2a8~...a5142fd with ruby 3.1.7, rubocop 1.56.3, haml-lint 0.64.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@jrafanie
Copy link
Member

sorry, I can't read the error in the screenshot. Is that the session issue losing the tree info? undefined method to_sym for nil or undefined method split for nil

Yes a similar one, Data undefined method downcase' for nil [ops/accordion_select] (replaced the screenshot)

Ok, I still have to get back to that one but I'm pretty sure it's a session race condition where one puma thread overwrites another threads session update.

}

function resetButtonValidation() {
// Confirm Reset button is disabled initially
cy.get('#diagnostics_collect_logs .bx--btn-set button[type="button"]')
.contains('Reset')
.contains(resetButton)
Copy link
Member

Choose a reason for hiding this comment

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

FYI, I just learned that cypress will retry the last query it did until the timeout. If you do cy.contains(selector, text), it will retry the whole thing and not just the contains call. If we rerender things and update the DOM element, we should probably avoid doing cy.get(selector).contains(string) as we may get the element before it's updated in the DOM. Instead, we should use cy.contains(selector+sub element, string)

https://www.youtube.com/watch?v=VSIaDbv4GyE

See also:
https://youtu.be/AhgkBjOF5Ts?t=1530

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking about making this as a new custom command, like I mentioned here

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

Copy link
Contributor Author

@asirvadAbrahamVarghese asirvadAbrahamVarghese Aug 5, 2025

Choose a reason for hiding this comment

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

PR with selector commands for common form elements: #9537

flashMessageOperationCanceled,
} = textConstants;

function interceptAndAwaitApi({
Copy link
Member

Choose a reason for hiding this comment

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

We might want to extract this to a custom command for use in other tests.

@asirvadAbrahamVarghese asirvadAbrahamVarghese force-pushed the edit-collect-logs-form-cypress-refactor branch 4 times, most recently from dca3967 to 0b9cb2e Compare July 29, 2025 13:38
@asirvadAbrahamVarghese
Copy link
Contributor Author

Requires #9531 to be merged for further refactor

@jrafanie
Copy link
Member

jrafanie commented Aug 5, 2025

Requires #9531 to be merged for further refactor

DONE, feel free to continue on with this one.

@jrafanie
Copy link
Member

jrafanie commented Aug 5, 2025

@asirvadAbrahamVarghese can you look at the failing test in the collect logs tests?

@asirvadAbrahamVarghese asirvadAbrahamVarghese force-pushed the edit-collect-logs-form-cypress-refactor branch from 0b9cb2e to a3e98c3 Compare August 5, 2025 17:14
@asirvadAbrahamVarghese
Copy link
Contributor Author

Todo in a follow up PR:

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM. I always notice how things are named. We might want different names for some of these specific functions for collect logs but since I don't have better suggestions, let's just go forward and rename if we find better ideas.

@jrafanie jrafanie merged commit 20738b2 into ManageIQ:master Aug 5, 2025
18 checks passed
This was referenced Aug 7, 2025
jrafanie added a commit that referenced this pull request Aug 19, 2025
…s-form-cypress-refactor

Follow up works from #9516
Fryguy pushed a commit that referenced this pull request Aug 21, 2025
…s-form-cypress-refactor

Follow up works from #9516

(cherry picked from commit 7b13181)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants