Skip to content

Commit 1f82ab3

Browse files
Ben Mehlowmeta-codesync[bot]
authored andcommitted
Fix BRR retry running zero tests when input file has suite-level names
Summary: High-level overview: We have some UDV because TPX does not perform BRR correctly in some cases. This is the first case that I have looked into - when we are trying to BRR a `- main` test. This case can be found on this sandcastle job: https://www.internalfb.com/sandcastle/workflow/4080261262411458331/artifact/actionlog.4080261262631586459.stdout.1?selectedLines=160-161-1-1 More detailed overview: When test listing/run-as-bundle fails in the original run, TPX records a suite-level failure with the synthetic name "suite - main" (via `fb_suite_name()`). During BRR retry, `get_filter()` previously created an exact-match filter for this name. If listing/run-as-bundle succeeds on the base revision, the discovered test cases (e.g., "suite - TestClass::test_method") don't match the exact string "suite - main", so zero tests run and the retry silently produces no results. Fix: when `get_filter()` detects `" - main"` entries (the `SUITE_CASE_NAME` suffix), it converts them to prefix-based regex matchers (`^suite - `) that match any individual test case from that suite. Non-"main" entries remain as exact-match regexes. When no suite-level names are present, behavior is unchanged (uses `RegexFilter::exact`). Running all tests from the suite is correct (ish) here: the `" - main"` entry means listing/run-as-bundle failed, so we don't know which specific tests to retry. The only way to determine pass/fail on the base revision is to run the full suite (perhaps less true for listing than run-as-bundle). This is conceptually similar to `--check-main-test-when-filtering`, which solves the same problem ("main" represents the whole suite) but at the UnitTestFinder allowlist/blocklist layer rather than the BRR regex filter layer. Both mechanisms are independent—the BRR filter operates on the test selection stream after discovery, while the UTF flag affects API request construction and response processing. Note: I am open to alternatives - the rust implementation feels hacky, but I couldn't come up with something better. The bug is real and is driving some UDV and this does seem to fix the issue. Reviewed By: zorroblue, yahayaohinoyi Differential Revision: D93185386 fbshipit-source-id: dc19fc65368f986c4681882bc5f2069008f0fee2
1 parent 5126fee commit 1f82ab3

2 files changed

Lines changed: 303 additions & 0 deletions

File tree

tests/e2e/test/BUCK

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,14 @@ buck2_e2e_test(
9090
"//buck2/tests/e2e_util/api:api",
9191
],
9292
)
93+
94+
buck2_e2e_test(
95+
name = "test_brr_suite_level_retry",
96+
srcs = ["test_brr_suite_level_retry.py"],
97+
skip_for_os = [
98+
"darwin",
99+
"windows",
100+
],
101+
test_with_deployed_buck2 = False,
102+
use_compiled_buck2_client_and_tpx = True,
103+
)
Lines changed: 292 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,292 @@
1+
# Copyright (c) Meta Platforms, Inc. and affiliates.
2+
#
3+
# This source code is dual-licensed under either the MIT license found in the
4+
# LICENSE-MIT file in the root directory of this source tree or the Apache
5+
# License, Version 2.0 found in the LICENSE-APACHE file in the root directory
6+
# of this source tree. You may select, at your option, one of the
7+
# above-listed licenses.
8+
9+
# pyre-strict
10+
11+
import json
12+
import re
13+
from pathlib import Path
14+
15+
from buck2.tests.e2e_util.api.buck import Buck
16+
from buck2.tests.e2e_util.api.buck_result import BuckException
17+
from buck2.tests.e2e_util.buck_workspace import buck_test, get_mode_from_platform
18+
19+
20+
def remove_ansi_escape_sequences(ansi_str: str) -> str:
21+
ansi_escape = re.compile(r"\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])")
22+
return ansi_escape.sub("", ansi_str)
23+
24+
25+
PYTHON_TEST_TARGET: str = "fbcode//buck2/tests/targets/rules/python/test:test"
26+
27+
28+
@buck_test(inplace=True, skip_for_os=["darwin", "windows"])
29+
async def test_brr_suite_level_main_runs_all_tests(buck: Buck, tmp_path: Path) -> None:
30+
"""
31+
When a BRR input file contains a suite-level '- main' entry (indicating
32+
a run_as_bundle test failed in the original run), the retry should run all
33+
tests from that suite rather than filtering them all out.
34+
"""
35+
mode = get_mode_from_platform()
36+
37+
# Step 1: Run the test normally to get a valid run_id from the test infra
38+
first_run = await buck.test(PYTHON_TEST_TARGET, mode)
39+
run_id_match = re.search(r"testinfra/testrun/(\d+)", first_run.stderr)
40+
assert run_id_match, "Could not extract run_id from test output"
41+
run_id = run_id_match.group(1)
42+
43+
# Step 2: Create BRR file with suite-level "- main" entry, simulating a
44+
# run_as_bundle failure in the original run
45+
brr_file = tmp_path / "brr_input.json"
46+
brr_data = {
47+
"suite_name": PYTHON_TEST_TARGET,
48+
"fully_qualified_target_name": PYTHON_TEST_TARGET,
49+
"test_names": [f"{PYTHON_TEST_TARGET} - main"],
50+
"run_id": run_id,
51+
"test_type": 5,
52+
"test_config": {
53+
"config": "",
54+
"host": "linux",
55+
"mode": "@fbcode//mode/dev",
56+
},
57+
}
58+
brr_file.write_text(json.dumps(brr_data))
59+
60+
# Step 3: Run with BRR retry — the fix converts "- main" to a prefix
61+
# matcher so individual test cases from the suite are included.
62+
# BRR mode may exit non-zero (code 32) even when tests pass, so we
63+
# catch BuckException and check stderr from whichever path we get.
64+
try:
65+
result = await buck.test(
66+
PYTHON_TEST_TARGET,
67+
mode,
68+
"--",
69+
"--base-rev-retry-with-input-file",
70+
str(brr_file),
71+
)
72+
stderr = result.stderr
73+
except BuckException as e:
74+
stderr = e.stderr
75+
76+
# Step 4: Assert tests actually ran (before the fix, this was "NO TESTS RAN")
77+
stderr = remove_ansi_escape_sequences(stderr)
78+
assert "NO TESTS RAN" not in stderr, (
79+
"BRR retry with suite-level '- main' entry should run tests, not skip them"
80+
)
81+
assert "Pass" in stderr
82+
83+
84+
BROKEN_RUN_AS_BUNDLE_TARGET: str = (
85+
"fbcode//testinfra/playground/python/broken_run_as_bundle:broken_run_as_bundle_test"
86+
)
87+
88+
89+
@buck_test(inplace=True, skip_for_os=["darwin", "windows"])
90+
async def test_brr_roundtrip_run_as_bundle_failure(buck: Buck, tmp_path: Path) -> None:
91+
"""
92+
End-to-end BRR roundtrip: a run_as_bundle test that fails produces a
93+
report with '- main', and feeding that report back via
94+
--base-rev-retry-with-input-file reproduces the failure (the retry
95+
report also contains '- main').
96+
"""
97+
mode = get_mode_from_platform()
98+
report_file = tmp_path / "report.json"
99+
retry_report_file = tmp_path / "retry_report.json"
100+
101+
# Step 1: Run the broken run_as_bundle target and save the failure report.
102+
# The test will fail (fatal error), so we expect BuckException.
103+
try:
104+
await buck.test(
105+
BROKEN_RUN_AS_BUNDLE_TARGET,
106+
mode,
107+
"--",
108+
"--env",
109+
"TPX_PLAYGROUND_FATAL=1",
110+
"--save-failures-for-retry-in-file",
111+
str(report_file),
112+
)
113+
raise AssertionError("Expected BuckException from broken run_as_bundle target")
114+
except BuckException:
115+
pass
116+
117+
# Step 2: Verify the report was written and contains a "- main" entry.
118+
assert report_file.exists(), "Failure report was not written"
119+
report = json.loads(report_file.read_text())
120+
test_names = report.get("test_names", [])
121+
has_main = any(name.endswith("- main") for name in test_names)
122+
assert has_main, f"Expected a '- main' entry in test_names, got: {test_names}"
123+
124+
# Step 3: Feed the report back as a BRR retry input, saving the retry
125+
# output to a second report file so we can inspect it directly.
126+
try:
127+
await buck.test(
128+
BROKEN_RUN_AS_BUNDLE_TARGET,
129+
mode,
130+
"--",
131+
"--env",
132+
"TPX_PLAYGROUND_FATAL=1",
133+
"--base-rev-retry-with-input-file",
134+
str(report_file),
135+
"--save-failures-for-retry-in-file",
136+
str(retry_report_file),
137+
)
138+
except BuckException:
139+
pass
140+
141+
# Step 4: The retry must reproduce the failure. Verify by
142+
# checking the retry report file rather than parsing stderr.
143+
assert retry_report_file.exists(), "Retry failure report was not written"
144+
retry_report = json.loads(retry_report_file.read_text())
145+
retry_test_names = retry_report.get("test_names", [])
146+
retry_has_main = any(name.endswith("- main") for name in retry_test_names)
147+
assert retry_has_main, (
148+
f"Expected a '- main' entry in retry report test_names, got: {retry_test_names}"
149+
)
150+
print(retry_report)
151+
152+
153+
BROKEN_LISTING_TARGET: str = (
154+
"fbcode//testinfra/playground/python/broken_listing:broken_listing_test"
155+
)
156+
157+
158+
@buck_test(inplace=True, skip_for_os=["darwin", "windows"])
159+
async def test_brr_roundtrip_listing_failure(buck: Buck, tmp_path: Path) -> None:
160+
"""
161+
End-to-end BRR roundtrip: a test whose listing fails (os._exit(1) at
162+
import time with supports_static_listing=False) produces a report with
163+
'- main', and feeding that report back via
164+
--base-rev-retry-with-input-file reproduces the listing failure (the
165+
retry report also contains '- main').
166+
"""
167+
mode = get_mode_from_platform()
168+
report_file = tmp_path / "report.json"
169+
retry_report_file = tmp_path / "retry_report.json"
170+
171+
# Step 1: Run the broken listing target and save the failure report.
172+
# With TPX_PLAYGROUND_FATAL=1, the test crashes at import time which
173+
# kills dynamic listing (supports_static_listing=False).
174+
try:
175+
await buck.test(
176+
BROKEN_LISTING_TARGET,
177+
mode,
178+
"--",
179+
"--env",
180+
"TPX_PLAYGROUND_FATAL=1",
181+
"--save-failures-for-retry-in-file",
182+
str(report_file),
183+
)
184+
raise AssertionError("Expected BuckException from broken listing target")
185+
except BuckException:
186+
pass
187+
188+
# Step 2: Verify the report was written and contains a "- main" entry.
189+
assert report_file.exists(), "Failure report was not written"
190+
report = json.loads(report_file.read_text())
191+
test_names = report.get("test_names", [])
192+
has_main = any(name.endswith("- main") for name in test_names)
193+
assert has_main, f"Expected a '- main' entry in test_names, got: {test_names}"
194+
195+
# Step 3: Feed the report back as a BRR retry input, saving the retry
196+
# output to a second report file so we can inspect it directly.
197+
try:
198+
await buck.test(
199+
BROKEN_LISTING_TARGET,
200+
mode,
201+
"--",
202+
"--env",
203+
"TPX_PLAYGROUND_FATAL=1",
204+
"--base-rev-retry-with-input-file",
205+
str(report_file),
206+
"--save-failures-for-retry-in-file",
207+
str(retry_report_file),
208+
)
209+
except BuckException:
210+
pass
211+
212+
# Step 4: The retry must reproduce the listing failure. Verify by
213+
# checking the retry report file rather than parsing stderr.
214+
assert retry_report_file.exists(), "Retry failure report was not written"
215+
retry_report = json.loads(retry_report_file.read_text())
216+
retry_test_names = retry_report.get("test_names", [])
217+
retry_has_main = any(name.endswith("- main") for name in retry_test_names)
218+
assert retry_has_main, (
219+
f"Expected a '- main' entry in retry report test_names, got: {retry_test_names}"
220+
)
221+
print(retry_report)
222+
223+
224+
@buck_test(inplace=True, skip_for_os=["darwin", "windows"])
225+
async def test_brr_transient_listing_failure_runs_tests(
226+
buck: Buck, tmp_path: Path
227+
) -> None:
228+
"""
229+
Transient listing failure: listing fails in the original run (producing
230+
'- main' in the BRR file), but succeeds on the retry. The fix in
231+
get_filter() converts '- main' to a prefix matcher so individual test
232+
cases from the suite pass through the filter and actually run, producing
233+
a result for the originally-failed test.
234+
235+
Without the fix, RegexFilter::exact(["target - main"]) would not match
236+
any discovered test cases, causing NO TESTS RAN and no result produced
237+
for the failed test.
238+
"""
239+
mode = get_mode_from_platform()
240+
report_file = tmp_path / "report.json"
241+
retry_report_file = tmp_path / "retry_report.json"
242+
243+
# Step 1: Run with TPX_PLAYGROUND_FATAL=1 to make listing crash.
244+
try:
245+
await buck.test(
246+
BROKEN_LISTING_TARGET,
247+
mode,
248+
"--",
249+
"--env",
250+
"TPX_PLAYGROUND_FATAL=1",
251+
"--save-failures-for-retry-in-file",
252+
str(report_file),
253+
)
254+
raise AssertionError("Expected BuckException from broken listing target")
255+
except BuckException:
256+
pass
257+
258+
# Step 2: Verify the report has "- main".
259+
assert report_file.exists(), "Failure report was not written"
260+
report = json.loads(report_file.read_text())
261+
test_names = report.get("test_names", [])
262+
has_main = any(name.endswith("- main") for name in test_names)
263+
assert has_main, f"Expected '- main' in test_names, got: {test_names}"
264+
265+
# Step 3: Feed the report back WITHOUT TPX_PLAYGROUND_FATAL — listing
266+
# succeeds this time. The BRR filter must let tests through so that
267+
# a result is produced for the originally-failed test.
268+
try:
269+
result = await buck.test(
270+
BROKEN_LISTING_TARGET,
271+
mode,
272+
"--",
273+
"--base-rev-retry-with-input-file",
274+
str(report_file),
275+
"--save-failures-for-retry-in-file",
276+
str(retry_report_file),
277+
)
278+
stderr = result.stderr
279+
except BuckException as e:
280+
stderr = e.stderr
281+
282+
# Step 4: The retry must have produced a result — not "NO TESTS RAN".
283+
# What matters is that the suite was not silently dropped by the filter.
284+
# Since listing succeeds on retry, the tests run and pass (they are
285+
# simple stubs), so we verify tests passed rather than checking for a
286+
# failure report — there are no failures to report.
287+
stderr = remove_ansi_escape_sequences(stderr)
288+
assert "NO TESTS RAN" not in stderr, (
289+
"BRR retry with transient listing failure should produce results, "
290+
"not silently skip the suite"
291+
)
292+
assert "Pass" in stderr

0 commit comments

Comments
 (0)