[Security Solution] Fix a build step when there are no tests to execute

[maximpn]

Summary

Fixes failed build steps when there are no tests to execute and hence no reports.

Details

Despite this PR fixes tests failure when there are no reports but yarn junit:merge tries to convert them to junit format the problem is partially stays as Failed Test Reporter runs for every build step meaning also for successful build steps. We don't need to handle failed test reports for successful build steps. There are cases when there are no tests to run so Cypress doesn't produce reports and nothing is converted to junit format so Failed Test Reporter fails and causes a build step to fails. It could be solved by defining an environment variable DISABLE_MISSING_TEST_REPORT_ERRORS=true but we need to make sure reports exist for failed tests. Taking this into account we can rely on BUILDKITE_COMMAND_EXIT_STATUS to avoid running Failed Test Reporter if the build step successed.

One may ask why don't skip Upload Artifacts too. We may need some build artifacts for successful build steps.

[delanni]

I'm not sure if this will work on Buildkite, locally I can't use -ne with [ ] so I suggest:

Suggested change

	if [ ${BUILDKITE_COMMAND_EXIT_STATUS} -ne 0 ]; then
	if [[ ${BUILDKITE_COMMAND_EXIT_STATUS} -ne 0 ]]; then

[delanni]

Reading through the post_command, and subsequently the failed_test_reporter.ts I found this -

kibana/packages/kbn-failed-test-reporter-cli/failed_tests_reporter/failed_tests_reporter_cli.ts

Line 94 in afb09cc

if (!reportPaths.length && DISABLE_MISSING_TEST_REPORT_ERRORS) {

Maybe we should set this flag in the post_command hooks: DISABLE_MISSING_TEST_REPORT_ERRORS=true, so this wouldn't cause an error. I think it might be a nicer way out of the errors (instead of skipping in whole), because there might be other things that the reporter does - it seems we're finding (and possibly reporting) some junit .xml files even in case of successful tests, which we would now skip. What do you think?

[maximpn]

@delanni I considered using DISABLE_MISSING_TEST_REPORT_ERRORS=true but it won't help catching situations when reports are broken by some PR. If a PR breaks reports it must break the built so PR's author can take actions.

Regarding skipping Failed Test Reporter for successful tests. As the name suggests Failed Test Reporter handles failed tests. Reports for successful tests are presumably ignored for now. If the script does something useful with successful test reports it must be handled in a different way. At first rename it to something like report_tests and make the intentions transparently clear.

[kibana-ci]

💔 Build Failed

• Buildkite Build
• Commit: 8184c33
• Interpreting CI Failures

Failed CI Steps

• Serverless Observability Examples Tests
• Serverless Security Investigations Cypress Tests #1
• Threat Intelligence Cypress Tests #1
• Threat Intelligence Cypress Tests #1
• Threat Intelligence Cypress Tests #2
• Threat Intelligence Cypress Tests #2
• Rules, Alerts and Exceptions ResponseOps Cypress Tests on Security Solution #2
• Fleet Cypress Tests

Test Failures

• [job] [logs] Serverless Security Investigations Cypress Tests #1 / Bulk Investigate in Timeline Host -> Events Viewer "before all" hook for "Adding multiple events to the timeline should be successful" "before all" hook for "Adding multiple events to the timeline should be successful"
• [job] [logs] Rules, Alerts and Exceptions ResponseOps Cypress Tests on Security Solution #2 / Detections > Callouts indicating read-only access to resources On Rule Details page When a user clicks Dismiss on the callouts We hide them and persist the dismissal We hide them and persist the dismissal
• [job] [logs] Serverless Observability Examples Tests / serverless examples UI Unified Field List Examples Fields existence info "after all" hook in "Fields existence info"
• [job] [logs] Serverless Observability Examples Tests / serverless examples UI Unified Field List Examples Fields existence info "before all" hook in "Fields existence info"
• [job] [logs] Fleet Cypress Tests / When the user has All privileges for Integrations but None for for Fleet Integrations are visible but cannot be added
• [job] [logs] Fleet Cypress Tests / When the user has Editor built-in role Integrations app are visible and can be added

Metrics [docs]

✅ unchanged

History

• 💔 Build #157190 failed 4bbbac4
• 💔 Build #157140 failed 54509e4
• 💔 Build #157125 failed 5663af7988353e19a4bef1e86e5038edb9b83787
• 💔 Build #157107 failed b8e2807c283f65c930d95bbe5c99bc838a96765d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @maximpn

[jbudz]

https://buildkite.com/elastic/kibana-pull-request/builds/157221#018a6fb8-a262-40ac-8d2a-e9e5a8ab06e3/279-290

This seems strange, command is exiting with a failure, report failed tests is correctly skipped but then the CI step is marked as a success. Is this intentional?

[maximpn]

@jbudz this is an intentional behavior and a temporally fix done by this PR. Reporter merger/converter can't handle missing tests gracefully and exit with non zero code. A proper fix will take some time and I created a ticket for that. While you can see non zero exit code in the log in fact it's swallowed by || : resulting in zero exit code.

[banderror]

💚 All backports created successfully

Status	Branch	Result
✅	8.10

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation