-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[Security Solution] Reusing Cypress tests POC2 #162058
Conversation
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MadameSheema I'm definitely in favor of this approach with tagging, compared to the first one with 3 folder hierarchies (#162023 (review)).
Some benefits of this one off the top of my head:
- It allows us to group tests by features, rather than by type of environment. This is very important for engineers who work on the tests because it will increase the focus on one thing and won't force us to jump between very distant folders. For example, for a hypothetical Feature X we could have 7 tests common for ESS and Serverless, 1 test specific to ESS, and 2 tests specific to Serverless. All 10 would live under the same folder, maybe just in 3 separate, but closely located files.
- It would be way easier to reuse common test helper code ("screens", "tasks", etc) between environment-specific tests. The code would be structured in a much more straightforward way.
- We could use tags for other, orthogonal things. For example, specify which tests should be executed against which license in ESS.
- It's overall a much simpler and easier-to-adopt approach. We could reuse many Cypress tests for Serverless in pretty much no time.
On the cons side, the two you mentioned don't seem like a big deal to me personally.
If we forget to add the label the test is not going to be executed.
We could probably write some sort of unit test or a linter rule to check that all tests are tagged. But tbh this might never happen.
If you want to know where a test is going to be executed you need to check the code to see the label.
If we need it that much, we could write a utility that would list all tests for ESS and all tests for Serverless.
As for the UPDATE
section, I'd suggest to do it more explicitly:
- Keep all Cypress tests under
x-pack/plugins/security_solution/cypress/e2e
, including Serverless-specific, ESS-specific, and tests common to both types of environments. - Tag Serverless-specific tests as
@serverless
- Tag ESS-specific tests as
@ess
- Tag tests that should be executed in both environments as
@serverless
AND@ess
The latter would allow us to:
- tag all the existing Cypress tests as
@ess
in one PR - start gradually tagging tests as
@serverless
and increasing test coverage for it
Overall, this is a great POC @MadameSheema and I'm really excited! 🙌
P.S. I left a few comments for your future consideration when you will be working on an implementation PR.
@@ -22,5 +22,6 @@ export default defineCypressConfig({ | |||
e2e: { | |||
experimentalRunAllSpecs: true, | |||
experimentalMemoryManagement: true, | |||
specPattern: ['./e2e/**/*.cy.ts', '../../../../../../security_solution/cypress/e2e/**/*.cy.ts'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
../../../../../../security_solution/cypress/e2e/**/*.cy.ts
🤔
Is it a mistake? What folder does it point to? If it's a legit folder for some reason, could you please add a comment explaining why it's needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it is not. In this POC we are splitting the tests between 2 different folders, the one for just serverless tests (the first path you see on the array) and the folder we usually use for Cypress tests (the second path of the array). As in this POC the execution of the tests for the serverless environment happens from the test_serverless folder, we need to tell cypress were are placed the tests we want to execute.
As seems that we finally we'll have all the tests on the same folder, that is not going to be necessary.
@@ -20,7 +20,7 @@ import { | |||
} from '../../../tasks/alerts'; | |||
import { USER_COLUMN } from '../../../screens/alerts'; | |||
|
|||
describe('user details flyout', () => { | |||
describe('user details flyout', { tags: '@serverless' }, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would make sense to use constants for the tags to avoid typos.
if (Cypress.env('grepTags') !== '@serverless') { | ||
cy.get(LOADING_INDICATOR_HIDDEN).should('exist'); | ||
cy.get(LOADING_INDICATOR).should('not.exist'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cypress.env('grepTags') !== '@serverless'
would be great to extract to a reusable function with a clear name -- not sure I'm able to immediately parse what this means by looking at the code
defaultCommandTimeout: 150000, | ||
env: { | ||
grepFilterSpecs: true, | ||
}, | ||
execTimeout: 150000, | ||
pageLoadTimeout: 150000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the updated timeouts based on something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, I was just playing a bit with the code.
"cypress:open": "../../../../../../node_modules/.bin/cypress open --config-file ./cypress.config.ts --env grepTags=@serverless", | ||
"cypress:run": "../../../../../../node_modules/.bin/cypress run --browser chrome --config-file ./cypress.config.ts --env grepTags=@serverless", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be specified in the config itself, or there's a reason for doing it via the CLI arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the tags can be configured on the cypress config file. The only thing we need to take into consideration is that if we want to follow that direction we would need to create a specific config file per execution type, other than that, not any specific reason for having them on the CLI.
const esArchiver = getService('esArchiver'); | ||
|
||
await esArchiver.load('x-pack/test/security_solution_cypress/es_archives/auditbeat'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for doing that for all tests by default? Just to have some source events for the app so it doesn't show any empty screens?
I think we should be moving away from using generic es archives like this one. If we have tests that specifically depend on this archive I'd try to refactor them before including them in the serverless suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. In order to be able to use our application we need to have data on the instance, if not, a splash screen is going to be displayed telling you to ingest data first. Most of our tests relies on that data, we did it that way to avoid loading it per each test with the aim of improving the execution time of the tests.
Closing in favor of: #162698 |
Summary
With the aim of speed-up the test automation on serverless and reusing the work we have done so far with Cypress, we are doing different POC to check which one fits better our needs (#161537).
NOTE: This PR is just a POC, the code is not going to be merged, the idea behind is to open a discussion to see which solution should be implemented and then create a proper PR for it.
Idea
To use tags to know which tests can be reused. The idea is to add the
@serverless
tag to those scenarios that need to be executed on the serverless environment.Implementation for the POC
We are using the
@cypress/grep
module to have the ability to label Cypress tests and theuser_details.cy.ts
tests for demonstration purposes.We are adding the
@serverless
label to both the serverless anduser_details
tests. On the execution of the tests from the serverless folder, we tell Cypress to execute all the tests that have the@serverless
label from there and also fromx-pack/plugins/security_solution/cypress/e2e
. In that way, when we execute the tests fromx-pack/plugins/security_solution/cypress/e2e
we'll have the ESS execution as usual, when we start the execution fromx-pack/test_serverless/functional/test_suites/security/cypress/e2e
we execute the serverless test living in that folder + the labeled ESS tests that should be executed on serverless as well.As part of this POC we have also:
waitForPageToBeLoaded
method since the locator does not exist on serverless.audibteat
archive when starting the FTR serverless environment and added some missing environment variables to make theuser_details.cy.ts
test work when is executed from serverless.user_details.cy.ts
test work when is executed from serverless.PROS
CONS
Takeaways
If we want to go for a fast solution, this can be our winner. We need to take into consideration though that this way is going to be easier to forget to add a test to the serverless execution.
UPDATE
After continuing investigating about this and syncing with different people my final proposal for serverless is the following one:
@serverless
for specific serverless tests@ess
for specific ess tests