Skip to content

Commit 1b199d1

Browse files
DavidSpickettDavidSpickett
authored
[ci] Handle the case where all reported tests pass but the build is still a failure (#120264)
In this build: https://buildkite.com/llvm-project/github-pull-requests/builds/126961 The builds actually failed, probably because prerequisite of a test suite failed to build. However they still ran other tests and all those passed. This meant that the test reports were green even though the build was red. On some level this is technically correct, but it is very misleading in practice. So I've also passed the build script's return code, as it was when we entered the on exit handler, to the generator, so that when this happens again, the report will draw the viewer's attention to the overall failure. There will be a link in the report to the build's log file, so the next step to investigate is clear. It would be nice to say "tests failed and there was some other build error", but we cannot tell what the non-zero return code was caused by. Could be either. The script handles the following situations now: | Have Result Files? | Tests reported failed? | Return code | Report | |--------------------|------------------------|-------------|-----------------------------------------------------------------------------| | Yes | No | 0 | Success style report. | | Yes | Yes | 0 | Shouldn't happen, but if it did, failure style report showing the failures. | | Yes | No | 1 | Failure style report, showing no failures but noting that the build failed. | | Yes | Yes | 1 | Failure style report, showing the test failures. | | No | ? | 0 | No test report, success shown in the normal build display. | | No | ? | 1 | No test report, failure shown in the normal build display. |
1 parent b270525 commit 1b199d1

File tree

3 files changed

+94
-16
lines changed

3 files changed

+94
-16
lines changed

‎.ci/generate_test_report.py

Lines changed: 88 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,13 @@ def junit_from_xml(xml):
1919

2020
class TestReports(unittest.TestCase):
2121
def test_title_only(self):
22-
self.assertEqual(_generate_report("Foo", []), ("", "success"))
22+
self.assertEqual(_generate_report("Foo", 0, []), ("", "success"))
2323

2424
def test_no_tests_in_testsuite(self):
2525
self.assertEqual(
2626
_generate_report(
2727
"Foo",
28+
1,
2829
[
2930
junit_from_xml(
3031
dedent(
@@ -45,6 +46,7 @@ def test_no_failures(self):
4546
self.assertEqual(
4647
_generate_report(
4748
"Foo",
49+
0,
4850
[
4951
junit_from_xml(
5052
dedent(
@@ -70,10 +72,51 @@ def test_no_failures(self):
7072
),
7173
)
7274

75+
def test_no_failures_build_failed(self):
76+
self.assertEqual(
77+
_generate_report(
78+
"Foo",
79+
1,
80+
[
81+
junit_from_xml(
82+
dedent(
83+
"""\
84+
<?xml version="1.0" encoding="UTF-8"?>
85+
<testsuites time="0.00">
86+
<testsuite name="Passed" tests="1" failures="0" skipped="0" time="0.00">
87+
<testcase classname="Bar/test_1" name="test_1" time="0.00"/>
88+
</testsuite>
89+
</testsuites>"""
90+
)
91+
)
92+
],
93+
buildkite_info={
94+
"BUILDKITE_ORGANIZATION_SLUG": "organization_slug",
95+
"BUILDKITE_PIPELINE_SLUG": "pipeline_slug",
96+
"BUILDKITE_BUILD_NUMBER": "build_number",
97+
"BUILDKITE_JOB_ID": "job_id",
98+
},
99+
),
100+
(
101+
dedent(
102+
"""\
103+
# Foo
104+
105+
* 1 test passed
106+
107+
All tests passed but another part of the build **failed**.
108+
109+
[Download](https://buildkite.com/organizations/organization_slug/pipelines/pipeline_slug/builds/build_number/jobs/job_id/download.txt) the build's log file to see the details."""
110+
),
111+
"error",
112+
),
113+
)
114+
73115
def test_report_single_file_single_testsuite(self):
74116
self.assertEqual(
75117
_generate_report(
76118
"Foo",
119+
1,
77120
[
78121
junit_from_xml(
79122
dedent(
@@ -166,6 +209,7 @@ def test_report_single_file_multiple_testsuites(self):
166209
self.assertEqual(
167210
_generate_report(
168211
"ABC and DEF",
212+
1,
169213
[
170214
junit_from_xml(
171215
dedent(
@@ -198,6 +242,7 @@ def test_report_multiple_files_multiple_testsuites(self):
198242
self.assertEqual(
199243
_generate_report(
200244
"ABC and DEF",
245+
1,
201246
[
202247
junit_from_xml(
203248
dedent(
@@ -238,6 +283,7 @@ def test_report_dont_list_failures(self):
238283
self.assertEqual(
239284
_generate_report(
240285
"Foo",
286+
1,
241287
[
242288
junit_from_xml(
243289
dedent(
@@ -272,6 +318,7 @@ def test_report_dont_list_failures_link_to_log(self):
272318
self.assertEqual(
273319
_generate_report(
274320
"Foo",
321+
1,
275322
[
276323
junit_from_xml(
277324
dedent(
@@ -312,6 +359,7 @@ def test_report_size_limit(self):
312359
self.assertEqual(
313360
_generate_report(
314361
"Foo",
362+
1,
315363
[
316364
junit_from_xml(
317365
dedent(
@@ -351,12 +399,18 @@ def test_report_size_limit(self):
351399
# and output will not be.
352400
def _generate_report(
353401
title,
402+
return_code,
354403
junit_objects,
355404
size_limit=1024 * 1024,
356405
list_failures=True,
357406
buildkite_info=None,
358407
):
359408
if not junit_objects:
409+
# Note that we do not post an empty report, therefore we can ignore a
410+
# non-zero return code in situations like this.
411+
#
412+
# If we were going to post a report, then yes, it would be misleading
413+
# to say we succeeded when the final return code was non-zero.
360414
return ("", "success")
361415

362416
failures = {}
@@ -385,7 +439,11 @@ def _generate_report(
385439
if not tests_run:
386440
return ("", None)
387441

388-
style = "error" if tests_failed else "success"
442+
style = "success"
443+
# Either tests failed, or all tests passed but something failed to build.
444+
if tests_failed or return_code != 0:
445+
style = "error"
446+
389447
report = [f"# {title}", ""]
390448

391449
tests_passed = tests_run - tests_skipped - tests_failed
@@ -400,17 +458,17 @@ def plural(num_tests):
400458
if tests_failed:
401459
report.append(f"* {tests_failed} {plural(tests_failed)} failed")
402460

403-
if not list_failures:
404-
if buildkite_info is not None:
405-
log_url = (
406-
"https://buildkite.com/organizations/{BUILDKITE_ORGANIZATION_SLUG}/"
407-
"pipelines/{BUILDKITE_PIPELINE_SLUG}/builds/{BUILDKITE_BUILD_NUMBER}/"
408-
"jobs/{BUILDKITE_JOB_ID}/download.txt".format(**buildkite_info)
409-
)
410-
download_text = f"[Download]({log_url})"
411-
else:
412-
download_text = "Download"
461+
if buildkite_info is not None:
462+
log_url = (
463+
"https://buildkite.com/organizations/{BUILDKITE_ORGANIZATION_SLUG}/"
464+
"pipelines/{BUILDKITE_PIPELINE_SLUG}/builds/{BUILDKITE_BUILD_NUMBER}/"
465+
"jobs/{BUILDKITE_JOB_ID}/download.txt".format(**buildkite_info)
466+
)
467+
download_text = f"[Download]({log_url})"
468+
else:
469+
download_text = "Download"
413470

471+
if not list_failures:
414472
report.extend(
415473
[
416474
"",
@@ -435,11 +493,23 @@ def plural(num_tests):
435493
"</details>",
436494
]
437495
)
496+
elif return_code != 0:
497+
# No tests failed but the build was in a failed state. Bring this to the user's
498+
# attention.
499+
report.extend(
500+
[
501+
"",
502+
"All tests passed but another part of the build **failed**.",
503+
"",
504+
f"{download_text} the build's log file to see the details.",
505+
]
506+
)
438507

439508
report = "\n".join(report)
440509
if len(report.encode("utf-8")) > size_limit:
441510
return _generate_report(
442511
title,
512+
return_code,
443513
junit_objects,
444514
size_limit,
445515
list_failures=False,
@@ -449,9 +519,10 @@ def plural(num_tests):
449519
return report, style
450520

451521

452-
def generate_report(title, junit_files, buildkite_info):
522+
def generate_report(title, return_code, junit_files, buildkite_info):
453523
return _generate_report(
454524
title,
525+
return_code,
455526
[JUnitXml.fromfile(p) for p in junit_files],
456527
buildkite_info=buildkite_info,
457528
)
@@ -463,6 +534,7 @@ def generate_report(title, junit_files, buildkite_info):
463534
"title", help="Title of the test report, without Markdown formatting."
464535
)
465536
parser.add_argument("context", help="Annotation context to write to.")
537+
parser.add_argument("return_code", help="The build's return code.", type=int)
466538
parser.add_argument("junit_files", help="Paths to JUnit report files.", nargs="*")
467539
args = parser.parse_args()
468540

@@ -477,7 +549,9 @@ def generate_report(title, junit_files, buildkite_info):
477549
if len(buildkite_info) != len(env_var_names):
478550
buildkite_info = None
479551

480-
report, style = generate_report(args.title, args.junit_files, buildkite_info)
552+
report, style = generate_report(
553+
args.title, args.return_code, args.junit_files, buildkite_info
554+
)
481555

482556
if report:
483557
p = subprocess.Popen(

‎.ci/monolithic-linux.sh

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ if [[ -n "${CLEAR_CACHE:-}" ]]; then
2929
fi
3030

3131
function at-exit {
32+
retcode=$?
33+
3234
mkdir -p artifacts
3335
ccache --print-stats > artifacts/ccache_stats.txt
3436

@@ -37,7 +39,7 @@ function at-exit {
3739
if command -v buildkite-agent 2>&1 >/dev/null
3840
then
3941
python3 "${MONOREPO_ROOT}"/.ci/generate_test_report.py ":linux: Linux x64 Test Results" \
40-
"linux-x64-test-results" "${BUILD_DIR}"/test-results.*.xml
42+
"linux-x64-test-results" $retcode "${BUILD_DIR}"/test-results.*.xml
4143
fi
4244
}
4345
trap at-exit EXIT

‎.ci/monolithic-windows.sh

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ fi
2828

2929
sccache --zero-stats
3030
function at-exit {
31+
retcode=$?
32+
3133
mkdir -p artifacts
3234
sccache --show-stats >> artifacts/sccache_stats.txt
3335

@@ -36,7 +38,7 @@ function at-exit {
3638
if command -v buildkite-agent 2>&1 >/dev/null
3739
then
3840
python "${MONOREPO_ROOT}"/.ci/generate_test_report.py ":windows: Windows x64 Test Results" \
39-
"windows-x64-test-results" "${BUILD_DIR}"/test-results.*.xml
41+
"windows-x64-test-results" $retcode "${BUILD_DIR}"/test-results.*.xml
4042
fi
4143
}
4244
trap at-exit EXIT

0 commit comments

Comments
 (0)