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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Discuss trade offs Cypress's "force: true" provides #152470

Closed
maximpn opened this issue Mar 1, 2023 · 5 comments
Closed

[Security Solution] Discuss trade offs Cypress's "force: true" provides #152470

maximpn opened this issue Mar 1, 2023 · 5 comments
Assignees
@maximpn
Labels
discuss release_note:skip Skip the PR/issue when compiling release notes skip-ci Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.

Comments

@maximpn
Copy link
Contributor

maximpn commented Mar 1, 2023

Relates to: #151244

Description

While looking through our Cypress tests one can see usage of { force: true } option. It's passed to some Cypress actions like click, type and etc. You can find it here or here (just two random places). This approach doesn't look consistent.

Forcing a Cypress action allows to avoid some checks before interacting with an element. Engineers tend to use it to reduce tests sensitivity or to just to make a test working. While forcing an action may look like a good idea it could lead to missing or even intentionally ignoring some UI bugs so the number will accumulate. For example recent EUI upgrade to v75.0.0 revealed UI problems which caused our tests failing. Forced typing would let the tests pass but we wouldn't know about the problem anything.

Taking the above into account to consider the following strategies (feel free to suggest more):

  • we avoid using { force: true } for Cypress actions, every usage require an explanation comment so we tend to remove them later when an UI bug is fixed or to keep them in some exceptional cases
  • we use { force: true } by default everywhere
  • we change nothing so an engineer who is working on a test decides if { force: true } is needed
@maximpn maximpn added discuss release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. skip-ci labels Mar 1, 2023
@maximpn maximpn self-assigned this Mar 1, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@maximpn maximpn changed the title [Security Solution] Discuss tradeoffs Cypress's "force: true" provide [Security Solution] Discuss tradeoffs Cypress's "force: true" provides Mar 1, 2023
@maximpn maximpn changed the title [Security Solution] Discuss tradeoffs Cypress's "force: true" provides [Security Solution] Discuss trade offs Cypress's "force: true" provides Mar 1, 2023
@maximpn
Copy link
Contributor Author

maximpn commented Mar 1, 2023 •
edited
Loading

@MadameSheema it needs your attention.

@MadameSheema
Copy link
Member

Hey @maximpn!! Lots of thanks for starting a discussion on this!! This is my two cents on the subject:

We should avoid using {force: true} by default since as you said, is avoiding some necessary checks to make sure our application is reliable.

If I'm not mistaken, the use of the {force: true} started when an action needed to be performed to elements that are detached from the DOM when performing the action.

I'm in favor of auditing the places where we are using the {force: true} and investigating a way of doing the same action without the flag (I'll create a ticket for that since this is going to be part of helping to remove flakiness of the overall suite).

Until the whole audit is not performed, I'll go for the first option.

Please let me know what you think. Thanks!

@maximpn maximpn mentioned this issue May 26, 2023
Merged
@maximpn
Copy link
Contributor Author

maximpn commented May 27, 2023

There is a small update related to the forced Cypress actions (aka forced: true flag).

Inability to avoid a forced typing in Cypress tests for persistent rules tables state to re-enable it led me to creation of the ticket #152344. The ticket was stuck for some time but the problem has been solved eventually in #158392 and it turned out to be some code added to fix EUI problem which was fixed by the EUI team eventually but the code wasn't removed.

This way it helped to fund a problem and I believe getting rid of forced actions will help us to track any appearing quirks in the UI. I've created a PR to get rid of forced actions in Detection rules Cypress tests.

On the other hand if it's impossible to avoid a forced action a comment should be left.

maximpn added a commit that referenced this issue May 30, 2023
@maximpn
[Security Solution] Remove forced actions from detection rules Cypres…
0d3b63b 
…s tests (#158560)

**Relates to:** #152344, #152470
**Depends on:** #158392

## Summary

Removes `force` option from Cypress actions like `type()`, `click()` and etc. It helps to catch UI quirks instead of silently ignoring them.

## Details

After merging back of #158392 it's possible to remove `force: true` flag from the Cypress actions used in Detection rules tests. Based on the discussion in #152470 it's bette to avoid forced actions and leave a comment if there is no other way than forcing an action.
@maximpn maximpn mentioned this issue May 31, 2023
Merged
maximpn added a commit that referenced this issue May 31, 2023
@maximpn
[Security Solution] Update Cypress readme with recommendations to avo…
891eb27 
…id forced actions (#158705)

**Relates to:** #152470

## Summary

Updates the Security Solution Cypress README with recommendations to avoid forced actions.
@maximpn maximpn mentioned this issue May 31, 2023
Open
@maximpn
Copy link
Contributor Author

maximpn commented May 31, 2023

As the result of discussion the docs were updated by #158705. Closing this ticket.

@maximpn maximpn closed this as completed May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@maximpn maximpn

Labels
discuss release_note:skip Skip the PR/issue when compiling release notes skip-ci Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@maximpn @elasticmachine @MadameSheema