[Security Solution] Refactor Cypress tests navigation utilities #166201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maximpn
added
technical debt
maximpn
force-pushed
the
rectify-cypress-urls
branch
2 times, most recently
from
7fe7738
to
5fd6a47
Compare
maximpn
changed the title
maximpn
force-pushed
the
rectify-cypress-urls
branch
from
5fd6a47
to
c3e7dc4
Compare
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
maximpn
force-pushed
the
rectify-cypress-urls
branch
2 times, most recently
from
91a3a3b
to
6afa689
Compare
maximpn
added
the
release_note:skip
maximpn
force-pushed
the
rectify-cypress-urls
branch
from
4f58527
to
0468999
Compare
semd
approved these changes
Sep 19, 2023
Files by Code Ownerelastic/security-detection-engine
elastic/security-detection-rule-management
elastic/security-threat-hunting-explore
elastic/security-threat-hunting-investigations
|
nikitaindik
approved these changes
Sep 20, 2023
PhilippeOberti
approved these changes
Sep 22, 2023
maximpn
force-pushed
the
rectify-cypress-urls
branch
from
69f07df
to
36a45d0
Compare
yctercero
approved these changes
Sep 22, 2023
maximpn
force-pushed
the
rectify-cypress-urls
branch
from
711fa6f
to
86fd68a
Compare
maximpn
force-pushed
the
rectify-cypress-urls
branch
from
942a764
to
59662ea
Compare
💔 Build FailedFailed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @maximpn |
joemcelroy
pushed a commit
to joemcelroy/kibana
that referenced
this pull request
Sep 25, 2023
…tic#166201) **Relates to:** elastic#153645 ## Summary This PR makes refactoring of navigation utilities and constants to make it transparent and be able to specify correct ownership later on. ## Motivation Engineers need to add new tests and maintain the older ones. Base building blocks of this are constants and utility functions in particular URL constants and `visit()` and `visitWithoutDataRange()` functions. It turned out a simple `visit()` helper function also passes `timerange` as a query param while only some tests need it. Having it as a base utility function (as the name suggests) it also gets used in the new code while `visitWithoutDateRange()` should be preferred. On top of that URL constants are combined in one file without clear ownership and contain a mix of legacy and new urls with some parts looking outdated as navigating to the url causes redirecting to a specific page. Having only relevant URL constants in a common file will help to reduce confusion. As the next step constants should be split into files/folders with clear ownership. Also having `visit()` adding no extra params (besides common for ALL the tests) will make the intention clear. Whenever a time range is needed `visitWithTimeRange()` can be used (ideally accepting a time range defined in a test). And the same stays true for any other pages, e.g. rule details page can have a utility `visitRuleDetailsPage(id: string)` containing some general waiting logic so the following actions operate on a loaded page. ## Details As a step towards clearness and transparent ownership this PR performs refactoring of `x-pack/test/security_solution_cypress/cypress/urls/navigation.ts` and `x-pack/test/security_solution_cypress/cypress/tasks/login.ts` files. The following has been done - all url constants in `x-pack/test/security_solution_cypress/cypress/urls/navigation.ts` were checked and updated to remove duplications, avoid redirections and grouped - legacy urls were moved to the only one test using them to test compatibility (`x-pack/test/security_solution_cypress/cypress/e2e/urls/compatibility.cy.ts`) - `visit()` was renamed to `visitWithTimeRange()` - `visitWithoutDateRange()` was renamed to `visit()` - `visit()` was refactored to accept a query string ## Next steps It's expected teams decompose `x-pack/test/security_solution_cypress/cypress/urls/navigation.ts` into feature specific file(s)/folder(s) with assigned owners. There is no 100% chance a generic wait for a page to be loaded helper function meet requirements for each page. It makes sense to consider adding per feature `visitFeatureAPage()` helper function containing assertions for the page to be loaded. `visitRuleDetailsPage(id: string)` was added for this purpose while waiting for page to be loaded functionality is omitted in this PR to reduce a number of changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relates to: #153645
Summary
This PR makes refactoring of navigation utilities and constants to make it transparent and be able to specify correct ownership later on.
Motivation
Engineers need to add new tests and maintain the older ones. Base building blocks of this are constants and utility functions in particular URL constants and
visit()andvisitWithoutDataRange()functions. It turned out a simplevisit()helper function also passestimerangeas a query param while only some tests need it. Having it as a base utility function (as the name suggests) it also gets used in the new code whilevisitWithoutDateRange()should be preferred. On top of that URL constants are combined in one file without clear ownership and contain a mix of legacy and new urls with some parts looking outdated as navigating to the url causes redirecting to a specific page.Having only relevant URL constants in a common file will help to reduce confusion. As the next step constants should be split into files/folders with clear ownership.
Also having
visit()adding no extra params (besides common for ALL the tests) will make the intention clear. Whenever a time range is neededvisitWithTimeRange()can be used (ideally accepting a time range defined in a test). And the same stays true for any other pages, e.g. rule details page can have a utilityvisitRuleDetailsPage(id: string)containing some general waiting logic so the following actions operate on a loaded page.Details
As a step towards clearness and transparent ownership this PR performs refactoring of
x-pack/test/security_solution_cypress/cypress/urls/navigation.tsandx-pack/test/security_solution_cypress/cypress/tasks/login.tsfiles. The following has been donex-pack/test/security_solution_cypress/cypress/urls/navigation.tswere checked and updated to remove duplications, avoid redirections and groupedx-pack/test/security_solution_cypress/cypress/e2e/urls/compatibility.cy.ts)visit()was renamed tovisitWithTimeRange()visitWithoutDateRange()was renamed tovisit()visit()was refactored to accept a query stringNext steps
It's expected teams decompose
x-pack/test/security_solution_cypress/cypress/urls/navigation.tsinto feature specific file(s)/folder(s) with assigned owners.There is no 100% chance a generic wait for a page to be loaded helper function meet requirements for each page. It makes sense to consider adding per feature
visitFeatureAPage()helper function containing assertions for the page to be loaded.visitRuleDetailsPage(id: string)was added for this purpose while waiting for page to be loaded functionality is omitted in this PR to reduce a number of changes.