-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][Serverless] FTR API Integration tests - Refactoring - Issue fixing #182245
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
0ebee95
to
e9d7a5a
Compare
Pinging @elastic/security-solution (Team: SecuritySolution) |
62c548e
to
d93cdb4
Compare
d93cdb4
to
447b12a
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.
Did cursory review of the code. Diamantis walked me through the relevant changes on Zoom. Action item on our end after this is merged will be to investigate any failing tests owned by our team that have falsely been reporting as passing.
Thank you, Diamantis!
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 behalf of appex-qa I left a question about project deletion
const statusCode = await executeCommand(command, envVars, workDir); | ||
|
||
// Delete serverless project | ||
log.info(`${id} : Deleting project ${PROJECT_NAME}...`); | ||
await cloudHandler.deleteSecurityProject(project.id, PROJECT_NAME); | ||
process.exit(statusCode); |
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.
question: could it happen that the promise is unexpectedly rejected and error will be throw in line 142. In this case project deletion won't be executed.
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.
Actually waitForEsStatusGreen
and other async operations that can throw error on timeout might unexpectedly stop the execution and leave project in cloud. Maybe it makes sense to wrap everything between project ready check and tests run completion in try
block and do cleanup in finally
?
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.
@dmlemeshko Thank you a lot for this. I think that this is a great catch here. I have a handler which automatically deletes all the errant projects after 6 hours in case a project is not deleted properly.
However it is really valuable to be added. I did the change, even though I would refactor it a bit better but for now I feel that we can proceed like this till I structure it more properly. Do you agree?
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.
Totally! Great work.
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.
Rule Management-related changes LGTM, thank you @dkirchan!
If I'm not mistaken, the tests our team owns shouldn't be affected because we haven't yet enabled any of them in the 2nd quality gate.
@@ -223,7 +223,7 @@ | |||
"prebuilt_rules_large_prebuilt_rules_package:server:serverless": "npm run initialize-server:rm prebuilt_rules/large_prebuilt_rules_package serverless", | |||
"prebuilt_rules_large_prebuilt_rules_package:runner:serverless": "npm run run-tests:rm prebuilt_rules/large_prebuilt_rules_package serverless serverlessEnv", | |||
"prebuilt_rules_large_prebuilt_rules_package:qa:serverless": "npm run run-tests:rm prebuilt_rules/large_prebuilt_rules_package serverless qaPeriodicEnv", | |||
"prebuilt_rules_large_prebuilt_rules_package:qa:serverles:release": "npm run run-tests:rm prebuilt_rules/large_prebuilt_rules_package serverlessQA qaEnv", | |||
"prebuilt_rules_large_prebuilt_rules_package:qa:serverless:release": "npm run run-tests:rm prebuilt_rules/large_prebuilt_rules_package serverlessQA qaEnv", |
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.
Thank you!
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.
Thanks @dkirchan!
const PROJECT_NAME_PREFIX = 'kibana-ftr-api-integration-security-solution'; | ||
|
||
// Function to execute a command and return a Promise with the status code | ||
function executeCommand(command: string, envVars: any, workDir: string): Promise<number> { |
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.
👋 Thanks for the PR.
Suggestion: maybe this could be helpful?
const util = require('node:util');
const exec = util.promisify(require('node:child_process').exec);
async function lsExample() {
const { stdout, stderr } = await exec('ls');
console.log('stdout:', stdout);
console.error('stderr:', stderr);
}
lsExample();
This was copied from the node docs (and the imports are probably different in our environment.) But with a helper like this, we might be able to avoid this custome executeCommand wrapper.
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.
Looks great. I ll close this PR in order to unblock things and I will raise a new one with this change!
export const cli = () => { | ||
run( | ||
async (context) => { | ||
const log = new ToolingLog({ |
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.
Suggestion: the api-integration-tests.sh
file has this at the beginning:
#!/bin/bash
if [ -z "$1" ]
then
echo "No target script from the package.json file, is supplied"
exit 1
fi
Perhaps this script could also exit/fail with a message if TARGET_SCRIPT
isn't defined.
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.
This is exactly what $1 is. The Target script.
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @dkirchan |
## Summary [Try catch statement](https://github.com/elastic/kibana/blob/9473241cf8f02eed535c846546114ee94b1ab5fc/.buildkite/scripts/pipelines/security_solution_quality_gate/api_integration/api_ftr_execution.ts#L144) in the PR for the [API Ftr Integration tests](#182245), introduced a cover above the target fix of the specific PR. The tests were failing however the exit code was still 0 so the failures are hidden. This PR addresses the specific issue.
Summary
This PR is addressing the following issues:
.buildkite/pipeline-resource-definitions/security-solution-quality-gate/
folder were skipping intermediate builds. We need to be able to run more than one build in the same time for these pipelines..buildkite/scripts/pipelines/security_solution_quality_gate/api_integration/api-integration-tests.sh
script, it now executes a TS script in order to handle the projects for serverless and execute the yarn script provided.x-pack/plugins/security_solution/scripts/run_cypress/parallel_serverless.ts
is now followed in order to reduce code duplication and maintenance.x-pack/test/security_solution_api_integration/scripts/index.js
. This issue was causing false green test executions in buildkite. The exit code was not actually returned from the child process so the exit code of this script was 0, even though the child process (test execution) was failing giving back an exit code 1..buildkite/pipelines/security_solution/api_integration.yml
to be running the correct test suite (release or periodic) depending on whether the environment variableQUALITY_GATE=1
is passed or not.The last bullet was misleading the test results interpretation, reading as successful test runtime scripts which had one or more test failures. E.g: Buildkite Test Execution being green with failing tests.