-
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
Fixed parallel script for cypress tests in QA and buildkite #169311
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
a59a310
to
3a1327e
Compare
x-pack/plugins/security_solution/scripts/run_cypress/parallel_serverless.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/scripts/run_cypress/parallel_serverless.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/scripts/run_cypress/parallel_serverless.ts
Outdated
Show resolved
Hide resolved
.buildkite/scripts/pipelines/security_solution_quality_gate/pipeline.sh
Outdated
Show resolved
Hide resolved
1431a92
to
9dc4e91
Compare
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.
.buildkite LGTM
244f289
to
a53ab3a
Compare
… the job is finalised
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.
@dkirchan thank you for addressing my comments and making the script better 👍
I tested locally and it works as expected. The only problem it takes a lot of time to run the tests. On top of that I left some extra comments.
Overall the PR looks like almost finalized. I approve it in advance to unblock.
product: response.data.type, | ||
}; | ||
} catch (error) { | ||
log.error(`${error}`); |
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 you log an error.message
instead of implicit error.toString()
? It's not transparent what's error.toString()
outputs.
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.
Fixed with d25ab4e
}); | ||
log.info(`Environment ${projectName} was successfully deleted!`); | ||
} catch (error) { | ||
log.error(`${error}`); |
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 you log an error.message
instead of implicit error.toString()
? It's not transparent what's error.toString()
outputs.
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.
Fixed with d25ab4e
username: response.data.username, | ||
}; | ||
} catch (error) { | ||
throw new Error(`${error}`); |
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 you throw new Error(error.message)
instead of implicit error.toString()
? It's not transparent what's error.toString()
outputs.
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.
Fixed with d25ab4e
throw new Error(`${runnerId} - ${error}`); | ||
}, | ||
retries: 50, | ||
factor: 2, |
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.
It looks like we can have a delay before attempting to fetch the status. As far as I know an MKI env takes around 6 minutes to be up and running. A delay can be a minute or two or the other amount of time we certain about.
Additionally we can "play" with params like factor
, minTimeout
and maxTimeout
to find an optimal approach. Most probably a linear or fixed attempt interval with a delay before will work better.
It doesn't have to be addresses in this PR, just a general though.
@@ -14,18 +14,17 @@ export default defineCypressConfig({ | |||
reporterOptions: { | |||
configFile: './cypress/reporter_config.json', | |||
}, | |||
defaultCommandTimeout: 150000, | |||
defaultCommandTimeout: 300000, |
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.
Why the timeout was increased?
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 can you respond this?
@@ -27,6 +27,7 @@ | |||
"cypress:investigations:run:serverless": "yarn cypress:serverless --spec './cypress/e2e/investigations/**/*.cy.ts'", | |||
"cypress:explore:run:serverless": "yarn cypress:serverless --spec './cypress/e2e/explore/**/*.cy.ts'", | |||
"cypress:changed-specs-only:serverless": "yarn cypress:serverless --changed-specs-only --env burn=5", | |||
"cypress:burn:serverless": "yarn cypress:serverless --env burn=5" | |||
"cypress:burn:serverless": "yarn cypress:serverless --env burn=5", |
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.
Let't set burn
to 2 instead of 5. It should be enough to verify the tests doesn't fail due to artefacts left.
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.
Fixed with d25ab4e
@@ -0,0 +1,11 @@ | |||
steps: |
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.
How often to we plan to run it?
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.
On demand, when triggered by the quality gate, maybe in different PRs..... Not yet strictly defined
agents: | ||
queue: n2-4-spot | ||
timeout_in_minutes: 300 | ||
parallelism: 6 |
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.
Why 6 as parallelism?
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.
I split the test suites to non explore/investigations and these two categories on their own.
Note that once this PR is merged, there is more work we need to do. We want to merge this PR as it is because we are not breaking any existing or new flow and if we continue working on it is going to become a huge/monster PR and we may face the risk of having huge conflicts. On following PRs:
|
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @dkirchan |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
Summary
A new parallel script is introduced, specifically for QA - Serverless environment and Cypress tests of security solution.
To be extended for:
A new target is created in package.json of security_solution_cypress in order to run the tests. With the introduced parallel script, the following steps are handled by the script during the test runtime.
TEST RUNTIME
With this change any new test development can be directly tested by the kibana serverless pipeline providing the branch/fork name and the commit hash in case a fork is under test.
FOR LOCAL RUN
The developer needs to have an API key configured for the QA environment. It can either live in
~/.elastic/cloud.json
file or be provided as an env var :API_KEY=... yarn run cypress:run:qa:serverless:parallel
If the credentials of the required environment are needed to be crosschecked then run the yarn target with the DEBUG env var:
DEBUG=1 yarn run cypress:run:qa:serverless:parallel
As mentioned above, at the time being, the only environment where we run the suites and this script is QA.
Succesful Buildkite run for serverless tests and the specific test functionality
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers