From 55baa647c06f92e9251e22e53b5288f101e1c92e Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sat, 17 Oct 2026 18:28:07 +0000 Subject: [PATCH 1/3] fix(ci): Hash-shard full runs that filter tests with -m or -k Per-file sharding plans whole files before pytest applies marker and keyword filters, so a filtered run puts its few matching files on a few shards. Snuba's CI runs sentry's tests with `-m snuba_ci` (via PYTEST_ADDOPTS) over 16 shards: most shards collected 0 tests, exited with code 5 and cancelled the rest. When -m or -k appear in the args or PYTEST_ADDOPTS, every shard collects everything and keeps its per-test hash share, as before per-file sharding. --- .github/workflows/scripts/pytest-selected.py | 21 ++ .../workflows/scripts/test_pytest_selected.py | 52 ++++- tests/sentry/testutils/pytest/test_sentry.py | 199 ++++++++++++++++++ 3 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 tests/sentry/testutils/pytest/test_sentry.py diff --git a/.github/workflows/scripts/pytest-selected.py b/.github/workflows/scripts/pytest-selected.py index 65bd6116fb40..08896c9981e4 100644 --- a/.github/workflows/scripts/pytest-selected.py +++ b/.github/workflows/scripts/pytest-selected.py @@ -22,6 +22,7 @@ import json import math import os +import shlex import sys from dataclasses import dataclass, field from pathlib import Path @@ -161,7 +162,27 @@ def file_weights(files: list[str]) -> dict[str, float]: return weights +def filters_tests(args: list[str]) -> bool: + """Whether `args` or PYTEST_ADDOPTS select tests by marker or keyword. + + The file plan balances every test in each file, so with `-m`/`-k` most + shards can end up with files that have no selected tests at all (e.g. + `-m snuba_ci` in Snuba's CI). + """ + every_arg = [*args, *shlex.split(os.environ.get("PYTEST_ADDOPTS", ""))] + return any(arg.startswith(("-m", "-k")) and not arg.startswith("--") for arg in every_arg) + + def run_full_shard(args: list[str], total: int, group: int) -> int: + if filters_tests(args): + # Every shard collects everything; the sentry plugin then keeps its + # hash share of the items, which spreads the selected tests evenly. + print( + "pytest-selected: -m/-k given, so every shard collects everything " + "and keeps its hash share", + file=sys.stderr, + ) + return int(pytest.main(args)) files = list_test_files(args) plan = plan_full_shards(file_weights(files), total) if files else None if plan is None: diff --git a/.github/workflows/scripts/test_pytest_selected.py b/.github/workflows/scripts/test_pytest_selected.py index 7582c0d0a4b5..c43b74d6de9f 100644 --- a/.github/workflows/scripts/test_pytest_selected.py +++ b/.github/workflows/scripts/test_pytest_selected.py @@ -4,6 +4,7 @@ import importlib import json import os +from pathlib import Path from unittest import mock import pytest @@ -199,7 +200,12 @@ def fake_main(args): calls.append((args, *(os.environ.get(k) for k in env))) return pytest.ExitCode.OK - env = {"SELECTED_TESTS_FILE": "", "TOTAL_TEST_GROUPS": str(total), "TEST_GROUP": str(group)} + env = { + "SELECTED_TESTS_FILE": "", + "TOTAL_TEST_GROUPS": str(total), + "TEST_GROUP": str(group), + "PYTEST_ADDOPTS": "", + } with ( contextlib.chdir(tmp_path), mock.patch.dict(os.environ, env), @@ -249,3 +255,47 @@ def test_new_and_renamed_files_run_once_with_estimated_weight(self, tmp_path): def test_without_durations_every_shard_collects_everything(self, tmp_path): _, calls = run_full_main(tmp_path, ["tests/test_a.py"], None, 3, 1) assert calls == [(["tests", "-q"], "", "3", "1", None)] + + +@pytest.mark.parametrize( + ("args", "addopts", "filtered"), + [ + (["tests", "-q"], "--reruns=5 -n 3 --dist=loadfile", False), + (["tests", "-m", "snuba_ci"], "", True), + (["tests", "-msnuba_ci"], "", True), + (["tests", "-k", "not slow"], "", True), + (["tests"], "-k 'not __In' -m snuba_ci -n 3", True), + (["tests", "--max-worker-restart=1"], "", False), + ], +) +def test_filters_tests(args: list[str], addopts: str, filtered: bool) -> None: + with mock.patch.dict(os.environ, {"PYTEST_ADDOPTS": addopts}): + assert _mod.filters_tests(args) is filtered + + +def test_filtered_full_run_falls_back_to_hash_sharding(tmp_path: Path) -> None: + write_files(tmp_path, ["tests/test_a.py", "tests/test_b.py"]) + durations_file = tmp_path / "durations.json" + durations_file.write_text(json.dumps({"seconds_per_test": 1.0, "files": {}})) + calls: list[tuple[object, ...]] = [] + + def fake_main(args: list[str]) -> pytest.ExitCode: + env = ("SELECTED_TESTS_FILE", "TOTAL_TEST_GROUPS", "TEST_GROUP", "PYTEST_SHARD_PARTS") + calls.append((args, *(os.environ.get(k) for k in env))) + return pytest.ExitCode.OK + + env = { + "SELECTED_TESTS_FILE": "", + "TOTAL_TEST_GROUPS": "16", + "TEST_GROUP": "12", + "PYTEST_ADDOPTS": "-m snuba_ci", + } + with ( + contextlib.chdir(tmp_path), + mock.patch.dict(os.environ, env), + mock.patch.object(_mod, "DURATIONS_FILE", durations_file), + mock.patch.object(pytest, "main", fake_main), + ): + assert main(["tests", "-q"]) == 0 + # The sentry plugin hashes items into 16 groups, as before per-file sharding. + assert calls == [(["tests", "-q"], "", "16", "12", None)] diff --git a/tests/sentry/testutils/pytest/test_sentry.py b/tests/sentry/testutils/pytest/test_sentry.py new file mode 100644 index 000000000000..02a945420878 --- /dev/null +++ b/tests/sentry/testutils/pytest/test_sentry.py @@ -0,0 +1,199 @@ +""" +Full backend runs shard by file: .github/workflows/scripts/pytest-selected.py +plans the shards, the sentry plugin's pytest_ignore_collect and +pytest_collection_modifyitems keep each shard's files, and pytest_shard_plugin +keeps each shard's part of a split file. These tests run that stack for real +in a throwaway project and check every test lands on exactly one shard. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +import textwrap +from collections import Counter +from pathlib import Path + +import pytest + +import sentry +from sentry.utils import json + +SCRIPTS = Path(sentry.__file__).parents[2] / ".github" / "workflows" / "scripts" +SHARDS = 3 + +# Runs pytest-selected.py like `python3 pytest-selected.py ARGS` in CI, with a +# durations file for the throwaway project and every slow-ish file split. +DRIVER = textwrap.dedent( + """ + import importlib, os, sys + from pathlib import Path + sys.path[0] = os.getcwd() + sys.path.append(os.environ["SCRIPTS"]) + selected = importlib.import_module("pytest-selected") + selected.DURATIONS_FILE = Path("durations.json") + selected.PIECES_PER_SHARD = 1 + raise SystemExit(selected.main(sys.argv[1:])) + """ +) + +FILES = { + "conftest.py": ( + "from sentry.testutils.pytest.sentry import ( # noqa: F401\n" + " pytest_collection_modifyitems,\n" + " pytest_ignore_collect,\n" + ")\n" + ), + "pytest.ini": "[pytest]\nmarkers =\n snuba_ci\n", + "tests/shardcheck/test_big.py": textwrap.dedent( + """ + import pytest + + @pytest.mark.parametrize("n", range(40)) + def test_param(n): + pass + + class TestBig: + @pytest.mark.parametrize("n", range(20)) + def test_method(self, n): + pass + """ + ), + "tests/shardcheck/test_inherit.py": textwrap.dedent( + """ + class TestBase: + def test_shared(self): + pass + + class TestChild(TestBase): + def test_own(self): + pass + """ + ), + **{f"tests/shardcheck/test_{name}.py": "def test_a():\n pass\n" for name in "abcdef"}, + "tests/shardcheck/helpers.py": "VALUE = 1\n", + "tests/shardcheck/ignored/test_ignored.py": "def test_ignored():\n pass\n", + "other/test_outside.py": "def test_outside():\n pass\n", + # Like `-m snuba_ci`: a few marked tests in a few of many files. + **{ + f"tests/shardcheck/marked/test_m{i}.py": textwrap.dedent( + """ + import pytest + + @pytest.mark.snuba_ci + @pytest.mark.parametrize("n", range(4)) + def test_marked(n): + pass + + def test_unmarked(): + pass + """ + ) + for i in range(3) + }, + **{f"tests/shardcheck/marked/test_plain{i}.py": "def test_p():\n pass\n" for i in range(12)}, +} + +DURATIONS = { + "seconds_per_test": 0.1, + "files": {"tests/shardcheck/test_big.py": 30.0, "tests/shardcheck/test_inherit.py": 2.0}, +} + + +@pytest.fixture +def project(tmp_path: Path) -> Path: + for path, content in FILES.items(): + (tmp_path / path).parent.mkdir(parents=True, exist_ok=True) + (tmp_path / path).write_text(content) + (tmp_path / "durations.json").write_text(json.dumps(DURATIONS)) + return tmp_path + + +def _run( + project: Path, command: list[str], env: dict[str, str] +) -> subprocess.CompletedProcess[str]: + # The outer run's own sharding, plugin and Django settings must not leak in. + base = { + k: v + for k, v in os.environ.items() + if not k.startswith(("PYTEST_", "DJANGO_", "SENTRY_", "TEST_GROUP", "TOTAL_TEST_GROUPS")) + and k not in ("SELECTED_TESTS_FILE", "PYTHONPATH") + } + base |= {"PYTEST_DISABLE_PLUGIN_AUTOLOAD": "1", "SCRIPTS": str(SCRIPTS)} + proc = subprocess.run( + command, + cwd=project, + env=base | env, + capture_output=True, + text=True, + timeout=300, + ) + assert proc.returncode == 0, proc.stdout + proc.stderr + return proc + + +def _nodeids(proc: subprocess.CompletedProcess[str], collect_only: bool) -> Counter[str]: + lines = proc.stdout.splitlines() + if collect_only: + return Counter(line for line in lines if "::" in line) + return Counter(line.split(" ", 1)[1] for line in lines if line.startswith("PASSED ")) + + +def _check_sharding( + project: Path, + args: list[str], + *, + collect_only: bool = True, + env: dict[str, str] | None = None, +) -> list[str]: + mode = ["--collect-only", "-q"] if collect_only else ["-rA"] + # CI sets PY_COLORS=1, which would prefix the PASSED lines with escapes. + common = ["-p", "no:cacheprovider", "--color=no", *args, *mode] + env = env or {} + unsharded = _nodeids( + _run(project, [sys.executable, "-m", "pytest", *common], env), collect_only + ) + assert unsharded + + sharded: Counter[str] = Counter() + logs = [] + for group in range(SHARDS): + proc = _run( + project, + [sys.executable, "-c", DRIVER, *common], + env + | { + "TOTAL_TEST_GROUPS": str(SHARDS), + "TEST_GROUP": str(group), + "TEST_GROUP_STRATEGY": "roundrobin", + }, + ) + shard = _nodeids(proc, collect_only) + assert shard, f"shard {group} ran nothing" + sharded += shard + logs.append(proc.stderr) + + assert sharded == unsharded + return logs + + +IGNORE = ["--ignore", "tests/shardcheck/ignored"] + + +@pytest.mark.parametrize( + ("args", "env"), + [ + (["tests", "-m", "snuba_ci"], {}), + (["tests", "-k", "test_marked"], {}), + # How Snuba's CI passes it. + (["tests"], {"PYTEST_ADDOPTS": "-k 'not __In' -m snuba_ci"}), + ], +) +def test_filtered_run_spreads_selected_tests_over_every_shard( + project: Path, args: list[str], env: dict[str, str] +) -> None: + # Planning by file would put the 3 marked files on at most 3 of the + # shards and leave the rest with nothing selected. + logs = _check_sharding(project, [*args, *IGNORE], collect_only=False, env=env) + assert all("keeps its hash share" in log for log in logs) From 7552cef2c0602bd4f1b04464d488a4ff26822c1e Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sat, 17 Oct 2026 18:28:38 +0000 Subject: [PATCH 2/3] fix(tests): Address review feedback on test infra changes - Keep pytest 9 subtest reports out of the JSON report's reruns, and show failed subtests in backend failure comments. - Lock tables while the transactional flush takes its inventory and deletes; fall back to Django's flush when a writer holds them. - Bound Relay readiness probes by the time left and remove the container when readiness fails. - Give each span buffer permutation a fresh SpansBuffer. - Fall back to hash sharding for paths per-file sharding cannot match, and validate merged duration shards. Co-Authored-By: Joshua Li <14209781+joshuarli@users.noreply.github.com> --- .../scripts/backend-test-durations.py | 61 +++- .github/workflows/scripts/pytest-selected.py | 44 ++- .../workflows/scripts/pytest_shard_plugin.py | 12 + .../scripts/report-backend-test-failures.js | 61 +++- .../report-backend-test-failures.test.js | 77 ++++ .../scripts/test_backend_test_durations.py | 73 +++- .../workflows/scripts/test_pytest_selected.py | 125 +++++-- .../testutils/pytest/json_report_reruns.py | 81 ++++- src/sentry/testutils/pytest/relay.py | 69 +++- .../testutils/pytest/transactional_flush.py | 240 +++++++++---- tests/sentry/spans/test_buffer.py | 19 +- .../pytest/test_json_report_reruns.py | 145 ++++++++ tests/sentry/testutils/pytest/test_relay.py | 106 ++++++ tests/sentry/testutils/pytest/test_sentry.py | 27 ++ .../pytest/test_transactional_flush.py | 338 ++++++++++++++++++ 15 files changed, 1324 insertions(+), 154 deletions(-) create mode 100644 tests/sentry/testutils/pytest/test_json_report_reruns.py create mode 100644 tests/sentry/testutils/pytest/test_relay.py create mode 100644 tests/sentry/testutils/pytest/test_transactional_flush.py diff --git a/.github/workflows/scripts/backend-test-durations.py b/.github/workflows/scripts/backend-test-durations.py index c1f632507de6..535867183df0 100644 --- a/.github/workflows/scripts/backend-test-durations.py +++ b/.github/workflows/scripts/backend-test-durations.py @@ -17,12 +17,26 @@ import argparse import json +import math +import os from collections import defaultdict +from collections.abc import Mapping from pathlib import Path +from typing import Any +SCHEMA = 1 +# Every shard of one attempt of one run must agree on these. +RUN_FIELDS = ("schema", "unit", "commit", "run_id", "run_attempt", "shards") -def extract(pytest_json: Path) -> dict[str, object]: - """Sum setup, call and teardown time per file from a pytest-json-report file.""" + +def extract(pytest_json: Path, env: Mapping[str, str] | None = None) -> dict[str, Any]: + """Sum setup, call and teardown time per file from a pytest-json-report file. + + Subtests (`subtests` in the report) ran inside the parent's call, so the + call duration already includes them. Reruns are left out. + """ + if env is None: + env = os.environ files: dict[str, float] = defaultdict(float) tests = 0 for test in json.loads(pytest_json.read_text()).get("tests", []): @@ -30,18 +44,53 @@ def extract(pytest_json: Path) -> dict[str, object]: path = test["nodeid"].split("::", 1)[0] for stage in ("setup", "call", "teardown"): files[path] += test.get(stage, {}).get("duration", 0.0) - return {"files": dict(files), "tests": tests} + return { + "schema": SCHEMA, + "unit": "seconds", + "commit": env.get("GITHUB_SHA"), + "run_id": env.get("GITHUB_RUN_ID"), + "run_attempt": env.get("GITHUB_RUN_ATTEMPT"), + "shard": int(env.get("TEST_GROUP", 0)), + "shards": int(env.get("TOTAL_TEST_GROUPS", 1)), + "files": dict(files), + "tests": tests, + } + + +def validate(shards: list[dict[str, Any]]) -> None: + """Reject inputs that would skew the weights: mixed runs, gaps, bad numbers.""" + if not shards: + raise ValueError("no shard durations given") + for shard in shards: + if shard.get("schema") != SCHEMA or shard.get("unit") != "seconds": + raise ValueError(f"unsupported durations format: {shard.get('schema')!r}") + runs = {tuple(shard.get(field) for field in RUN_FIELDS) for shard in shards} + if len(runs) > 1: + raise ValueError(f"durations come from more than one run: {sorted(map(str, runs))}") + (run,) = runs + if None in run: + raise ValueError(f"durations are missing run details: {dict(zip(RUN_FIELDS, run))}") + indexes = sorted(shard["shard"] for shard in shards) + if indexes != list(range(shards[0]["shards"])): + raise ValueError(f"expected shards 0..{shards[0]['shards'] - 1} once each, got {indexes}") + for shard in shards: + values = [*shard["files"].values(), shard["tests"]] + if not all(isinstance(v, (int, float)) and math.isfinite(v) and v >= 0 for v in values): + raise ValueError(f"shard {shard['shard']} has a negative or non-finite duration") -def merge(shards: list[dict[str, object]]) -> dict[str, object]: +def merge(shards: list[dict[str, Any]]) -> dict[str, Any]: + validate(shards) files: dict[str, float] = defaultdict(float) tests = 0 for shard in shards: - tests += shard["tests"] # type: ignore[operator] - for path, seconds in shard["files"].items(): # type: ignore[attr-defined] + tests += shard["tests"] + for path, seconds in shard["files"].items(): files[path] += seconds total = sum(files.values()) + first = shards[0] return { + "source": {field: first[field] for field in ("commit", "run_id", "run_attempt", "shards")}, # For files added since: their AST test count times this. "seconds_per_test": round(total / tests, 3) if tests else 0.0, "files": {path: round(files[path], 1) for path in sorted(files)}, diff --git a/.github/workflows/scripts/pytest-selected.py b/.github/workflows/scripts/pytest-selected.py index 08896c9981e4..4483461af775 100644 --- a/.github/workflows/scripts/pytest-selected.py +++ b/.github/workflows/scripts/pytest-selected.py @@ -113,12 +113,8 @@ def plan_full_shards(weights: dict[str, float], total: int) -> list[Shard] | Non ] -def list_test_files(args: list[str]) -> list[str]: - """Every .py file pytest could collect from the paths in `args`, minus --ignore. - - Non-test files are harmless: pytest doesn't collect them either way. Missing - one that pytest does collect would drop its tests, so this errs wide. - """ +def _parse_paths(args: list[str]) -> tuple[list[str], set[str]]: + """The existing paths in `args` and the --ignore'd ones, normalized.""" roots, ignored = [], set() it = iter(args) for arg in it: @@ -128,7 +124,16 @@ def list_test_files(args: list[str]) -> list[str]: ignored.add(os.path.normpath(arg.split("=", 1)[1])) elif not arg.startswith("-") and os.path.exists(arg): roots.append(os.path.normpath(arg)) + return roots, ignored + +def list_test_files(args: list[str]) -> list[str]: + """Every .py file pytest could collect from the paths in `args`, minus --ignore. + + Non-test files are harmless: pytest doesn't collect them either way. Missing + one that pytest does collect would drop its tests, so this errs wide. + """ + roots, ignored = _parse_paths(args) files: set[str] = set() for root in roots: if os.path.isfile(root): @@ -145,6 +150,26 @@ def list_test_files(args: list[str]) -> list[str]: return sorted(files) +def per_file_sharding_supported(args: list[str]) -> bool: + """Whether the file paths written for the sentry plugin will match its nodeids. + + The plugin compares them with paths relative to pytest's rootdir, which + pytest_shard_plugin checks is the working directory. Anything else falls + back to per-test hash sharding. + """ + if any( + arg in ("-c", "--config-file", "--rootdir") + or arg.startswith(("--config-file=", "--rootdir=")) + for arg in [*args, *shlex.split(os.environ.get("PYTEST_ADDOPTS", ""))] + ): + return False + roots, _ = _parse_paths(args) + return bool(roots) and all( + not os.path.isabs(root) and root != os.pardir and not root.startswith(os.pardir + os.sep) + for root in roots + ) + + def file_weights(files: list[str]) -> dict[str, float]: """Seconds per file from the last full run, estimated from test counts for new files.""" durations = json.loads(DURATIONS_FILE.read_text()) @@ -183,6 +208,13 @@ def run_full_shard(args: list[str], total: int, group: int) -> int: file=sys.stderr, ) return int(pytest.main(args)) + if not per_file_sharding_supported(args): + print( + "pytest-selected: paths are not relative to the working directory, " + "so every shard collects everything and keeps its hash share", + file=sys.stderr, + ) + return int(pytest.main(args)) files = list_test_files(args) plan = plan_full_shards(file_weights(files), total) if files else None if plan is None: diff --git a/.github/workflows/scripts/pytest_shard_plugin.py b/.github/workflows/scripts/pytest_shard_plugin.py index 959ddddceb52..b42b24418c25 100644 --- a/.github/workflows/scripts/pytest_shard_plugin.py +++ b/.github/workflows/scripts/pytest_shard_plugin.py @@ -11,6 +11,7 @@ import json import os from hashlib import sha256 +from pathlib import Path import pytest @@ -19,6 +20,17 @@ def item_part(nodeid: str, parts: int) -> int: return int(sha256(nodeid.encode()).hexdigest(), 16) % parts +def pytest_configure(config: pytest.Config) -> None: + # The shard's file list is relative to the working directory, and the + # sentry plugin matches it against paths relative to the rootdir. If they + # differ, every file would be skipped (or kept) on every shard. + if os.environ.get("PYTEST_SHARD_PARTS") and config.rootpath != Path.cwd().resolve(): + raise pytest.UsageError( + f"pytest_shard_plugin: rootdir {config.rootpath} is not the working " + f"directory {Path.cwd()}; run pytest-selected.py from the rootdir" + ) + + def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item]) -> None: parts_file = os.environ.get("PYTEST_SHARD_PARTS") if not parts_file: diff --git a/.github/workflows/scripts/report-backend-test-failures.js b/.github/workflows/scripts/report-backend-test-failures.js index 87352f95ad06..0d31358591f7 100644 --- a/.github/workflows/scripts/report-backend-test-failures.js +++ b/.github/workflows/scripts/report-backend-test-failures.js @@ -21,6 +21,7 @@ import {basename, dirname} from 'node:path'; const MAX_FAILURES = 30; const MAX_TRACEBACK_LINES = 50; +const MAX_SUBTESTS = 5; const MAX_BODY_LENGTH = 65_000; export const COMMENT_MARKER = ''; @@ -43,11 +44,28 @@ export function parseFailures(files, core) { .map(t => ({ nodeid: t.nodeid ?? 'unknown', longrepr: (t.call ?? t.setup ?? t.teardown ?? {}).longrepr ?? '', + // A parent whose subtests failed only says "contains N failed + // subtests"; the assertions live in each subtest's own longrepr. + subtests: (t.subtests ?? []) + .filter(s => s.outcome === 'failed') + .map(s => ({ + description: describeSubtest(s.context), + longrepr: s.longrepr ?? '', + })), artifactDir, })); }); } +// Same shape pytest prints after SUBFAILED: "[msg] (k=v, ...)". +export function describeSubtest(context) { + const parts = []; + if (context?.msg !== null && context?.msg !== undefined) parts.push(`[${context.msg}]`); + const kwargs = Object.entries(context?.kwargs ?? {}); + if (kwargs.length) parts.push(`(${kwargs.map(([k, v]) => `${k}=${v}`).join(', ')})`); + return parts.join(' ') || '()'; +} + // Maps artifact directory names to GitHub Actions job log URLs. const JOB_MATCHERS = [ { @@ -107,24 +125,39 @@ function truncateBody(body) { ); } +function truncateTraceback(tb) { + if (!tb) return tb; + const lines = tb.split('\n'); + if (lines.length <= MAX_TRACEBACK_LINES) return tb; + return ( + lines.slice(0, MAX_TRACEBACK_LINES).join('\n') + + `\n... (${lines.length - MAX_TRACEBACK_LINES} more lines)` + ); +} + +const codeBlock = text => `\`\`\`\n${text}\n\`\`\`\n\n`; + // Renders
blocks for each failure (no header). export function buildFailureBlocks(failures) { return failures - .map(({nodeid, longrepr, jobUrl}) => { - let tb = longrepr; - if (tb) { - const lines = tb.split('\n'); - if (lines.length > MAX_TRACEBACK_LINES) { - tb = - lines.slice(0, MAX_TRACEBACK_LINES).join('\n') + - `\n... (${lines.length - MAX_TRACEBACK_LINES} more lines)`; - } - } + .map(({nodeid, longrepr, subtests = [], jobUrl}) => { const logLink = jobUrl ? ` — log` : ''; - return ( - `
${nodeid}${logLink}\n\n` + - `\`\`\`\n${tb || 'No traceback available'}\n\`\`\`\n\n
\n\n` - ); + // Subtest descriptions can contain reprs like "", so they go + // inside code blocks rather than the HTML summary. + const body = subtests.length + ? subtests + .slice(0, MAX_SUBTESTS) + .map(({description, longrepr: tb}) => + codeBlock( + `SUBFAILED ${description}\n${truncateTraceback(tb) || 'No traceback available'}` + ) + ) + .join('') + + (subtests.length > MAX_SUBTESTS + ? `... and ${subtests.length - MAX_SUBTESTS} more failed subtests.\n\n` + : '') + : codeBlock(truncateTraceback(longrepr) || 'No traceback available'); + return `
${nodeid}${logLink}\n\n${body}
\n\n`; }) .join(''); } diff --git a/.github/workflows/scripts/report-backend-test-failures.test.js b/.github/workflows/scripts/report-backend-test-failures.test.js index 05be483fb99c..8cf87becfc9f 100644 --- a/.github/workflows/scripts/report-backend-test-failures.test.js +++ b/.github/workflows/scripts/report-backend-test-failures.test.js @@ -9,6 +9,7 @@ import { buildFailureBlocks, COMMENT_MARKER, commitMarker, + describeSubtest, extractNodeids, parseFailures, reportShard, @@ -67,6 +68,24 @@ const SETUP_FAILURE = { }, }; +// Shape written by sentry.testutils.pytest.json_report_reruns. +const SUBTEST_FAILURE = { + nodeid: 'tests/sentry/web/test_client_config.py::test_client_config_features', + outcome: 'failed', + setup: {outcome: 'passed', duration: 0.5}, + call: {outcome: 'failed', duration: 2.0, longrepr: 'contains 1 failed subtest'}, + subtests: [ + {outcome: 'passed', duration: 0.1, context: {msg: null, kwargs: {factory: "'a'"}}}, + { + outcome: 'failed', + duration: 0.1, + longrepr: + 'def test():\n> assert 1 == 2\nE AssertionError: subtest boom', + context: {msg: 'm', kwargs: {factory: '', n: '2'}}, + }, + ], +}; + // -- Helpers ----------------------------------------------------------------- let tmpDir; @@ -224,6 +243,24 @@ describe('parseFailures', () => { assert.equal(parseFailures([file], mockCore()).length, 0); }); + it('surfaces failed subtests with their context and traceback', () => { + const file = createArtifactFile( + 'pytest-results-backend-111-0', + 'pytest.json', + makePytestJson([SUBTEST_FAILURE]) + ); + + const failures = parseFailures([file], mockCore()); + + assert.equal(failures.length, 1); + assert.deepEqual(failures[0].subtests, [ + { + description: '[m] (factory=, n=2)', + longrepr: SUBTEST_FAILURE.subtests[1].longrepr, + }, + ]); + }); + it('aggregates failures across multiple files', () => { const file1 = createArtifactFile( 'pytest-results-backend-111-0', @@ -262,7 +299,47 @@ describe('extractNodeids', () => { }); }); +describe('describeSubtest', () => { + it('matches pytest SUBFAILED descriptions', () => { + assert.equal(describeSubtest({msg: 'x', kwargs: {a: '1'}}), '[x] (a=1)'); + assert.equal( + describeSubtest({msg: null, kwargs: {a: '1', b: "'s'"}}), + "(a=1, b='s')" + ); + assert.equal(describeSubtest({msg: null, kwargs: {}}), '()'); + }); +}); + describe('buildFailureBlocks', () => { + it('renders failed subtests instead of the parent summary', () => { + const blocks = buildFailureBlocks([ + { + nodeid: 'a::b', + longrepr: 'contains 1 failed subtest', + subtests: [ + {description: '()', longrepr: 'AssertionError: subtest boom'}, + ], + }, + ]); + assert.ok(blocks.includes('a::b')); + assert.ok( + blocks.includes('```\nSUBFAILED ()\nAssertionError: subtest boom\n```') + ); + assert.ok(!blocks.includes('contains 1 failed subtest')); + assert.deepEqual([...extractNodeids(blocks)], ['a::b']); + }); + + it('caps the number of rendered subtests', () => { + const subtests = Array.from({length: 7}, (_, i) => ({ + description: `(i=${i})`, + longrepr: `boom ${i}`, + })); + const blocks = buildFailureBlocks([{nodeid: 'a::b', longrepr: '', subtests}]); + assert.ok(blocks.includes('boom 4')); + assert.ok(!blocks.includes('boom 5')); + assert.ok(blocks.includes('... and 2 more failed subtests.')); + }); + it('renders a details block per failure', () => { const blocks = buildFailureBlocks([{nodeid: 'a::b::c', longrepr: 'traceback here'}]); assert.ok(blocks.includes('
')); diff --git a/.github/workflows/scripts/test_backend_test_durations.py b/.github/workflows/scripts/test_backend_test_durations.py index 95097fadd47b..b643733689c0 100644 --- a/.github/workflows/scripts/test_backend_test_durations.py +++ b/.github/workflows/scripts/test_backend_test_durations.py @@ -2,12 +2,25 @@ import importlib import json +import math +from pathlib import Path +from typing import Any + +import pytest # Module has a hyphen in its name, so use importlib. _mod = importlib.import_module("backend-test-durations") +ENV = { + "GITHUB_SHA": "abc123", + "GITHUB_RUN_ID": "42", + "GITHUB_RUN_ATTEMPT": "1", + "TEST_GROUP": "0", + "TOTAL_TEST_GROUPS": "2", +} + -def test_extract_sums_stages_per_file(tmp_path): +def test_extract_sums_stages_per_file(tmp_path: Path) -> None: report = tmp_path / "pytest.json" report.write_text( json.dumps( @@ -18,6 +31,9 @@ def test_extract_sums_stages_per_file(tmp_path): "setup": {"duration": 1.0}, "call": {"duration": 2.0}, "teardown": {"duration": 0.5}, + # Ran inside the call above; counting them again would double it. + "subtests": [{"duration": 1.5}, {"duration": 0.4}], + "reruns": [{"setup": {"duration": 9.0}, "call": {"duration": 9.0}}], }, {"nodeid": "tests/a.py::test_2[x]", "setup": {"duration": 0.5}}, {"nodeid": "tests/b.py::test_3", "call": {"duration": 4.0}}, @@ -25,15 +41,58 @@ def test_extract_sums_stages_per_file(tmp_path): } ) ) - assert _mod.extract(report) == {"files": {"tests/a.py": 4.0, "tests/b.py": 4.0}, "tests": 3} + assert _mod.extract(report, ENV) == { + "schema": 1, + "unit": "seconds", + "commit": "abc123", + "run_id": "42", + "run_attempt": "1", + "shard": 0, + "shards": 2, + "files": {"tests/a.py": 4.0, "tests/b.py": 4.0}, + "tests": 3, + } + +def shard( + index: int, files: dict[str, float], tests: int = 2, **overrides: object +) -> dict[str, Any]: + return { + "schema": 1, + "unit": "seconds", + "commit": "abc123", + "run_id": "42", + "run_attempt": "1", + "shard": index, + "shards": 2, + "files": files, + "tests": tests, + } | overrides -def test_merge_adds_up_shards(): - shards = [ - {"files": {"tests/a.py": 1.04, "tests/b.py": 2.0}, "tests": 2}, - {"files": {"tests/a.py": 1.0}, "tests": 2}, - ] + +def test_merge_adds_up_shards() -> None: + shards = [shard(0, {"tests/a.py": 1.04, "tests/b.py": 2.0}), shard(1, {"tests/a.py": 1.0})] assert _mod.merge(shards) == { + "source": {"commit": "abc123", "run_id": "42", "run_attempt": "1", "shards": 2}, "seconds_per_test": 1.01, "files": {"tests/a.py": 2.0, "tests/b.py": 2.0}, } + + +@pytest.mark.parametrize( + ("shards", "error"), + [ + ([shard(0, {})], "expected shards 0..1 once each"), + ([shard(0, {}), shard(0, {})], "expected shards 0..1 once each"), + ([shard(0, {}), shard(1, {}, run_attempt="2")], "more than one run"), + ([shard(0, {}), shard(1, {}, commit="def456")], "more than one run"), + ([shard(0, {}, commit=None), shard(1, {}, commit=None)], "missing run details"), + ([shard(0, {"tests/a.py": math.nan}), shard(1, {})], "non-finite"), + ([shard(0, {"tests/a.py": -1.0}), shard(1, {})], "non-finite"), + ([{"files": {}, "tests": 1}], "unsupported durations format"), + ([], "no shard durations"), + ], +) +def test_merge_rejects_inconsistent_shards(shards: list[dict[str, Any]], error: str) -> None: + with pytest.raises(ValueError, match=error): + _mod.merge(shards) diff --git a/.github/workflows/scripts/test_pytest_selected.py b/.github/workflows/scripts/test_pytest_selected.py index c43b74d6de9f..f0312a9fe067 100644 --- a/.github/workflows/scripts/test_pytest_selected.py +++ b/.github/workflows/scripts/test_pytest_selected.py @@ -4,7 +4,9 @@ import importlib import json import os +from contextlib import AbstractContextManager from pathlib import Path +from typing import Any from unittest import mock import pytest @@ -17,14 +19,14 @@ class TestPlanShards: - def test_packs_whole_files_by_weight(self): + def test_packs_whole_files_by_weight(self) -> None: # 10 + 1 vs 6 + 5 assert plan_shards({"a.py": 10, "b.py": 6, "c.py": 5, "d.py": 1}, 2) == [ Shard(["a.py", "d.py"]), Shard(["b.py", "c.py"]), ] - def test_splits_a_file_too_big_for_one_shard(self): + def test_splits_a_file_too_big_for_one_shard(self) -> None: # 80 tests over 4 shards: big.py is worth 3 shards, the rest share one. assert plan_shards({"a.py": 10, "big.py": 60, "b.py": 10}, 4) == [ Shard(["a.py", "b.py"]), @@ -33,18 +35,18 @@ def test_splits_a_file_too_big_for_one_shard(self): Shard(["big.py"], 3, 2), ] - def test_spare_shards_go_to_the_heaviest_file(self): + def test_spare_shards_go_to_the_heaviest_file(self) -> None: assert plan_shards({"a.py": 3, "b.py": 1}, 3) == [ Shard(["b.py"]), Shard(["a.py"], 2, 0), Shard(["a.py"], 2, 1), ] - def test_keeps_a_shard_for_the_small_files(self): + def test_keeps_a_shard_for_the_small_files(self) -> None: # big.py rounds up to both shards on its own. assert plan_shards({"big.py": 3, "a.py": 1}, 2) == [Shard(["big.py"]), Shard(["a.py"])] - def test_every_file_runs_exactly_once(self): + def test_every_file_runs_exactly_once(self) -> None: weights = {f"t{i}.py": i % 7 + 1 for i in range(37)} plan = plan_shards(weights, 5) assert len(plan) == 5 @@ -52,14 +54,21 @@ def test_every_file_runs_exactly_once(self): assert sorted(f for shard in plan for f in shard.files) == sorted(weights) -def run_main(tmp_path, selected, counts, total, group, exit_code=pytest.ExitCode.OK): +def run_main( + tmp_path: Path, + selected: list[str], + counts: dict[str, int], + total: int, + group: int, + exit_code: pytest.ExitCode = pytest.ExitCode.OK, +) -> tuple[int, list[tuple[object, ...]]]: selected_file = tmp_path / "selected-tests.txt" selected_file.write_text("".join(f"{path}\n" for path in selected)) if counts: (tmp_path / "selected-tests.counts.json").write_text(json.dumps(counts)) - calls = [] + calls: list[tuple[object, ...]] = [] - def fake_main(args): + def fake_main(args: list[str]) -> pytest.ExitCode: env = ("SELECTED_TESTS_FILE", "TOTAL_TEST_GROUPS", "TEST_GROUP") calls.append((args, *(os.environ[k] for k in env))) return exit_code @@ -74,23 +83,23 @@ def fake_main(args): class TestMain: - def test_runs_only_this_shards_files(self, tmp_path): + def test_runs_only_this_shards_files(self, tmp_path: Path) -> None: exit_code, calls = run_main(tmp_path, ["a.py", "b.py"], {"a.py": 5, "b.py": 3}, 2, 1) shard_file = tmp_path / "selected-tests.shard-1.txt" assert exit_code == 0 assert calls == [(["tests", "-q"], str(shard_file), "1", "0")] assert shard_file.read_text() == "b.py\n" - def test_split_file_shards_by_test(self, tmp_path): + def test_split_file_shards_by_test(self, tmp_path: Path) -> None: _, calls = run_main(tmp_path, ["a.py", "big.py"], {"a.py": 1, "big.py": 9}, 3, 2) assert calls[0][2:] == ("2", "1") assert (tmp_path / "selected-tests.shard-2.txt").read_text() == "big.py\n" - def test_single_shard_runs_pytest_unchanged(self, tmp_path): + def test_single_shard_runs_pytest_unchanged(self, tmp_path: Path) -> None: _, calls = run_main(tmp_path, ["a.py"], {}, 1, 0) assert calls == [(["tests", "-q"], str(tmp_path / "selected-tests.txt"), "1", "0")] - def test_empty_shard_is_not_a_failure(self, tmp_path): + def test_empty_shard_is_not_a_failure(self, tmp_path: Path) -> None: exit_code, _ = run_main( tmp_path, ["a.py", "b.py"], {}, 2, 0, exit_code=pytest.ExitCode.NO_TESTS_COLLECTED ) @@ -101,19 +110,19 @@ def test_empty_shard_is_not_a_failure(self, tmp_path): item_part = importlib.import_module("pytest_shard_plugin").item_part -def pieces_per_shard(n): +def pieces_per_shard(n: int) -> AbstractContextManager[object]: return mock.patch.object(_mod, "PIECES_PER_SHARD", n) -def write_files(root, files): +def write_files(root: Path, files: list[str]) -> None: for path in files: (root / path).parent.mkdir(parents=True, exist_ok=True) (root / path).write_text("def test_one(): pass\ndef test_two(): pass\n") -def assert_runs_every_test_once(plan, tests): +def assert_runs_every_test_once(plan: list[Any], tests: dict[str, list[str]]) -> None: """`tests` maps each file to its test ids. Checks each one runs on exactly one shard.""" - ran = [] + ran: list[str] = [] for shard in plan: for path in shard.files: part, parts = shard.parts.get(path, (0, 1)) @@ -122,7 +131,7 @@ def assert_runs_every_test_once(plan, tests): class TestPlanFullShards: - def test_packs_whole_files_by_weight(self): + def test_packs_whole_files_by_weight(self) -> None: weights = {f"t{i}.py": 1.0 for i in range(24)} | {"z.py": 0.0} with pieces_per_shard(1): plan = plan_full_shards(weights, 4) @@ -130,7 +139,7 @@ def test_packs_whole_files_by_weight(self): assert [len(s.files) for s in plan] == [7, 6, 6, 6] assert all(not s.parts for s in plan) - def test_splits_slow_files_across_shards(self): + def test_splits_slow_files_across_shards(self) -> None: # Shards hold 24s, so files over 2s are cut into 2s parts. weights = {"big.py": 10.0} | {f"t{i}.py": 1.0 for i in range(86)} with pieces_per_shard(12): @@ -140,19 +149,19 @@ def test_splits_slow_files_across_shards(self): assert sorted(s.parts["big.py"] for s in holders) == [(i, 4) for i in range(4)] assert all(len(s.parts) == 1 for s in plan) - def test_never_splits_a_file_past_the_shard_count(self): + def test_never_splits_a_file_past_the_shard_count(self) -> None: plan = plan_full_shards({"big.py": 100.0, "a.py": 1.0, "b.py": 1.0}, 3) assert plan is not None assert sorted(s.parts["big.py"] for s in plan) == [(0, 3), (1, 3), (2, 3)] - def test_no_plan_when_a_shard_would_be_empty(self): + def test_no_plan_when_a_shard_would_be_empty(self) -> None: assert plan_full_shards({"a.py": 0.0}, 2) is None - def test_same_weights_same_plan(self): + def test_same_weights_same_plan(self) -> None: weights = {f"t{i}.py": float(i % 13) for i in range(200)} assert plan_full_shards(weights, 7) == plan_full_shards(dict(reversed(weights.items())), 7) - def test_balances_shards(self): + def test_balances_shards(self) -> None: weights = {f"t{i}.py": float(i % 13 + 1) ** 2 for i in range(500)} | {"big.py": 900.0} plan = plan_full_shards(weights, 10) assert plan is not None @@ -161,7 +170,7 @@ def test_balances_shards(self): ] assert max(loads) - min(loads) <= 1.0 - def test_every_test_runs_exactly_once(self): + def test_every_test_runs_exactly_once(self) -> None: tests = {f"t{i}.py": [f"t{i}.py::test_{j}" for j in range(i % 50)] for i in range(300)} tests["big.py"] = [f"big.py::Test::test_{j}[{k}]" for j in range(40) for k in range(30)] weights = {path: len(ids) * 0.5 for path, ids in tests.items()} @@ -171,7 +180,7 @@ def test_every_test_runs_exactly_once(self): assert_runs_every_test_once(plan, tests) -def test_list_test_files_lists_python_files_under_roots_minus_ignores(tmp_path): +def test_list_test_files_lists_python_files_under_roots_minus_ignores(tmp_path: Path) -> None: for path in ( "tests/a/test_a.py", "tests/a/helpers.py", @@ -188,14 +197,20 @@ def test_list_test_files_lists_python_files_under_roots_minus_ignores(tmp_path): assert _mod.list_test_files(args) == ["tests/a/helpers.py", "tests/a/test_a.py"] -def run_full_main(tmp_path, files, durations, total, group): +def run_full_main( + tmp_path: Path, + files: list[str], + durations: dict[str, object] | None, + total: int, + group: int, +) -> tuple[int, list[tuple[object, ...]]]: write_files(tmp_path, files) durations_file = tmp_path / "durations.json" if durations is not None: durations_file.write_text(json.dumps(durations)) - calls = [] + calls: list[tuple[object, ...]] = [] - def fake_main(args): + def fake_main(args: list[str]) -> pytest.ExitCode: env = ("SELECTED_TESTS_FILE", "TOTAL_TEST_GROUPS", "TEST_GROUP", "PYTEST_SHARD_PARTS") calls.append((args, *(os.environ.get(k) for k in env))) return pytest.ExitCode.OK @@ -216,7 +231,7 @@ def fake_main(args): class TestFullRunMain: - def test_runs_only_this_shards_files(self, tmp_path): + def test_runs_only_this_shards_files(self, tmp_path: Path) -> None: files = ["tests/test_a.py", "tests/test_b.py", "tests/test_new.py"] durations = {"seconds_per_test": 1.0, "files": {"tests/test_a.py": 3.0}} with pieces_per_shard(1): @@ -229,7 +244,7 @@ def test_runs_only_this_shards_files(self, tmp_path): assert (tmp_path / shard_file).read_text() == "tests/test_b.py\ntests/test_new.py\n" assert json.loads((tmp_path / parts_file).read_text()) == {} - def test_new_and_renamed_files_run_once_with_estimated_weight(self, tmp_path): + def test_new_and_renamed_files_run_once_with_estimated_weight(self, tmp_path: Path) -> None: # test_renamed.py was test_old.py when the durations were recorded: the stale # entry is ignored and the new name is weighted by its 2 tests, like any new file. files = [f"tests/test_{i}.py" for i in range(8)] + ["tests/test_renamed.py"] @@ -252,11 +267,61 @@ def test_new_and_renamed_files_run_once_with_estimated_weight(self, tmp_path): assert plan is not None assert sorted(f for s in plan for f in s.files) == sorted(files) - def test_without_durations_every_shard_collects_everything(self, tmp_path): + def test_without_durations_every_shard_collects_everything(self, tmp_path: Path) -> None: _, calls = run_full_main(tmp_path, ["tests/test_a.py"], None, 3, 1) assert calls == [(["tests", "-q"], "", "3", "1", None)] +@pytest.mark.parametrize( + ("args", "supported"), + [ + (["tests", "-q"], True), + (["tests", "--ignore", "tests/skip"], True), + (["ABSOLUTE"], False), + (["../outside"], False), + (["tests", "--rootdir=."], False), + (["tests", "-c", "setup.cfg"], False), + (["-q"], False), + ], +) +def test_per_file_sharding_needs_paths_relative_to_the_working_directory( + tmp_path: Path, args: list[str], supported: bool +) -> None: + (tmp_path / "work" / "tests" / "skip").mkdir(parents=True) + (tmp_path / "outside").mkdir() + (tmp_path / "work" / "setup.cfg").touch() + args = [str(tmp_path / "work" / "tests") if a == "ABSOLUTE" else a for a in args] + with contextlib.chdir(tmp_path / "work"): + assert _mod.per_file_sharding_supported(args) is supported + + +def test_absolute_root_falls_back_to_hash_sharding(tmp_path: Path) -> None: + write_files(tmp_path, ["tests/test_a.py", "tests/test_b.py"]) + durations_file = tmp_path / "durations.json" + durations_file.write_text(json.dumps({"seconds_per_test": 1.0, "files": {}})) + calls: list[tuple[object, ...]] = [] + + def fake_main(args: list[str]) -> pytest.ExitCode: + calls.append((args, os.environ.get("SELECTED_TESTS_FILE"), os.environ["TEST_GROUP"])) + return pytest.ExitCode.OK + + args = [str(tmp_path / "tests"), "-q"] + env = { + "SELECTED_TESTS_FILE": "", + "TOTAL_TEST_GROUPS": "2", + "TEST_GROUP": "1", + "PYTEST_ADDOPTS": "", + } + with ( + contextlib.chdir(tmp_path), + mock.patch.dict(os.environ, env), + mock.patch.object(_mod, "DURATIONS_FILE", durations_file), + mock.patch.object(pytest, "main", fake_main), + ): + assert main(args) == 0 + assert calls == [(args, "", "1")] + + @pytest.mark.parametrize( ("args", "addopts", "filtered"), [ diff --git a/src/sentry/testutils/pytest/json_report_reruns.py b/src/sentry/testutils/pytest/json_report_reruns.py index 386e611847f6..e2954d85153d 100644 --- a/src/sentry/testutils/pytest/json_report_reruns.py +++ b/src/sentry/testutils/pytest/json_report_reruns.py @@ -1,21 +1,39 @@ -"""Extend the pytest-json-report plugin with a new "reruns" attribute. +"""Extend the pytest-json-report plugin with "reruns" and "subtests" attributes. Each test will (if it's run more than once) store its prior results in a -`reruns` array attribute, where each item may have `setup` `call` and -`teardown` sections. +`reruns` array attribute, where each item may have `setup` `call` `teardown` +and `subtests` sections. + +pytest 9 subtests (the `subtests` fixture) log one extra `call` report per +subtest under the parent's nodeid. pytest-json-report stores each of them as +the test's `call` stage, which made every subtest look like a rerun. They are +kept in a separate `subtests` array instead, next to the `call` they ran in, +each with its `context` (the `msg` and `kwargs` given to `subtests.test()`). +A passing subtest leaves the test's top-level `outcome` alone; a failing one +sets it to "failed". See also: * https://github.com/pytest-dev/pytest-rerunfailures * https://github.com/numirias/pytest-json-report + * https://docs.pytest.org/en/stable/how-to/subtests.html * https://getsentry.atlassian.net/browse/DEVINFRA-630 """ +from __future__ import annotations + +from collections import defaultdict +from collections.abc import Generator +from typing import Any + import pytest import pytest_jsonreport.plugin +from _pytest._io.saferepr import saferepr -JSONTestItem = dict[str, list[object]] +JSONTestItem = dict[str, Any] JSONTestItems = dict[str, JSONTestItem] +PHASES = ("setup", "call", "teardown") + def pytest_plugin_registered(plugin: object, manager: pytest.PytestPluginManager) -> None: """Only register if/when the plugin we're extending is registered.""" @@ -27,12 +45,55 @@ def pytest_plugin_registered(plugin: object, manager: pytest.PytestPluginManager class PytestRerunJSONReporter: def __init__(self, json_tests: JSONTestItems): self.json_tests = json_tests + # pytest-rerunfailures holds back an attempt's setup/call/teardown + # reports until the attempt ends, but subtest reports are logged live + # from inside the call. Buffer them until the call they belong to. + self.pending_subtests: defaultdict[str, list[dict[str, Any]]] = defaultdict(list) + + @pytest.hookimpl(wrapper=True) + def pytest_runtest_logreport( + self, report: pytest.TestReport + ) -> Generator[None, object, object]: + if not isinstance(report, pytest.SubtestReport): + return (yield) + + # JSONReport's own hook writes every report to json_testitem[report.when] + # and may replace the outcome (e.g. with "subtests passed"). Undo both. + missing = object() + before = self.json_tests.get(report.nodeid, {}) + prior_stage = before.get(report.when, missing) + prior_outcome = before.get("outcome", "passed") + + result = yield + + json_testitem = self.json_tests[report.nodeid] + stage = json_testitem.pop(report.when) + if prior_stage is not missing: + json_testitem[report.when] = prior_stage + # The parent's own report only turns "failed" when the subtests plugin + # on the same process counted the failure, which isn't the case on an + # xdist controller. + json_testitem["outcome"] = "failed" if report.failed else prior_outcome + + context = report.context + self.pending_subtests[report.nodeid].append( + { + **stage, + # kwargs can hold arbitrary objects (factories, classes), so + # keep the repr pytest prints in SUBFAILED lines. + "context": { + "msg": context.msg, + "kwargs": {str(k): saferepr(v) for k, v in context.kwargs.items()}, + }, + } + ) + return result def pytest_json_runtest_stage(self, report: pytest.TestReport) -> None: - assert self.json_tests is not None + if isinstance(report, pytest.SubtestReport): + return - nodeid = report.nodeid - json_testitem = self.json_tests[nodeid] + json_testitem = self.json_tests[report.nodeid] if report.when in json_testitem: # this is a new result of some kind -- record all prior data as a # "rerun" and start over fresh @@ -40,7 +101,11 @@ def pytest_json_runtest_stage(self, report: pytest.TestReport) -> None: reruns.append( { key: json_testitem.pop(key) - for key in ("setup", "call", "teardown") + for key in (*PHASES, "subtests") if key in json_testitem } ) + if report.when == "call": + subtests = self.pending_subtests.pop(report.nodeid, None) + if subtests: + json_testitem["subtests"] = subtests diff --git a/src/sentry/testutils/pytest/relay.py b/src/sentry/testutils/pytest/relay.py index d952a9c8d0c6..99bde5651425 100644 --- a/src/sentry/testutils/pytest/relay.py +++ b/src/sentry/testutils/pytest/relay.py @@ -6,6 +6,7 @@ import shutil import sys import time +from collections.abc import Callable from os import environ, path from unittest import mock from urllib.parse import urlparse @@ -31,6 +32,48 @@ def _relay_server_container_name() -> str: return "sentry_test_relay_server" +class RelayNotReadyError(Exception): + def __init__(self, url: str, last_result: str) -> None: + super().__init__(f"{url} was not ready in time, last result: {last_result}") + self.last_result = last_result + + +def wait_until_ready( + url: str, + *, + timeout: float, + interval: float = 0.05, + request_timeout: float = 1.0, + clock: Callable[[], float] = time.monotonic, + sleep: Callable[[float], None] = time.sleep, + probe: Callable[..., requests.Response] = requests.get, +) -> None: + """ + Poll `url` until it answers 200, or raise RelayNotReadyError after `timeout` + seconds. + + No probe starts after the deadline, and each probe's timeout is capped by + the time left, so the wait overshoots the deadline by at most one probe. + A 200 from a probe that started before the deadline counts as ready even + if it arrives after it. + """ + deadline = clock() + timeout + last_result = "not probed" + while (remaining := deadline - clock()) > 0: + try: + status = probe(url, timeout=min(request_timeout, remaining)).status_code + except requests.RequestException as e: + last_result = f"{type(e).__name__}: {e}" + else: + if status == 200: + return + last_result = f"HTTP {status}" + remaining = deadline - clock() + if remaining > 0: + sleep(min(interval, remaining)) + raise RelayNotReadyError(url, last_result) + + def _get_template_dir(): return path.abspath(path.join(path.dirname(__file__), "template")) @@ -193,23 +236,17 @@ def relay_server(relay_server_setup, settings): # seconds, and exponential backoff overshoots that by up to the last sleep. # The deadline matches the previous backoff schedule's total. readiness_url = f"{url}/api/relay/healthcheck/ready/" - deadline = time.monotonic() + 25.5 - while True: + try: + wait_until_ready(readiness_url, timeout=25.5) + except RelayNotReadyError as e: try: - if requests.get(readiness_url, timeout=1).status_code == 200: - break - except Exception as ex: - if time.monotonic() >= deadline: - _log.exception(str(ex)) - raise ValueError( - f"relay did not start in time (now: {datetime.datetime.now().isoformat()}) {url}:\n{container.logs().decode()}" - ) from ex - else: - if time.monotonic() >= deadline: - raise ValueError( - f"relay did not become ready in time (now: {datetime.datetime.now().isoformat()}) {readiness_url}:\n{container.logs().decode()}" - ) - time.sleep(0.05) + logs = container.logs().decode() + finally: + if not environ.get("RELAY_TEST_KEEP_CONTAINER", False): + _remove_container_if_exists(docker_client, container_name) + raise ValueError( + f"relay did not become ready in time (now: {datetime.datetime.now().isoformat()}, last result: {e.last_result}) {readiness_url}:\n{logs}" + ) from e yield {"url": relay_server_setup["url"]} diff --git a/src/sentry/testutils/pytest/transactional_flush.py b/src/sentry/testutils/pytest/transactional_flush.py index f554201e88ce..25381ac1a6fa 100644 --- a/src/sentry/testutils/pytest/transactional_flush.py +++ b/src/sentry/testutils/pytest/transactional_flush.py @@ -15,69 +15,134 @@ the last real flush produced, so ``post_migrate`` does not have to run again. When they don't match, Django's flush runs. Either way every table ends up empty apart from the ``post_migrate`` rows. + +Set ``SENTRY_LEGACY_TRANSACTIONAL_FLUSH=1`` to keep Django's own teardown. """ from __future__ import annotations -from collections.abc import Collection +import os +from collections.abc import Callable, Collection, Sequence +from dataclasses import dataclass import pytest from django.core.management import call_command -from django.db import connections, transaction +from django.db import OperationalError, connections, transaction from django.db.backends.base.base import BaseDatabaseWrapper +from django.db.backends.utils import CursorWrapper from django.test import TransactionTestCase from sentry.silo.safety import unguarded_write -# For each database alias: the tables that held rows right after Django's flush -# and post_migrate last ran, and a fingerprint of those rows. -_post_migrate_rows: dict[str, tuple[frozenset[str], str]] = {} +LEGACY_ENV_VAR = "SENTRY_LEGACY_TRANSACTIONAL_FLUSH" + +# A writer that is still open when cleanup starts makes the lock wait. Give up +# quickly and let Django's flush wait for it instead, as TRUNCATE always has. +LOCK_TIMEOUT = "1s" + +# lock_not_available and deadlock_detected. +_LOCK_FAILURES = frozenset({"55P03", "40P01"}) + + +@dataclass(frozen=True) +class _SeededRows: + """The tables that held rows right after Django's flush and post_migrate + last ran, and a fingerprint of those rows.""" + + tables: frozenset[str] + fingerprint: str + + +# Keyed by alias and database name: the test databases get renamed per xdist +# worker, and a session may point one alias at another database. +_post_migrate_rows: dict[tuple[str, str], _SeededRows] = {} + +# Tests set this to run code while the tables are locked and the inventory of +# rows to delete has been taken. +_after_inventory: Callable[[str], None] | None = None + + +class _NeedsDjangoFlush(Exception): + pass + + +def _cache_key(connection: BaseDatabaseWrapper) -> tuple[str, str]: + return (connection.alias, str(connection.settings_dict["NAME"])) -def _tables_with_pages(connection: BaseDatabaseWrapper) -> frozenset[str]: +def _existing_tables(connection: BaseDatabaseWrapper, cursor: CursorWrapper) -> list[str]: + """The tables Django's flush would truncate.""" + names = connection.introspection.django_table_names(include_views=False) + cursor.execute( + """ + SELECT relname FROM pg_class + WHERE relkind IN ('r', 'p') + AND pg_table_is_visible(oid) + AND relname = ANY(%s) + ORDER BY relname + """, + [names], + ) + return [row[0] for row in cursor.fetchall()] + + +def _tables_with_pages(cursor: CursorWrapper, tables: Sequence[str]) -> frozenset[str]: """ - Tables that Django's flush would truncate and that may hold rows. + The tables that may hold rows. A table that has never had rows has no pages, and checking that does not load each table's metadata, so it stays cheap on a fresh connection. """ - names = connection.introspection.django_table_names(include_views=False) - with connection.cursor() as cursor: - cursor.execute( - """ - SELECT relname FROM pg_class - WHERE relkind IN ('r', 'p') - AND pg_table_is_visible(oid) - AND relname = ANY(%s) - AND (relkind = 'p' OR pg_relation_size(oid) > 0) - """, - [names], - ) - return frozenset(row[0] for row in cursor.fetchall()) + cursor.execute( + """ + SELECT relname FROM pg_class + WHERE relkind IN ('r', 'p') + AND pg_table_is_visible(oid) + AND relname = ANY(%s) + AND (relkind = 'p' OR pg_relation_size(oid) > 0) + """, + [list(tables)], + ) + return frozenset(row[0] for row in cursor.fetchall()) -def _other_transactions_open(connection: BaseDatabaseWrapper) -> bool: +def _lock_tables(connection: BaseDatabaseWrapper, cursor: CursorWrapper) -> list[str]: """ - TRUNCATE waits for other open transactions (a leaked thread, say) to finish - and then removes their rows too. DELETE would not see their uncommitted - rows, so leave those cases to Django's flush. + Block writes to every table until the transaction ends, so no row can be + committed between taking the inventory and deleting. Reads still go + through. """ - with connection.cursor() as cursor: - cursor.execute( - """ - SELECT EXISTS ( - SELECT 1 FROM pg_stat_activity - WHERE datname = current_database() - AND pid <> pg_backend_pid() - AND xact_start IS NOT NULL - ) - """ + tables = _existing_tables(connection, cursor) + qn = connection.ops.quote_name + cursor.execute("SELECT set_config('lock_timeout', %s, true)", [LOCK_TIMEOUT]) + cursor.execute(f"LOCK TABLE {', '.join(qn(t) for t in tables)} IN EXCLUSIVE MODE") + return tables + + +def _other_transactions_open(cursor: CursorWrapper) -> bool: + """ + TRUNCATE waits for every other open transaction on its tables (a leaked + thread, say) to finish and then removes its rows too. The lock already + waits for writers; a transaction that has only read so far could still + write once the lock is released, so leave that case to Django's flush. + """ + cursor.execute( + """ + SELECT EXISTS ( + SELECT 1 FROM pg_stat_activity + WHERE datname = current_database() + AND pid <> pg_backend_pid() + AND xact_start IS NOT NULL ) - row = cursor.fetchone() - return bool(row and row[0]) + """ + ) + row = cursor.fetchone() + return bool(row and row[0]) -def _fingerprint(connection: BaseDatabaseWrapper, tables: Collection[str]) -> str: +def _fingerprint( + connection: BaseDatabaseWrapper, cursor: CursorWrapper, tables: Collection[str] +) -> str: if not tables: return "" qn = connection.ops.quote_name @@ -85,9 +150,32 @@ def _fingerprint(connection: BaseDatabaseWrapper, tables: Collection[str]) -> st f"SELECT %s, md5(coalesce(string_agg(r::text, ',' ORDER BY r::text), '')) FROM {qn(name)} r" for name in tables ) - with connection.cursor() as cursor: - cursor.execute(sql, list(tables)) - return repr(sorted(cursor.fetchall())) + cursor.execute(sql, list(tables)) + return repr(sorted(cursor.fetchall())) + + +def _is_lock_failure(error: OperationalError) -> bool: + return getattr(error.__cause__, "pgcode", None) in _LOCK_FAILURES + + +def _record_seeded_rows(db_name: str) -> None: + connection = connections[db_name] + key = _cache_key(connection) + _post_migrate_rows.pop(key, None) + try: + with transaction.atomic(using=db_name), connection.cursor() as cursor: + tables = _lock_tables(connection, cursor) + if _other_transactions_open(cursor): + # Another connection could be about to write a row that would + # then be mistaken for one of post_migrate's. + return + # TRUNCATE leaves every table without pages, so the tables that + # have pages now are the ones post_migrate wrote to. + seeded = _tables_with_pages(cursor, tables) + _post_migrate_rows[key] = _SeededRows(seeded, _fingerprint(connection, cursor, seeded)) + except OperationalError as e: + if not _is_lock_failure(e): + raise def _django_flush(db_name: str) -> None: @@ -100,36 +188,55 @@ def _django_flush(db_name: str) -> None: allow_cascade=False, inhibit_post_migrate=False, ) - # TRUNCATE leaves every table without pages, so the tables that have pages - # now are the ones post_migrate wrote to. + _record_seeded_rows(db_name) + + +def _delete_rows(db_name: str, seeded: _SeededRows) -> None: connection = connections[db_name] - seeded = _tables_with_pages(connection) - _post_migrate_rows[db_name] = (seeded, _fingerprint(connection, seeded)) + with transaction.atomic(using=db_name), connection.cursor() as cursor: + tables = _lock_tables(connection, cursor) + if _other_transactions_open(cursor): + raise _NeedsDjangoFlush + with_pages = _tables_with_pages(cursor, tables) + if ( + not seeded.tables <= with_pages + or _fingerprint(connection, cursor, seeded.tables) != seeded.fingerprint + ): + raise _NeedsDjangoFlush + to_delete = sorted(with_pages - seeded.tables) + if _after_inventory is not None: + _after_inventory(db_name) + if not to_delete: + return + # Foreign keys are deferred, so the order of the deletes does not + # matter. The fence keeps the outbox write audit from flagging the + # deletes. It wraps only the deletes because its closing query would + # fail in a transaction that a lock timeout has aborted. + qn = connection.ops.quote_name + with unguarded_write(using=db_name): + cursor.execute("; ".join(f"DELETE FROM {qn(t)}" for t in to_delete)) def flush_database(db_name: str) -> None: connection = connections[db_name] - tables = _tables_with_pages(connection) - - seeded = _post_migrate_rows.get(db_name) - if ( - seeded is None - or _other_transactions_open(connection) - or not seeded[0] <= tables - or _fingerprint(connection, seeded[0]) != seeded[1] - ): + seeded = _post_migrate_rows.get(_cache_key(connection)) + if seeded is None: _django_flush(db_name) return - - to_delete = sorted(tables - seeded[0]) - if not to_delete: - return - # Foreign keys are deferred, so the order of the deletes does not matter. - # The fence keeps the outbox write audit from flagging the deletes. - qn = connection.ops.quote_name - with transaction.atomic(using=db_name), unguarded_write(using=db_name): - with connection.cursor() as cursor: - cursor.execute("; ".join(f"DELETE FROM {qn(t)}" for t in to_delete)) + with connection.cursor() as cursor: + # Checked again under the lock; this only skips waiting out + # LOCK_TIMEOUT behind a transaction that is already known to be open. + if _other_transactions_open(cursor): + _django_flush(db_name) + return + try: + _delete_rows(db_name, seeded) + except _NeedsDjangoFlush: + _django_flush(db_name) + except OperationalError as e: + if not _is_lock_failure(e): + raise + _django_flush(db_name) _original_fixture_teardown = TransactionTestCase._fixture_teardown # type: ignore[attr-defined] @@ -149,4 +256,11 @@ def _fixture_teardown(self: TransactionTestCase) -> None: def pytest_configure(config: pytest.Config) -> None: + if os.environ.get(LEGACY_ENV_VAR) == "1": + return TransactionTestCase._fixture_teardown = _fixture_teardown # type: ignore[attr-defined] + + +def pytest_unconfigure(config: pytest.Config) -> None: + TransactionTestCase._fixture_teardown = _original_fixture_teardown # type: ignore[attr-defined] + _post_migrate_rows.clear() diff --git a/tests/sentry/spans/test_buffer.py b/tests/sentry/spans/test_buffer.py index 2987263ebc79..ebca3c0e0438 100644 --- a/tests/sentry/spans/test_buffer.py +++ b/tests/sentry/spans/test_buffer.py @@ -215,6 +215,17 @@ def process_spans(spans: Sequence[Span | _SplitBatch], buffer: SpansBuffer, now) buffer.process_spans(chunk, now) +def _fresh_buffer(buffer: SpansBuffer) -> SpansBuffer: + """ + Reset Redis and Python-side state between subtests. The fixture's setup + (options, Redis cluster client) is what the subtests share; a new + SpansBuffer drops attributes such as `any_shard_at_limit` and the cached + store that an earlier permutation may have changed. + """ + _delete_span_buffer_keys(buffer.client) + return SpansBuffer(assigned_shards=buffer.assigned_shards, slice_id=buffer.slice_id) + + def _span_order(spans: Sequence[Span | _SplitBatch]) -> str: """Name a permutation by span id initials, with `|` for a batch split, e.g. `d | b a c`.""" return " ".join("|" if isinstance(span, _SplitBatch) else span.span_id[0] for span in spans) @@ -570,7 +581,7 @@ def test_basic(buffer: SpansBuffer, subtests: pytest.Subtests) -> None: ] ): with subtests.test(msg=_span_order(spans)): - _delete_span_buffer_keys(buffer.client) + buffer = _fresh_buffer(buffer) process_spans(spans, buffer, now=0) assert_ttls(buffer.client) @@ -767,7 +778,7 @@ def test_deep(buffer: SpansBuffer, subtests: pytest.Subtests) -> None: ] for spans in itertools.permutations(spans_to_permute): with subtests.test(msg=_span_order(spans)): - _delete_span_buffer_keys(buffer.client) + buffer = _fresh_buffer(buffer) process_spans(spans, buffer, now=0) assert_ttls(buffer.client) @@ -843,7 +854,7 @@ def test_deep2(buffer: SpansBuffer, subtests: pytest.Subtests) -> None: ] ): with subtests.test(msg=_span_order(spans)): - _delete_span_buffer_keys(buffer.client) + buffer = _fresh_buffer(buffer) process_spans(spans, buffer, now=0) assert_ttls(buffer.client) @@ -912,7 +923,7 @@ def test_parent_in_other_project(buffer: SpansBuffer, subtests: pytest.Subtests) ] ): with subtests.test(msg=_span_order(spans)): - _delete_span_buffer_keys(buffer.client) + buffer = _fresh_buffer(buffer) process_spans(spans, buffer, now=0) assert_ttls(buffer.client) diff --git a/tests/sentry/testutils/pytest/test_json_report_reruns.py b/tests/sentry/testutils/pytest/test_json_report_reruns.py new file mode 100644 index 000000000000..44f149e82af3 --- /dev/null +++ b/tests/sentry/testutils/pytest/test_json_report_reruns.py @@ -0,0 +1,145 @@ +from __future__ import annotations + +import os +import subprocess +import sys +import textwrap +from pathlib import Path +from typing import Any + +import pytest + +from sentry.utils import json + +# Runs in a subprocess with the same plugin stack CI uses for backend shards. +INNER_TESTS = textwrap.dedent( + """ + import pytest + + attempts = {"flaky": 0, "flaky_subtests": 0} + + @pytest.fixture + def resource(): + yield 1 + + def test_passing_subtests(subtests, resource): + for i in range(3): + with subtests.test(msg="loop", i=i): + assert i < 3 + + def test_failing_subtest(subtests, resource): + for i in range(3): + with subtests.test(i=i, kind="int"): + assert i != 1, "subtest boom" + + def test_flaky(): + attempts["flaky"] += 1 + assert attempts["flaky"] > 1, "first attempt fails" + + def test_flaky_after_subtests(subtests): + attempts["flaky_subtests"] += 1 + for i in range(2): + with subtests.test(attempt=attempts["flaky_subtests"], i=i): + pass + assert attempts["flaky_subtests"] > 1, "first attempt fails" + """ +) + + +def run_inner(tmp_path: Path, *args: str) -> dict[str, dict[str, Any]]: + (tmp_path / "pytest.ini").write_text("[pytest]\n") + (tmp_path / "test_inner.py").write_text(INNER_TESTS) + # Keep the outer run's PYTEST_ADDOPTS (CI sets --reruns and --fail-slow), + # xdist worker variables and Django settings out of the inner run, and + # load exactly the plugins CI combines. + env = { + k: v for k, v in os.environ.items() if not k.startswith(("PYTEST_", "DJANGO_", "SENTRY_")) + } + env["PYTEST_DISABLE_PLUGIN_AUTOLOAD"] = "1" + plugins = ( + "xdist.plugin", + "pytest_rerunfailures", + "pytest_jsonreport.plugin", + "sentry.testutils.pytest.json_report_reruns", + ) + proc = subprocess.run( + [ + sys.executable, + "-m", + "pytest", + "-p", + "no:cacheprovider", + *(arg for plugin in plugins for arg in ("-p", plugin)), + "--json-report", + "--json-report-file=report.json", + "--reruns=1", + *args, + "test_inner.py", + ], + cwd=tmp_path, + env=env, + capture_output=True, + text=True, + timeout=120, + ) + # test_failing_subtest fails, so a clean report still exits 1. + assert proc.returncode == 1, proc.stdout + proc.stderr + report = json.loads((tmp_path / "report.json").read_text()) + return {test["nodeid"].split("::")[1]: test for test in report["tests"]} + + +@pytest.fixture(params=[(), ("-n", "2", "--dist=loadfile")], ids=["inprocess", "xdist"]) +def tests(request: pytest.FixtureRequest, tmp_path: Path) -> dict[str, dict[str, Any]]: + return run_inner(tmp_path, *request.param) + + +def test_passing_subtests_are_not_reruns(tests: dict[str, dict[str, Any]]) -> None: + test = tests["test_passing_subtests"] + assert "reruns" not in test + assert test["outcome"] == "passed" + assert [test[phase]["outcome"] for phase in ("setup", "call", "teardown")] == ["passed"] * 3 + assert [(s["outcome"], s["context"]) for s in test["subtests"]] == [ + ("passed", {"msg": "loop", "kwargs": {"i": "0"}}), + ("passed", {"msg": "loop", "kwargs": {"i": "1"}}), + ("passed", {"msg": "loop", "kwargs": {"i": "2"}}), + ] + assert all(s["duration"] >= 0 for s in test["subtests"]) + + +def test_failed_subtest_keeps_context_and_traceback(tests: dict[str, dict[str, Any]]) -> None: + test = tests["test_failing_subtest"] + assert "reruns" not in test + assert test["outcome"] == "failed" + assert test["setup"]["outcome"] == "passed" + assert test["teardown"]["outcome"] == "passed" + assert "contains 1 failed subtest" in test["call"]["longrepr"] + assert [s["outcome"] for s in test["subtests"]] == ["passed", "failed", "passed"] + failed = test["subtests"][1] + assert failed["context"] == {"msg": None, "kwargs": {"i": "1", "kind": "'int'"}} + assert "AssertionError: subtest boom" in failed["longrepr"] + + +def test_genuine_rerun_is_recorded(tests: dict[str, dict[str, Any]]) -> None: + test = tests["test_flaky"] + assert len(test["reruns"]) == 1 + assert test["reruns"][0]["call"]["outcome"] == "rerun" + assert "first attempt fails" in test["reruns"][0]["call"]["longrepr"] + assert [test[phase]["outcome"] for phase in ("setup", "call", "teardown")] == ["passed"] * 3 + assert "subtests" not in test + + +def test_subtests_stay_with_their_attempt(tests: dict[str, dict[str, Any]]) -> None: + test = tests["test_flaky_after_subtests"] + assert len(test["reruns"]) == 1 + rerun = test["reruns"][0] + assert rerun["call"]["outcome"] == "rerun" + assert [s["context"]["kwargs"] for s in rerun["subtests"]] == [ + {"attempt": "1", "i": "0"}, + {"attempt": "1", "i": "1"}, + ] + assert [s["context"]["kwargs"] for s in test["subtests"]] == [ + {"attempt": "2", "i": "0"}, + {"attempt": "2", "i": "1"}, + ] + assert test["setup"]["outcome"] == "passed" + assert test["call"]["outcome"] == "passed" diff --git a/tests/sentry/testutils/pytest/test_relay.py b/tests/sentry/testutils/pytest/test_relay.py new file mode 100644 index 000000000000..202184466ff7 --- /dev/null +++ b/tests/sentry/testutils/pytest/test_relay.py @@ -0,0 +1,106 @@ +from __future__ import annotations + +from collections.abc import Iterator +from dataclasses import dataclass, field + +import pytest +import requests + +from sentry.testutils.pytest.relay import RelayNotReadyError, wait_until_ready + +URL = "http://relay/api/relay/healthcheck/ready/" + + +@dataclass +class FakeRelay: + """A clock plus a readiness endpoint whose answers take simulated time.""" + + answers: Iterator[int | requests.RequestException] + probe_seconds: float = 0.01 + now: float = 0.0 + probe_timeouts: list[float] = field(default_factory=list) + sleeps: list[float] = field(default_factory=list) + + def clock(self) -> float: + return self.now + + def sleep(self, seconds: float) -> None: + self.sleeps.append(seconds) + self.now += seconds + + def probe(self, url: str, *, timeout: float) -> requests.Response: + assert url == URL + self.probe_timeouts.append(timeout) + answer = next(self.answers) + if isinstance(answer, requests.Timeout): + self.now += timeout + raise answer + self.now += self.probe_seconds + if isinstance(answer, requests.RequestException): + raise answer + response = requests.Response() + response.status_code = answer + return response + + def wait(self, timeout: float = 2.0) -> None: + wait_until_ready(URL, timeout=timeout, clock=self.clock, sleep=self.sleep, probe=self.probe) + + +def forever(answer: int | requests.RequestException) -> Iterator[int | requests.RequestException]: + while True: + yield answer + + +def test_ready_immediately() -> None: + relay = FakeRelay(iter([200])) + relay.wait() + assert relay.probe_timeouts == [1.0] + assert relay.sleeps == [] + + +def test_unavailable_then_ready() -> None: + relay = FakeRelay(iter([503, 503, 200])) + relay.wait() + assert len(relay.probe_timeouts) == 3 + assert relay.sleeps == [0.05, 0.05] + + +def test_connection_error_then_ready() -> None: + relay = FakeRelay(iter([requests.ConnectionError("refused"), 200])) + relay.wait() + assert len(relay.probe_timeouts) == 2 + + +def test_permanently_unavailable() -> None: + relay = FakeRelay(forever(503)) + with pytest.raises(RelayNotReadyError) as excinfo: + relay.wait(timeout=2.0) + assert excinfo.value.last_result == "HTTP 503" + assert relay.now == pytest.approx(2.0) + + +def test_request_timeout_is_reported() -> None: + relay = FakeRelay(forever(requests.Timeout("read timed out"))) + with pytest.raises(RelayNotReadyError) as excinfo: + relay.wait(timeout=2.5) + assert excinfo.value.last_result == "Timeout: read timed out" + # Two full-length probes, then one capped by the 0.4s left after the sleeps. + assert relay.probe_timeouts == pytest.approx([1.0, 1.0, 0.4]) + assert relay.now == pytest.approx(2.5) + + +def test_near_deadline_probe_is_capped_and_its_answer_counts() -> None: + relay = FakeRelay(iter([503, 200]), probe_seconds=1.9) + relay.wait(timeout=2.0) + # The second probe starts with 0.05s left and answers after the deadline. + assert relay.probe_timeouts == pytest.approx([1.0, 0.05]) + assert relay.sleeps == pytest.approx([0.05]) + assert relay.now > 2.0 + + +def test_sleep_never_passes_the_deadline() -> None: + relay = FakeRelay(forever(503), probe_seconds=1.98) + with pytest.raises(RelayNotReadyError): + relay.wait(timeout=2.0) + assert relay.probe_timeouts == [1.0] + assert relay.sleeps == pytest.approx([0.02]) diff --git a/tests/sentry/testutils/pytest/test_sentry.py b/tests/sentry/testutils/pytest/test_sentry.py index 02a945420878..a93f8af63411 100644 --- a/tests/sentry/testutils/pytest/test_sentry.py +++ b/tests/sentry/testutils/pytest/test_sentry.py @@ -181,6 +181,33 @@ def _check_sharding( IGNORE = ["--ignore", "tests/shardcheck/ignored"] +def test_relative_root_runs_each_test_once(project: Path) -> None: + logs = _check_sharding(project, ["tests", *IGNORE]) + assert all("1 of them split" in log for log in logs) + + +def test_relative_root_runs_each_test_once_with_xdist(project: Path) -> None: + logs = _check_sharding( + project, ["-p", "xdist.plugin", "-n", "2", "tests", *IGNORE], collect_only=False + ) + assert all("1 of them split" in log for log in logs) + + +def test_root_outside_tests_runs_each_test_once(project: Path) -> None: + logs = _check_sharding(project, ["tests", "other", *IGNORE]) + assert all("runs" in log for log in logs) + + +def test_absolute_root_falls_back_to_hash_sharding(project: Path) -> None: + logs = _check_sharding(project, [str(project / "tests"), *IGNORE]) + assert all("keeps its hash share" in log for log in logs) + + +def test_rootdir_option_falls_back_to_hash_sharding(project: Path) -> None: + logs = _check_sharding(project, ["--rootdir=.", "tests", *IGNORE]) + assert all("keeps its hash share" in log for log in logs) + + @pytest.mark.parametrize( ("args", "env"), [ diff --git a/tests/sentry/testutils/pytest/test_transactional_flush.py b/tests/sentry/testutils/pytest/test_transactional_flush.py new file mode 100644 index 000000000000..07d238a10e2a --- /dev/null +++ b/tests/sentry/testutils/pytest/test_transactional_flush.py @@ -0,0 +1,338 @@ +from __future__ import annotations + +import os +import threading +import time +from collections.abc import Callable, Iterator, Sequence +from contextlib import contextmanager +from typing import Any +from unittest import mock + +import pytest +from django.contrib.auth.models import Permission +from django.contrib.contenttypes.models import ContentType +from django.contrib.sites.models import Site +from django.db import connections +from django.test import TransactionTestCase as DjangoTransactionTestCase + +from sentry.testutils.cases import TransactionTestCase +from sentry.testutils.pytest import transactional_flush as tf +from sentry.testutils.silo import no_silo_test + +ALIASES = ("default", "control", "secondary") +WAIT_SECONDS = 30 + + +def _wait_for(condition: Callable[[], bool], what: str) -> None: + """Poll a condition that another connection's progress makes true.""" + deadline = time.monotonic() + WAIT_SECONDS + while not condition(): + assert time.monotonic() < deadline, f"timed out waiting for {what}" + threading.Event().wait(0.01) + + +def _scalar(alias: str, sql: str, params: Sequence[Any] | None = None) -> object: + with connections[alias].cursor() as cursor: + cursor.execute(sql, params) + row = cursor.fetchone() + assert row is not None + return row[0] + + +def _backend_pid(alias: str) -> int: + pid = _scalar(alias, "SELECT pg_backend_pid()") + assert isinstance(pid, int) + return pid + + +def _option_rows() -> object: + return _scalar("default", "SELECT count(*) FROM sentry_option") + + +def _insert_option_sql(key: str) -> str: + return ( + "INSERT INTO sentry_option (key, value, last_updated, last_updated_by) " + f"VALUES ('{key}', '1', now(), 'unknown')" + ) + + +def _tables_with_rows(alias: str) -> list[str]: + """Every Django table in the database that holds a row, pages or not.""" + connection = connections[alias] + qn = connection.ops.quote_name + with connection.cursor() as cursor: + tables = tf._existing_tables(connection, cursor) + cursor.execute( + " UNION ALL ".join(f"SELECT %s WHERE EXISTS (SELECT 1 FROM {qn(t)})" for t in tables), + tables, + ) + return sorted(row[0] for row in cursor.fetchall()) + + +def _post_migrate_contents() -> tuple[list[tuple[str, str]], list[tuple[str, str]], list[str]]: + """The post_migrate rows without their ids.""" + return ( + sorted(ContentType.objects.values_list("app_label", "model")), + sorted(Permission.objects.values_list("content_type__app_label", "codename")), + sorted(Site.objects.values_list("domain", flat=True)), + ) + + +@contextmanager +def _count_django_flushes() -> Iterator[list[str]]: + flushed: list[str] = [] + original = tf._django_flush + + def spy(db_name: str) -> None: + flushed.append(db_name) + original(db_name) + + with mock.patch.object(tf, "_django_flush", spy): + yield flushed + + +@no_silo_test +class TransactionalFlushTest(TransactionTestCase): + def flush_all(self) -> None: + for alias in ALIASES: + tf.flush_database(alias) + + def assert_flushed(self) -> None: + """Every table is empty apart from the rows a real Django flush leaves.""" + for alias in ALIASES: + seeded = tf._post_migrate_rows[tf._cache_key(connections[alias])] + assert _tables_with_rows(alias) == sorted(seeded.tables), alias + with connections[alias].cursor() as cursor: + fingerprint = tf._fingerprint(connections[alias], cursor, seeded.tables) + assert fingerprint == seeded.fingerprint, alias + + def setUp(self) -> None: + super().setUp() + # Each test starts from a recorded flush so none depends on what ran + # before it in the session. + self.flush_all() + self.baseline = _post_migrate_contents() + + def test_orm_and_raw_sql_writes_are_deleted(self) -> None: + org = self.create_organization(owner=self.create_user()) + project = self.create_project(organization=org) + self.create_monitor(organization=org, project=project) + with connections["default"].cursor() as cursor: + cursor.execute(_insert_option_sql("raw-sql")) + assert _tables_with_rows("secondary") == ["sentry_monitor"] + assert "sentry_organization" in _tables_with_rows("default") + assert "auth_user" in _tables_with_rows("control") + + with _count_django_flushes() as flushed: + self.flush_all() + + assert flushed == [] + self.assert_flushed() + + def test_toasted_value_is_deleted(self) -> None: + large = os.urandom(320 * 1024).hex() + with connections["default"].cursor() as cursor: + cursor.execute( + "INSERT INTO sentry_option (key, value, last_updated, last_updated_by) " + "VALUES ('toast', to_jsonb(%s::text), now(), 'unknown')", + [large], + ) + toast_size = _scalar( + "default", + "SELECT pg_relation_size(reltoastrelid) FROM pg_class WHERE relname = 'sentry_option'", + ) + assert isinstance(toast_size, int) and toast_size > 0 + + with _count_django_flushes() as flushed: + self.flush_all() + + assert flushed == [] + assert _option_rows() == 0 + self.assert_flushed() + + def test_changed_post_migrate_rows_fall_back_to_django_flush(self) -> None: + changes: list[Callable[[], object]] = [ + lambda: ContentType.objects.create(app_label="flushtest", model="thing"), + lambda: Permission.objects.filter(pk=Permission.objects.order_by("pk")[0].pk).update( + name="renamed" + ), + lambda: Site.objects.all().delete(), + ] + for change in changes: + change() + with _count_django_flushes() as flushed: + self.flush_all() + assert flushed == ["control"] + assert _post_migrate_contents() == self.baseline + self.assert_flushed() + + def test_cache_is_keyed_by_database_name(self) -> None: + connection = connections["control"] + seeded = tf._post_migrate_rows[tf._cache_key(connection)] + other = {("control", "some_other_database"): seeded} + with mock.patch.object(tf, "_post_migrate_rows", other), _count_django_flushes() as flushed: + tf.flush_database("control") + assert flushed == ["control"] + + def test_write_during_cleanup_waits_for_it(self) -> None: + # No pages, so the table is left out of the inventory taken below. + with connections["default"].cursor() as cursor: + cursor.execute("TRUNCATE sentry_option") + cleanup_pid = _backend_pid("default") + writer_pid: list[int] = [] + committed = threading.Event() + + def writer() -> None: + try: + writer_pid.append(_backend_pid("default")) + with connections["default"].cursor() as cursor: + cursor.execute(_insert_option_sql("concurrent")) + committed.set() + finally: + connections["default"].close() + + thread = threading.Thread(target=writer) + blocked_by_cleanup: list[bool] = [] + + def during_cleanup(db_name: str) -> None: + if db_name != "default": + return + thread.start() + + def settled() -> bool: + if committed.is_set(): + return True + if not writer_pid: + return False + blocked = _scalar( + "default", "SELECT %s = ANY(pg_blocking_pids(%s))", [cleanup_pid, writer_pid[0]] + ) + blocked_by_cleanup.append(bool(blocked)) + return bool(blocked) + + _wait_for(settled, "the writer to commit or wait for cleanup") + + with mock.patch.object(tf, "_after_inventory", during_cleanup): + tf.flush_database("default") + thread.join(WAIT_SECONDS) + + # Without the lock the insert commits between the inventory and the + # DELETE, and its row outlives the cleanup. + assert not thread.is_alive() + assert blocked_by_cleanup[-1:] == [True] + assert committed.is_set() + assert _option_rows() == 1 + tf.flush_database("default") + assert _option_rows() == 0 + + def _flush_with_open_writer(self) -> list[str]: + cleanup_pid = _backend_pid("default") + inserted = threading.Event() + + def writer() -> None: + try: + with connections["default"].cursor() as cursor: + cursor.execute("BEGIN") + cursor.execute(_insert_option_sql("open-writer")) + inserted.set() + + def truncate_waits() -> bool: + cursor.execute("SELECT pg_stat_clear_snapshot()") + cursor.execute( + "SELECT query, wait_event_type FROM pg_stat_activity WHERE pid = %s", + [cleanup_pid], + ) + query, wait_event_type = cursor.fetchone() + return query.startswith("TRUNCATE") and wait_event_type == "Lock" + + _wait_for(truncate_waits, "Django's flush to wait for the writer") + cursor.execute("COMMIT") + finally: + connections["default"].close() + + thread = threading.Thread(target=writer) + thread.start() + try: + assert inserted.wait(WAIT_SECONDS) + with _count_django_flushes() as flushed: + tf.flush_database("default") + finally: + thread.join(WAIT_SECONDS) + assert not thread.is_alive() + return flushed + + def test_open_writer_falls_back_to_django_flush(self) -> None: + flushed = self._flush_with_open_writer() + + assert flushed == ["default"] + assert _option_rows() == 0 + self.assert_flushed() + + def test_lock_timeout_falls_back_to_django_flush(self) -> None: + # Skip the open-transaction checks so the writer is only noticed by + # the lock timing out. + with ( + mock.patch.object(tf, "_other_transactions_open", return_value=False), + mock.patch.object(tf, "LOCK_TIMEOUT", "50ms"), + ): + flushed = self._flush_with_open_writer() + + assert flushed == ["default"] + assert _option_rows() == 0 + self.assert_flushed() + + +class _Case(DjangoTransactionTestCase): + databases = {"default"} + + def test_nothing(self) -> None: + pass + + +@pytest.mark.parametrize( + ("attribute", "value"), [("available_apps", ["sentry"]), ("serialized_rollback", True)] +) +def test_unsupported_options_use_django_teardown(attribute: str, value: object) -> None: + case = _Case("test_nothing") + setattr(case, attribute, value) + with ( + mock.patch.object(tf, "_original_fixture_teardown") as original, + mock.patch.object(tf, "flush_database") as flush_database, + ): + tf._fixture_teardown(case) + original.assert_called_once_with(case) + flush_database.assert_not_called() + + +def test_default_options_use_fast_flush() -> None: + case = _Case("test_nothing") + with ( + mock.patch.object(tf, "_original_fixture_teardown") as original, + mock.patch.object(tf, "flush_database") as flush_database, + ): + tf._fixture_teardown(case) + original.assert_not_called() + flush_database.assert_called_once_with("default") + + +def test_legacy_env_var_keeps_django_teardown(request: pytest.FixtureRequest) -> None: + original = tf._original_fixture_teardown + with mock.patch.object(DjangoTransactionTestCase, "_fixture_teardown", original): + with mock.patch.dict(os.environ, {tf.LEGACY_ENV_VAR: "1"}): + tf.pytest_configure(request.config) + assert DjangoTransactionTestCase._fixture_teardown is original # type: ignore[attr-defined] + + with mock.patch.dict(os.environ, {tf.LEGACY_ENV_VAR: ""}): + tf.pytest_configure(request.config) + assert DjangoTransactionTestCase._fixture_teardown is tf._fixture_teardown # type: ignore[attr-defined] + + +def test_unconfigure_restores_django_teardown(request: pytest.FixtureRequest) -> None: + cached = {("default", "test_region"): tf._SeededRows(frozenset(), "")} + with ( + mock.patch.object(DjangoTransactionTestCase, "_fixture_teardown", tf._fixture_teardown), + mock.patch.object(tf, "_post_migrate_rows", cached), + ): + tf.pytest_unconfigure(request.config) + assert DjangoTransactionTestCase._fixture_teardown is tf._original_fixture_teardown # type: ignore[attr-defined] + assert cached == {} From 10fb0551764ba80976e4ae4c6ae0369334e982fb Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Sat, 17 Oct 2026 19:03:56 +0000 Subject: [PATCH 3/3] ref(tests): Simplify the test infra follow-ups Keep the fixes and the tests that prove them, and drop the rest: the flush kill switch, test hook and pg_stat_activity check (an ACCESS EXCLUSIVE lock with a timeout covers it), the Relay readiness DI and exception class, the durations provenance checks, the failure-comment subtest cap, and the overlapping tests. The -m/-k and path checks are now one predicate in pytest-selected.py, and pytest_shard_plugin keeps the rootdir check, which only pytest can make. Co-Authored-By: Joshua Li <14209781+joshuarli@users.noreply.github.com> --- .../scripts/backend-test-durations.py | 61 +--- .github/workflows/scripts/pytest-selected.py | 71 ++--- .../workflows/scripts/pytest_shard_plugin.py | 5 +- .../scripts/report-backend-test-failures.js | 75 ++--- .../report-backend-test-failures.test.js | 105 ++----- .../scripts/test_backend_test_durations.py | 71 +---- .../workflows/scripts/test_pytest_selected.py | 108 ++----- .../testutils/pytest/json_report_reruns.py | 38 +-- src/sentry/testutils/pytest/relay.py | 45 +-- .../testutils/pytest/transactional_flush.py | 230 ++++---------- tests/sentry/spans/test_buffer.py | 7 +- .../pytest/test_json_report_reruns.py | 95 ++---- tests/sentry/testutils/pytest/test_relay.py | 120 ++------ tests/sentry/testutils/pytest/test_sentry.py | 212 +++---------- .../pytest/test_transactional_flush.py | 284 +++++------------- 15 files changed, 362 insertions(+), 1165 deletions(-) diff --git a/.github/workflows/scripts/backend-test-durations.py b/.github/workflows/scripts/backend-test-durations.py index 535867183df0..c1f632507de6 100644 --- a/.github/workflows/scripts/backend-test-durations.py +++ b/.github/workflows/scripts/backend-test-durations.py @@ -17,26 +17,12 @@ import argparse import json -import math -import os from collections import defaultdict -from collections.abc import Mapping from pathlib import Path -from typing import Any -SCHEMA = 1 -# Every shard of one attempt of one run must agree on these. -RUN_FIELDS = ("schema", "unit", "commit", "run_id", "run_attempt", "shards") - -def extract(pytest_json: Path, env: Mapping[str, str] | None = None) -> dict[str, Any]: - """Sum setup, call and teardown time per file from a pytest-json-report file. - - Subtests (`subtests` in the report) ran inside the parent's call, so the - call duration already includes them. Reruns are left out. - """ - if env is None: - env = os.environ +def extract(pytest_json: Path) -> dict[str, object]: + """Sum setup, call and teardown time per file from a pytest-json-report file.""" files: dict[str, float] = defaultdict(float) tests = 0 for test in json.loads(pytest_json.read_text()).get("tests", []): @@ -44,53 +30,18 @@ def extract(pytest_json: Path, env: Mapping[str, str] | None = None) -> dict[str path = test["nodeid"].split("::", 1)[0] for stage in ("setup", "call", "teardown"): files[path] += test.get(stage, {}).get("duration", 0.0) - return { - "schema": SCHEMA, - "unit": "seconds", - "commit": env.get("GITHUB_SHA"), - "run_id": env.get("GITHUB_RUN_ID"), - "run_attempt": env.get("GITHUB_RUN_ATTEMPT"), - "shard": int(env.get("TEST_GROUP", 0)), - "shards": int(env.get("TOTAL_TEST_GROUPS", 1)), - "files": dict(files), - "tests": tests, - } - - -def validate(shards: list[dict[str, Any]]) -> None: - """Reject inputs that would skew the weights: mixed runs, gaps, bad numbers.""" - if not shards: - raise ValueError("no shard durations given") - for shard in shards: - if shard.get("schema") != SCHEMA or shard.get("unit") != "seconds": - raise ValueError(f"unsupported durations format: {shard.get('schema')!r}") - runs = {tuple(shard.get(field) for field in RUN_FIELDS) for shard in shards} - if len(runs) > 1: - raise ValueError(f"durations come from more than one run: {sorted(map(str, runs))}") - (run,) = runs - if None in run: - raise ValueError(f"durations are missing run details: {dict(zip(RUN_FIELDS, run))}") - indexes = sorted(shard["shard"] for shard in shards) - if indexes != list(range(shards[0]["shards"])): - raise ValueError(f"expected shards 0..{shards[0]['shards'] - 1} once each, got {indexes}") - for shard in shards: - values = [*shard["files"].values(), shard["tests"]] - if not all(isinstance(v, (int, float)) and math.isfinite(v) and v >= 0 for v in values): - raise ValueError(f"shard {shard['shard']} has a negative or non-finite duration") + return {"files": dict(files), "tests": tests} -def merge(shards: list[dict[str, Any]]) -> dict[str, Any]: - validate(shards) +def merge(shards: list[dict[str, object]]) -> dict[str, object]: files: dict[str, float] = defaultdict(float) tests = 0 for shard in shards: - tests += shard["tests"] - for path, seconds in shard["files"].items(): + tests += shard["tests"] # type: ignore[operator] + for path, seconds in shard["files"].items(): # type: ignore[attr-defined] files[path] += seconds total = sum(files.values()) - first = shards[0] return { - "source": {field: first[field] for field in ("commit", "run_id", "run_attempt", "shards")}, # For files added since: their AST test count times this. "seconds_per_test": round(total / tests, 3) if tests else 0.0, "files": {path: round(files[path], 1) for path in sorted(files)}, diff --git a/.github/workflows/scripts/pytest-selected.py b/.github/workflows/scripts/pytest-selected.py index 4483461af775..6fa86e75d215 100644 --- a/.github/workflows/scripts/pytest-selected.py +++ b/.github/workflows/scripts/pytest-selected.py @@ -113,8 +113,12 @@ def plan_full_shards(weights: dict[str, float], total: int) -> list[Shard] | Non ] -def _parse_paths(args: list[str]) -> tuple[list[str], set[str]]: - """The existing paths in `args` and the --ignore'd ones, normalized.""" +def list_test_files(args: list[str]) -> list[str]: + """Every .py file pytest could collect from the paths in `args`, minus --ignore. + + Non-test files are harmless: pytest doesn't collect them either way. Missing + one that pytest does collect would drop its tests, so this errs wide. + """ roots, ignored = [], set() it = iter(args) for arg in it: @@ -124,16 +128,7 @@ def _parse_paths(args: list[str]) -> tuple[list[str], set[str]]: ignored.add(os.path.normpath(arg.split("=", 1)[1])) elif not arg.startswith("-") and os.path.exists(arg): roots.append(os.path.normpath(arg)) - return roots, ignored - -def list_test_files(args: list[str]) -> list[str]: - """Every .py file pytest could collect from the paths in `args`, minus --ignore. - - Non-test files are harmless: pytest doesn't collect them either way. Missing - one that pytest does collect would drop its tests, so this errs wide. - """ - roots, ignored = _parse_paths(args) files: set[str] = set() for root in roots: if os.path.isfile(root): @@ -150,26 +145,6 @@ def list_test_files(args: list[str]) -> list[str]: return sorted(files) -def per_file_sharding_supported(args: list[str]) -> bool: - """Whether the file paths written for the sentry plugin will match its nodeids. - - The plugin compares them with paths relative to pytest's rootdir, which - pytest_shard_plugin checks is the working directory. Anything else falls - back to per-test hash sharding. - """ - if any( - arg in ("-c", "--config-file", "--rootdir") - or arg.startswith(("--config-file=", "--rootdir=")) - for arg in [*args, *shlex.split(os.environ.get("PYTEST_ADDOPTS", ""))] - ): - return False - roots, _ = _parse_paths(args) - return bool(roots) and all( - not os.path.isabs(root) and root != os.pardir and not root.startswith(os.pardir + os.sep) - for root in roots - ) - - def file_weights(files: list[str]) -> dict[str, float]: """Seconds per file from the last full run, estimated from test counts for new files.""" durations = json.loads(DURATIONS_FILE.read_text()) @@ -187,35 +162,31 @@ def file_weights(files: list[str]) -> dict[str, float]: return weights -def filters_tests(args: list[str]) -> bool: - """Whether `args` or PYTEST_ADDOPTS select tests by marker or keyword. +def per_file_plan_applies(args: list[str], files: list[str]) -> bool: + """Whether planning by file gives each shard the tests pytest would run. - The file plan balances every test in each file, so with `-m`/`-k` most - shards can end up with files that have no selected tests at all (e.g. - `-m snuba_ci` in Snuba's CI). + `-m`/`-k` filter after the plan, so most shards could end up with no + selected tests (e.g. `-m snuba_ci` in Snuba's CI). The sentry plugin + matches the planned paths relative to the rootdir, which + pytest_shard_plugin checks is the working directory. """ every_arg = [*args, *shlex.split(os.environ.get("PYTEST_ADDOPTS", ""))] - return any(arg.startswith(("-m", "-k")) and not arg.startswith("--") for arg in every_arg) + if any(arg.startswith(("-m", "-k")) and not arg.startswith("--") for arg in every_arg): + return False + return not any(os.path.isabs(f) or f.startswith(os.pardir + os.sep) for f in files) def run_full_shard(args: list[str], total: int, group: int) -> int: - if filters_tests(args): - # Every shard collects everything; the sentry plugin then keeps its - # hash share of the items, which spreads the selected tests evenly. - print( - "pytest-selected: -m/-k given, so every shard collects everything " - "and keeps its hash share", - file=sys.stderr, - ) - return int(pytest.main(args)) - if not per_file_sharding_supported(args): + files = list_test_files(args) + if not per_file_plan_applies(args, files): + # As before per-file sharding: every shard collects everything and the + # sentry plugin keeps its hash share of the items. print( - "pytest-selected: paths are not relative to the working directory, " - "so every shard collects everything and keeps its hash share", + "pytest-selected: -m/-k or paths outside the working directory, so " + "every shard collects everything and keeps its hash share", file=sys.stderr, ) return int(pytest.main(args)) - files = list_test_files(args) plan = plan_full_shards(file_weights(files), total) if files else None if plan is None: return int(pytest.main(args)) diff --git a/.github/workflows/scripts/pytest_shard_plugin.py b/.github/workflows/scripts/pytest_shard_plugin.py index b42b24418c25..9e9a60e2d79d 100644 --- a/.github/workflows/scripts/pytest_shard_plugin.py +++ b/.github/workflows/scripts/pytest_shard_plugin.py @@ -21,9 +21,8 @@ def item_part(nodeid: str, parts: int) -> int: def pytest_configure(config: pytest.Config) -> None: - # The shard's file list is relative to the working directory, and the - # sentry plugin matches it against paths relative to the rootdir. If they - # differ, every file would be skipped (or kept) on every shard. + # pytest-selected.py writes paths relative to the working directory, and + # the sentry plugin matches them relative to the rootdir. if os.environ.get("PYTEST_SHARD_PARTS") and config.rootpath != Path.cwd().resolve(): raise pytest.UsageError( f"pytest_shard_plugin: rootdir {config.rootpath} is not the working " diff --git a/.github/workflows/scripts/report-backend-test-failures.js b/.github/workflows/scripts/report-backend-test-failures.js index 0d31358591f7..de645913e0b1 100644 --- a/.github/workflows/scripts/report-backend-test-failures.js +++ b/.github/workflows/scripts/report-backend-test-failures.js @@ -21,7 +21,6 @@ import {basename, dirname} from 'node:path'; const MAX_FAILURES = 30; const MAX_TRACEBACK_LINES = 50; -const MAX_SUBTESTS = 5; const MAX_BODY_LENGTH = 65_000; export const COMMENT_MARKER = ''; @@ -43,27 +42,26 @@ export function parseFailures(files, core) { .filter(t => t.outcome === 'failed') .map(t => ({ nodeid: t.nodeid ?? 'unknown', - longrepr: (t.call ?? t.setup ?? t.teardown ?? {}).longrepr ?? '', - // A parent whose subtests failed only says "contains N failed - // subtests"; the assertions live in each subtest's own longrepr. - subtests: (t.subtests ?? []) - .filter(s => s.outcome === 'failed') - .map(s => ({ - description: describeSubtest(s.context), - longrepr: s.longrepr ?? '', - })), + longrepr: + failedSubtests(t) || ((t.call ?? t.setup ?? t.teardown ?? {}).longrepr ?? ''), artifactDir, })); }); } -// Same shape pytest prints after SUBFAILED: "[msg] (k=v, ...)". -export function describeSubtest(context) { - const parts = []; - if (context?.msg !== null && context?.msg !== undefined) parts.push(`[${context.msg}]`); - const kwargs = Object.entries(context?.kwargs ?? {}); - if (kwargs.length) parts.push(`(${kwargs.map(([k, v]) => `${k}=${v}`).join(', ')})`); - return parts.join(' ') || '()'; +// The parent's own longrepr only says "contains N failed subtests". Render +// each failed subtest the way pytest's SUBFAILED line does. +function failedSubtests(test) { + return (test.subtests ?? []) + .filter(s => s.outcome === 'failed') + .map(({context, longrepr}) => { + const kwargs = Object.entries(context?.kwargs ?? {}).map(([k, v]) => `${k}=${v}`); + const msg = + context?.msg === null || context?.msg === undefined ? [] : [`[${context.msg}]`]; + const description = [...msg, ...(kwargs.length ? [`(${kwargs.join(', ')})`] : [])]; + return `SUBFAILED ${description.join(' ') || '()'}\n${longrepr ?? ''}`; + }) + .join('\n\n'); } // Maps artifact directory names to GitHub Actions job log URLs. @@ -125,39 +123,24 @@ function truncateBody(body) { ); } -function truncateTraceback(tb) { - if (!tb) return tb; - const lines = tb.split('\n'); - if (lines.length <= MAX_TRACEBACK_LINES) return tb; - return ( - lines.slice(0, MAX_TRACEBACK_LINES).join('\n') + - `\n... (${lines.length - MAX_TRACEBACK_LINES} more lines)` - ); -} - -const codeBlock = text => `\`\`\`\n${text}\n\`\`\`\n\n`; - // Renders
blocks for each failure (no header). export function buildFailureBlocks(failures) { return failures - .map(({nodeid, longrepr, subtests = [], jobUrl}) => { + .map(({nodeid, longrepr, jobUrl}) => { + let tb = longrepr; + if (tb) { + const lines = tb.split('\n'); + if (lines.length > MAX_TRACEBACK_LINES) { + tb = + lines.slice(0, MAX_TRACEBACK_LINES).join('\n') + + `\n... (${lines.length - MAX_TRACEBACK_LINES} more lines)`; + } + } const logLink = jobUrl ? ` — log` : ''; - // Subtest descriptions can contain reprs like "", so they go - // inside code blocks rather than the HTML summary. - const body = subtests.length - ? subtests - .slice(0, MAX_SUBTESTS) - .map(({description, longrepr: tb}) => - codeBlock( - `SUBFAILED ${description}\n${truncateTraceback(tb) || 'No traceback available'}` - ) - ) - .join('') + - (subtests.length > MAX_SUBTESTS - ? `... and ${subtests.length - MAX_SUBTESTS} more failed subtests.\n\n` - : '') - : codeBlock(truncateTraceback(longrepr) || 'No traceback available'); - return `
${nodeid}${logLink}\n\n${body}
\n\n`; + return ( + `
${nodeid}${logLink}\n\n` + + `\`\`\`\n${tb || 'No traceback available'}\n\`\`\`\n\n
\n\n` + ); }) .join(''); } diff --git a/.github/workflows/scripts/report-backend-test-failures.test.js b/.github/workflows/scripts/report-backend-test-failures.test.js index 8cf87becfc9f..7a93c2a06aa4 100644 --- a/.github/workflows/scripts/report-backend-test-failures.test.js +++ b/.github/workflows/scripts/report-backend-test-failures.test.js @@ -9,7 +9,6 @@ import { buildFailureBlocks, COMMENT_MARKER, commitMarker, - describeSubtest, extractNodeids, parseFailures, reportShard, @@ -68,24 +67,6 @@ const SETUP_FAILURE = { }, }; -// Shape written by sentry.testutils.pytest.json_report_reruns. -const SUBTEST_FAILURE = { - nodeid: 'tests/sentry/web/test_client_config.py::test_client_config_features', - outcome: 'failed', - setup: {outcome: 'passed', duration: 0.5}, - call: {outcome: 'failed', duration: 2.0, longrepr: 'contains 1 failed subtest'}, - subtests: [ - {outcome: 'passed', duration: 0.1, context: {msg: null, kwargs: {factory: "'a'"}}}, - { - outcome: 'failed', - duration: 0.1, - longrepr: - 'def test():\n> assert 1 == 2\nE AssertionError: subtest boom', - context: {msg: 'm', kwargs: {factory: '', n: '2'}}, - }, - ], -}; - // -- Helpers ----------------------------------------------------------------- let tmpDir; @@ -243,24 +224,6 @@ describe('parseFailures', () => { assert.equal(parseFailures([file], mockCore()).length, 0); }); - it('surfaces failed subtests with their context and traceback', () => { - const file = createArtifactFile( - 'pytest-results-backend-111-0', - 'pytest.json', - makePytestJson([SUBTEST_FAILURE]) - ); - - const failures = parseFailures([file], mockCore()); - - assert.equal(failures.length, 1); - assert.deepEqual(failures[0].subtests, [ - { - description: '[m] (factory=, n=2)', - longrepr: SUBTEST_FAILURE.subtests[1].longrepr, - }, - ]); - }); - it('aggregates failures across multiple files', () => { const file1 = createArtifactFile( 'pytest-results-backend-111-0', @@ -275,6 +238,34 @@ describe('parseFailures', () => { assert.equal(parseFailures([file1, file2], mockCore()).length, 2); }); + + it('uses failed subtests instead of the parent summary', () => { + const file = createArtifactFile( + 'pytest-results-backend-111-0', + 'pytest.json', + makePytestJson([ + { + nodeid: 'tests/a.py::test_a', + outcome: 'failed', + call: {outcome: 'failed', longrepr: 'contains 1 failed subtest'}, + subtests: [ + {outcome: 'passed', context: {msg: null, kwargs: {n: '1'}}}, + { + outcome: 'failed', + longrepr: 'AssertionError: boom', + context: {msg: 'm', kwargs: {f: '', n: '2'}}, + }, + ], + }, + ]) + ); + + const [failure] = parseFailures([file], mockCore()); + assert.equal( + failure.longrepr, + 'SUBFAILED [m] (f=, n=2)\nAssertionError: boom' + ); + }); }); describe('extractNodeids', () => { @@ -299,47 +290,7 @@ describe('extractNodeids', () => { }); }); -describe('describeSubtest', () => { - it('matches pytest SUBFAILED descriptions', () => { - assert.equal(describeSubtest({msg: 'x', kwargs: {a: '1'}}), '[x] (a=1)'); - assert.equal( - describeSubtest({msg: null, kwargs: {a: '1', b: "'s'"}}), - "(a=1, b='s')" - ); - assert.equal(describeSubtest({msg: null, kwargs: {}}), '()'); - }); -}); - describe('buildFailureBlocks', () => { - it('renders failed subtests instead of the parent summary', () => { - const blocks = buildFailureBlocks([ - { - nodeid: 'a::b', - longrepr: 'contains 1 failed subtest', - subtests: [ - {description: '()', longrepr: 'AssertionError: subtest boom'}, - ], - }, - ]); - assert.ok(blocks.includes('a::b')); - assert.ok( - blocks.includes('```\nSUBFAILED ()\nAssertionError: subtest boom\n```') - ); - assert.ok(!blocks.includes('contains 1 failed subtest')); - assert.deepEqual([...extractNodeids(blocks)], ['a::b']); - }); - - it('caps the number of rendered subtests', () => { - const subtests = Array.from({length: 7}, (_, i) => ({ - description: `(i=${i})`, - longrepr: `boom ${i}`, - })); - const blocks = buildFailureBlocks([{nodeid: 'a::b', longrepr: '', subtests}]); - assert.ok(blocks.includes('boom 4')); - assert.ok(!blocks.includes('boom 5')); - assert.ok(blocks.includes('... and 2 more failed subtests.')); - }); - it('renders a details block per failure', () => { const blocks = buildFailureBlocks([{nodeid: 'a::b::c', longrepr: 'traceback here'}]); assert.ok(blocks.includes('
')); diff --git a/.github/workflows/scripts/test_backend_test_durations.py b/.github/workflows/scripts/test_backend_test_durations.py index b643733689c0..adaa2f24620e 100644 --- a/.github/workflows/scripts/test_backend_test_durations.py +++ b/.github/workflows/scripts/test_backend_test_durations.py @@ -2,23 +2,11 @@ import importlib import json -import math from pathlib import Path -from typing import Any - -import pytest # Module has a hyphen in its name, so use importlib. _mod = importlib.import_module("backend-test-durations") -ENV = { - "GITHUB_SHA": "abc123", - "GITHUB_RUN_ID": "42", - "GITHUB_RUN_ATTEMPT": "1", - "TEST_GROUP": "0", - "TOTAL_TEST_GROUPS": "2", -} - def test_extract_sums_stages_per_file(tmp_path: Path) -> None: report = tmp_path / "pytest.json" @@ -31,9 +19,9 @@ def test_extract_sums_stages_per_file(tmp_path: Path) -> None: "setup": {"duration": 1.0}, "call": {"duration": 2.0}, "teardown": {"duration": 0.5}, - # Ran inside the call above; counting them again would double it. - "subtests": [{"duration": 1.5}, {"duration": 0.4}], - "reruns": [{"setup": {"duration": 9.0}, "call": {"duration": 9.0}}], + # Subtests ran inside the call; reruns don't count. + "subtests": [{"duration": 1.5}], + "reruns": [{"call": {"duration": 9.0}}], }, {"nodeid": "tests/a.py::test_2[x]", "setup": {"duration": 0.5}}, {"nodeid": "tests/b.py::test_3", "call": {"duration": 4.0}}, @@ -41,58 +29,15 @@ def test_extract_sums_stages_per_file(tmp_path: Path) -> None: } ) ) - assert _mod.extract(report, ENV) == { - "schema": 1, - "unit": "seconds", - "commit": "abc123", - "run_id": "42", - "run_attempt": "1", - "shard": 0, - "shards": 2, - "files": {"tests/a.py": 4.0, "tests/b.py": 4.0}, - "tests": 3, - } - - -def shard( - index: int, files: dict[str, float], tests: int = 2, **overrides: object -) -> dict[str, Any]: - return { - "schema": 1, - "unit": "seconds", - "commit": "abc123", - "run_id": "42", - "run_attempt": "1", - "shard": index, - "shards": 2, - "files": files, - "tests": tests, - } | overrides + assert _mod.extract(report) == {"files": {"tests/a.py": 4.0, "tests/b.py": 4.0}, "tests": 3} def test_merge_adds_up_shards() -> None: - shards = [shard(0, {"tests/a.py": 1.04, "tests/b.py": 2.0}), shard(1, {"tests/a.py": 1.0})] + shards = [ + {"files": {"tests/a.py": 1.04, "tests/b.py": 2.0}, "tests": 2}, + {"files": {"tests/a.py": 1.0}, "tests": 2}, + ] assert _mod.merge(shards) == { - "source": {"commit": "abc123", "run_id": "42", "run_attempt": "1", "shards": 2}, "seconds_per_test": 1.01, "files": {"tests/a.py": 2.0, "tests/b.py": 2.0}, } - - -@pytest.mark.parametrize( - ("shards", "error"), - [ - ([shard(0, {})], "expected shards 0..1 once each"), - ([shard(0, {}), shard(0, {})], "expected shards 0..1 once each"), - ([shard(0, {}), shard(1, {}, run_attempt="2")], "more than one run"), - ([shard(0, {}), shard(1, {}, commit="def456")], "more than one run"), - ([shard(0, {}, commit=None), shard(1, {}, commit=None)], "missing run details"), - ([shard(0, {"tests/a.py": math.nan}), shard(1, {})], "non-finite"), - ([shard(0, {"tests/a.py": -1.0}), shard(1, {})], "non-finite"), - ([{"files": {}, "tests": 1}], "unsupported durations format"), - ([], "no shard durations"), - ], -) -def test_merge_rejects_inconsistent_shards(shards: list[dict[str, Any]], error: str) -> None: - with pytest.raises(ValueError, match=error): - _mod.merge(shards) diff --git a/.github/workflows/scripts/test_pytest_selected.py b/.github/workflows/scripts/test_pytest_selected.py index f0312a9fe067..4c90a31fc0a5 100644 --- a/.github/workflows/scripts/test_pytest_selected.py +++ b/.github/workflows/scripts/test_pytest_selected.py @@ -203,6 +203,7 @@ def run_full_main( durations: dict[str, object] | None, total: int, group: int, + addopts: str = "", ) -> tuple[int, list[tuple[object, ...]]]: write_files(tmp_path, files) durations_file = tmp_path / "durations.json" @@ -219,7 +220,7 @@ def fake_main(args: list[str]) -> pytest.ExitCode: "SELECTED_TESTS_FILE": "", "TOTAL_TEST_GROUPS": str(total), "TEST_GROUP": str(group), - "PYTEST_ADDOPTS": "", + "PYTEST_ADDOPTS": addopts, } with ( contextlib.chdir(tmp_path), @@ -271,96 +272,29 @@ def test_without_durations_every_shard_collects_everything(self, tmp_path: Path) _, calls = run_full_main(tmp_path, ["tests/test_a.py"], None, 3, 1) assert calls == [(["tests", "-q"], "", "3", "1", None)] - -@pytest.mark.parametrize( - ("args", "supported"), - [ - (["tests", "-q"], True), - (["tests", "--ignore", "tests/skip"], True), - (["ABSOLUTE"], False), - (["../outside"], False), - (["tests", "--rootdir=."], False), - (["tests", "-c", "setup.cfg"], False), - (["-q"], False), - ], -) -def test_per_file_sharding_needs_paths_relative_to_the_working_directory( - tmp_path: Path, args: list[str], supported: bool -) -> None: - (tmp_path / "work" / "tests" / "skip").mkdir(parents=True) - (tmp_path / "outside").mkdir() - (tmp_path / "work" / "setup.cfg").touch() - args = [str(tmp_path / "work" / "tests") if a == "ABSOLUTE" else a for a in args] - with contextlib.chdir(tmp_path / "work"): - assert _mod.per_file_sharding_supported(args) is supported - - -def test_absolute_root_falls_back_to_hash_sharding(tmp_path: Path) -> None: - write_files(tmp_path, ["tests/test_a.py", "tests/test_b.py"]) - durations_file = tmp_path / "durations.json" - durations_file.write_text(json.dumps({"seconds_per_test": 1.0, "files": {}})) - calls: list[tuple[object, ...]] = [] - - def fake_main(args: list[str]) -> pytest.ExitCode: - calls.append((args, os.environ.get("SELECTED_TESTS_FILE"), os.environ["TEST_GROUP"])) - return pytest.ExitCode.OK - - args = [str(tmp_path / "tests"), "-q"] - env = { - "SELECTED_TESTS_FILE": "", - "TOTAL_TEST_GROUPS": "2", - "TEST_GROUP": "1", - "PYTEST_ADDOPTS": "", - } - with ( - contextlib.chdir(tmp_path), - mock.patch.dict(os.environ, env), - mock.patch.object(_mod, "DURATIONS_FILE", durations_file), - mock.patch.object(pytest, "main", fake_main), - ): - assert main(args) == 0 - assert calls == [(args, "", "1")] + def test_filtered_run_falls_back_to_hash_sharding(self, tmp_path: Path) -> None: + durations = {"seconds_per_test": 1.0, "files": {}} + _, calls = run_full_main( + tmp_path, ["tests/test_a.py", "tests/test_b.py"], durations, 16, 12, "-m snuba_ci" + ) + assert calls == [(["tests", "-q"], "", "16", "12", None)] @pytest.mark.parametrize( - ("args", "addopts", "filtered"), + ("args", "addopts", "files", "applies"), [ - (["tests", "-q"], "--reruns=5 -n 3 --dist=loadfile", False), - (["tests", "-m", "snuba_ci"], "", True), - (["tests", "-msnuba_ci"], "", True), - (["tests", "-k", "not slow"], "", True), - (["tests"], "-k 'not __In' -m snuba_ci -n 3", True), - (["tests", "--max-worker-restart=1"], "", False), + (["tests"], "--reruns=5 -n 3 --dist=loadfile", ["tests/a.py"], True), + (["tests", "--max-worker-restart=1"], "", ["tests/a.py"], True), + (["tests", "-m", "snuba_ci"], "", ["tests/a.py"], False), + (["tests", "-msnuba_ci"], "", ["tests/a.py"], False), + (["tests", "-k", "not slow"], "", ["tests/a.py"], False), + (["tests"], "-k 'not __In' -m snuba_ci", ["tests/a.py"], False), + (["/abs/tests"], "", ["/abs/tests/a.py"], False), + (["../tests"], "", ["../tests/a.py"], False), ], ) -def test_filters_tests(args: list[str], addopts: str, filtered: bool) -> None: +def test_per_file_plan_applies( + args: list[str], addopts: str, files: list[str], applies: bool +) -> None: with mock.patch.dict(os.environ, {"PYTEST_ADDOPTS": addopts}): - assert _mod.filters_tests(args) is filtered - - -def test_filtered_full_run_falls_back_to_hash_sharding(tmp_path: Path) -> None: - write_files(tmp_path, ["tests/test_a.py", "tests/test_b.py"]) - durations_file = tmp_path / "durations.json" - durations_file.write_text(json.dumps({"seconds_per_test": 1.0, "files": {}})) - calls: list[tuple[object, ...]] = [] - - def fake_main(args: list[str]) -> pytest.ExitCode: - env = ("SELECTED_TESTS_FILE", "TOTAL_TEST_GROUPS", "TEST_GROUP", "PYTEST_SHARD_PARTS") - calls.append((args, *(os.environ.get(k) for k in env))) - return pytest.ExitCode.OK - - env = { - "SELECTED_TESTS_FILE": "", - "TOTAL_TEST_GROUPS": "16", - "TEST_GROUP": "12", - "PYTEST_ADDOPTS": "-m snuba_ci", - } - with ( - contextlib.chdir(tmp_path), - mock.patch.dict(os.environ, env), - mock.patch.object(_mod, "DURATIONS_FILE", durations_file), - mock.patch.object(pytest, "main", fake_main), - ): - assert main(["tests", "-q"]) == 0 - # The sentry plugin hashes items into 16 groups, as before per-file sharding. - assert calls == [(["tests", "-q"], "", "16", "12", None)] + assert _mod.per_file_plan_applies(args, files) is applies diff --git a/src/sentry/testutils/pytest/json_report_reruns.py b/src/sentry/testutils/pytest/json_report_reruns.py index e2954d85153d..1972fa4275c2 100644 --- a/src/sentry/testutils/pytest/json_report_reruns.py +++ b/src/sentry/testutils/pytest/json_report_reruns.py @@ -4,13 +4,8 @@ `reruns` array attribute, where each item may have `setup` `call` `teardown` and `subtests` sections. -pytest 9 subtests (the `subtests` fixture) log one extra `call` report per -subtest under the parent's nodeid. pytest-json-report stores each of them as -the test's `call` stage, which made every subtest look like a rerun. They are -kept in a separate `subtests` array instead, next to the `call` they ran in, -each with its `context` (the `msg` and `kwargs` given to `subtests.test()`). -A passing subtest leaves the test's top-level `outcome` alone; a failing one -sets it to "failed". +Subtest reports share the parent's nodeid and `when == "call"`, so they are +kept in a `subtests` array instead of looking like reruns of the call. See also: * https://github.com/pytest-dev/pytest-rerunfailures @@ -45,9 +40,8 @@ def pytest_plugin_registered(plugin: object, manager: pytest.PytestPluginManager class PytestRerunJSONReporter: def __init__(self, json_tests: JSONTestItems): self.json_tests = json_tests - # pytest-rerunfailures holds back an attempt's setup/call/teardown - # reports until the attempt ends, but subtest reports are logged live - # from inside the call. Buffer them until the call they belong to. + # rerunfailures logs an attempt's reports when it ends, but subtest + # reports arrive live, so hold them until their attempt's call report. self.pending_subtests: defaultdict[str, list[dict[str, Any]]] = defaultdict(list) @pytest.hookimpl(wrapper=True) @@ -57,30 +51,24 @@ def pytest_runtest_logreport( if not isinstance(report, pytest.SubtestReport): return (yield) - # JSONReport's own hook writes every report to json_testitem[report.when] - # and may replace the outcome (e.g. with "subtests passed"). Undo both. - missing = object() + # JSONReport stores the subtest as the call stage and its outcome as + # "subtests passed", so put both back. before = self.json_tests.get(report.nodeid, {}) - prior_stage = before.get(report.when, missing) - prior_outcome = before.get("outcome", "passed") - + prior = {"outcome": "passed"} | { + k: before[k] for k in (report.when, "outcome") if k in before + } result = yield - json_testitem = self.json_tests[report.nodeid] stage = json_testitem.pop(report.when) - if prior_stage is not missing: - json_testitem[report.when] = prior_stage - # The parent's own report only turns "failed" when the subtests plugin - # on the same process counted the failure, which isn't the case on an - # xdist controller. - json_testitem["outcome"] = "failed" if report.failed else prior_outcome + json_testitem.update(prior) + # On an xdist controller the parent's report doesn't count failed subtests. + if report.failed: + json_testitem["outcome"] = "failed" context = report.context self.pending_subtests[report.nodeid].append( { **stage, - # kwargs can hold arbitrary objects (factories, classes), so - # keep the repr pytest prints in SUBFAILED lines. "context": { "msg": context.msg, "kwargs": {str(k): saferepr(v) for k, v in context.kwargs.items()}, diff --git a/src/sentry/testutils/pytest/relay.py b/src/sentry/testutils/pytest/relay.py index 99bde5651425..a97f84a58426 100644 --- a/src/sentry/testutils/pytest/relay.py +++ b/src/sentry/testutils/pytest/relay.py @@ -6,7 +6,6 @@ import shutil import sys import time -from collections.abc import Callable from os import environ, path from unittest import mock from urllib.parse import urlparse @@ -32,46 +31,20 @@ def _relay_server_container_name() -> str: return "sentry_test_relay_server" -class RelayNotReadyError(Exception): - def __init__(self, url: str, last_result: str) -> None: - super().__init__(f"{url} was not ready in time, last result: {last_result}") - self.last_result = last_result - - -def wait_until_ready( - url: str, - *, - timeout: float, - interval: float = 0.05, - request_timeout: float = 1.0, - clock: Callable[[], float] = time.monotonic, - sleep: Callable[[float], None] = time.sleep, - probe: Callable[..., requests.Response] = requests.get, -) -> None: - """ - Poll `url` until it answers 200, or raise RelayNotReadyError after `timeout` - seconds. - - No probe starts after the deadline, and each probe's timeout is capped by - the time left, so the wait overshoots the deadline by at most one probe. - A 200 from a probe that started before the deadline counts as ready even - if it arrives after it. - """ - deadline = clock() + timeout +def _wait_until_ready(url: str, timeout: float) -> None: + deadline = time.monotonic() + timeout last_result = "not probed" - while (remaining := deadline - clock()) > 0: + while (remaining := deadline - time.monotonic()) > 0: try: - status = probe(url, timeout=min(request_timeout, remaining)).status_code + status = requests.get(url, timeout=min(1, remaining)).status_code except requests.RequestException as e: last_result = f"{type(e).__name__}: {e}" else: if status == 200: return last_result = f"HTTP {status}" - remaining = deadline - clock() - if remaining > 0: - sleep(min(interval, remaining)) - raise RelayNotReadyError(url, last_result) + time.sleep(max(0, min(0.05, deadline - time.monotonic()))) + raise TimeoutError(last_result) def _get_template_dir(): @@ -237,15 +210,15 @@ def relay_server(relay_server_setup, settings): # The deadline matches the previous backoff schedule's total. readiness_url = f"{url}/api/relay/healthcheck/ready/" try: - wait_until_ready(readiness_url, timeout=25.5) - except RelayNotReadyError as e: + _wait_until_ready(readiness_url, timeout=25.5) + except TimeoutError as e: try: logs = container.logs().decode() finally: if not environ.get("RELAY_TEST_KEEP_CONTAINER", False): _remove_container_if_exists(docker_client, container_name) raise ValueError( - f"relay did not become ready in time (now: {datetime.datetime.now().isoformat()}, last result: {e.last_result}) {readiness_url}:\n{logs}" + f"relay did not become ready in time (now: {datetime.datetime.now().isoformat()}, last result: {e}) {readiness_url}:\n{logs}" ) from e yield {"url": relay_server_setup["url"]} diff --git a/src/sentry/testutils/pytest/transactional_flush.py b/src/sentry/testutils/pytest/transactional_flush.py index 25381ac1a6fa..f19a11871510 100644 --- a/src/sentry/testutils/pytest/transactional_flush.py +++ b/src/sentry/testutils/pytest/transactional_flush.py @@ -16,133 +16,63 @@ When they don't match, Django's flush runs. Either way every table ends up empty apart from the ``post_migrate`` rows. -Set ``SENTRY_LEGACY_TRANSACTIONAL_FLUSH=1`` to keep Django's own teardown. +The tables stay locked while the rows are counted and deleted, so a row +committed in between can't outlive the test. """ from __future__ import annotations -import os -from collections.abc import Callable, Collection, Sequence -from dataclasses import dataclass +from collections.abc import Collection import pytest from django.core.management import call_command from django.db import OperationalError, connections, transaction from django.db.backends.base.base import BaseDatabaseWrapper -from django.db.backends.utils import CursorWrapper from django.test import TransactionTestCase from sentry.silo.safety import unguarded_write -LEGACY_ENV_VAR = "SENTRY_LEGACY_TRANSACTIONAL_FLUSH" +# For each database alias: the tables that held rows right after Django's flush +# and post_migrate last ran, and a fingerprint of those rows. +_post_migrate_rows: dict[str, tuple[frozenset[str], str]] = {} -# A writer that is still open when cleanup starts makes the lock wait. Give up -# quickly and let Django's flush wait for it instead, as TRUNCATE always has. -LOCK_TIMEOUT = "1s" -# lock_not_available and deadlock_detected. -_LOCK_FAILURES = frozenset({"55P03", "40P01"}) - - -@dataclass(frozen=True) -class _SeededRows: - """The tables that held rows right after Django's flush and post_migrate - last ran, and a fingerprint of those rows.""" - - tables: frozenset[str] - fingerprint: str - - -# Keyed by alias and database name: the test databases get renamed per xdist -# worker, and a session may point one alias at another database. -_post_migrate_rows: dict[tuple[str, str], _SeededRows] = {} - -# Tests set this to run code while the tables are locked and the inventory of -# rows to delete has been taken. -_after_inventory: Callable[[str], None] | None = None - - -class _NeedsDjangoFlush(Exception): - pass - - -def _cache_key(connection: BaseDatabaseWrapper) -> tuple[str, str]: - return (connection.alias, str(connection.settings_dict["NAME"])) - - -def _existing_tables(connection: BaseDatabaseWrapper, cursor: CursorWrapper) -> list[str]: - """The tables Django's flush would truncate.""" - names = connection.introspection.django_table_names(include_views=False) - cursor.execute( - """ - SELECT relname FROM pg_class - WHERE relkind IN ('r', 'p') - AND pg_table_is_visible(oid) - AND relname = ANY(%s) - ORDER BY relname - """, - [names], - ) - return [row[0] for row in cursor.fetchall()] - - -def _tables_with_pages(cursor: CursorWrapper, tables: Sequence[str]) -> frozenset[str]: +def _tables_with_pages(connection: BaseDatabaseWrapper) -> frozenset[str]: """ - The tables that may hold rows. + Tables that Django's flush would truncate and that may hold rows. A table that has never had rows has no pages, and checking that does not load each table's metadata, so it stays cheap on a fresh connection. """ - cursor.execute( - """ - SELECT relname FROM pg_class - WHERE relkind IN ('r', 'p') - AND pg_table_is_visible(oid) - AND relname = ANY(%s) - AND (relkind = 'p' OR pg_relation_size(oid) > 0) - """, - [list(tables)], - ) - return frozenset(row[0] for row in cursor.fetchall()) + names = connection.introspection.django_table_names(include_views=False) + with connection.cursor() as cursor: + cursor.execute( + """ + SELECT relname FROM pg_class + WHERE relkind IN ('r', 'p') + AND pg_table_is_visible(oid) + AND relname = ANY(%s) + AND (relkind = 'p' OR pg_relation_size(oid) > 0) + """, + [names], + ) + return frozenset(row[0] for row in cursor.fetchall()) -def _lock_tables(connection: BaseDatabaseWrapper, cursor: CursorWrapper) -> list[str]: +def _lock_tables(connection: BaseDatabaseWrapper) -> None: """ - Block writes to every table until the transaction ends, so no row can be - committed between taking the inventory and deleting. Reads still go - through. + Takes the same lock as TRUNCATE. DELETE can't see another transaction's + uncommitted rows, so if one has touched any table (a leaked thread, say) + this times out and Django's TRUNCATE waits for it instead. """ - tables = _existing_tables(connection, cursor) + tables = connection.introspection.django_table_names(only_existing=True, include_views=False) qn = connection.ops.quote_name - cursor.execute("SELECT set_config('lock_timeout', %s, true)", [LOCK_TIMEOUT]) - cursor.execute(f"LOCK TABLE {', '.join(qn(t) for t in tables)} IN EXCLUSIVE MODE") - return tables - - -def _other_transactions_open(cursor: CursorWrapper) -> bool: - """ - TRUNCATE waits for every other open transaction on its tables (a leaked - thread, say) to finish and then removes its rows too. The lock already - waits for writers; a transaction that has only read so far could still - write once the lock is released, so leave that case to Django's flush. - """ - cursor.execute( - """ - SELECT EXISTS ( - SELECT 1 FROM pg_stat_activity - WHERE datname = current_database() - AND pid <> pg_backend_pid() - AND xact_start IS NOT NULL - ) - """ - ) - row = cursor.fetchone() - return bool(row and row[0]) + with connection.cursor() as cursor: + cursor.execute("SET LOCAL lock_timeout = '1s'") + cursor.execute(f"LOCK TABLE {', '.join(qn(t) for t in tables)} IN ACCESS EXCLUSIVE MODE") -def _fingerprint( - connection: BaseDatabaseWrapper, cursor: CursorWrapper, tables: Collection[str] -) -> str: +def _fingerprint(connection: BaseDatabaseWrapper, tables: Collection[str]) -> str: if not tables: return "" qn = connection.ops.quote_name @@ -150,32 +80,9 @@ def _fingerprint( f"SELECT %s, md5(coalesce(string_agg(r::text, ',' ORDER BY r::text), '')) FROM {qn(name)} r" for name in tables ) - cursor.execute(sql, list(tables)) - return repr(sorted(cursor.fetchall())) - - -def _is_lock_failure(error: OperationalError) -> bool: - return getattr(error.__cause__, "pgcode", None) in _LOCK_FAILURES - - -def _record_seeded_rows(db_name: str) -> None: - connection = connections[db_name] - key = _cache_key(connection) - _post_migrate_rows.pop(key, None) - try: - with transaction.atomic(using=db_name), connection.cursor() as cursor: - tables = _lock_tables(connection, cursor) - if _other_transactions_open(cursor): - # Another connection could be about to write a row that would - # then be mistaken for one of post_migrate's. - return - # TRUNCATE leaves every table without pages, so the tables that - # have pages now are the ones post_migrate wrote to. - seeded = _tables_with_pages(cursor, tables) - _post_migrate_rows[key] = _SeededRows(seeded, _fingerprint(connection, cursor, seeded)) - except OperationalError as e: - if not _is_lock_failure(e): - raise + with connection.cursor() as cursor: + cursor.execute(sql, list(tables)) + return repr(sorted(cursor.fetchall())) def _django_flush(db_name: str) -> None: @@ -188,54 +95,41 @@ def _django_flush(db_name: str) -> None: allow_cascade=False, inhibit_post_migrate=False, ) - _record_seeded_rows(db_name) - - -def _delete_rows(db_name: str, seeded: _SeededRows) -> None: + # TRUNCATE leaves every table without pages, so the tables that have pages + # now are the ones post_migrate wrote to. connection = connections[db_name] - with transaction.atomic(using=db_name), connection.cursor() as cursor: - tables = _lock_tables(connection, cursor) - if _other_transactions_open(cursor): - raise _NeedsDjangoFlush - with_pages = _tables_with_pages(cursor, tables) - if ( - not seeded.tables <= with_pages - or _fingerprint(connection, cursor, seeded.tables) != seeded.fingerprint - ): - raise _NeedsDjangoFlush - to_delete = sorted(with_pages - seeded.tables) - if _after_inventory is not None: - _after_inventory(db_name) - if not to_delete: - return - # Foreign keys are deferred, so the order of the deletes does not - # matter. The fence keeps the outbox write audit from flagging the - # deletes. It wraps only the deletes because its closing query would - # fail in a transaction that a lock timeout has aborted. - qn = connection.ops.quote_name - with unguarded_write(using=db_name): - cursor.execute("; ".join(f"DELETE FROM {qn(t)}" for t in to_delete)) + seeded = _tables_with_pages(connection) + _post_migrate_rows[db_name] = (seeded, _fingerprint(connection, seeded)) -def flush_database(db_name: str) -> None: +def _delete_rows(db_name: str, seeded: tuple[frozenset[str], str]) -> bool: + """Delete every row post_migrate didn't create, or return False if that isn't safe.""" connection = connections[db_name] - seeded = _post_migrate_rows.get(_cache_key(connection)) - if seeded is None: - _django_flush(db_name) - return - with connection.cursor() as cursor: - # Checked again under the lock; this only skips waiting out - # LOCK_TIMEOUT behind a transaction that is already known to be open. - if _other_transactions_open(cursor): - _django_flush(db_name) - return try: - _delete_rows(db_name, seeded) - except _NeedsDjangoFlush: - _django_flush(db_name) + with transaction.atomic(using=db_name): + _lock_tables(connection) + tables = _tables_with_pages(connection) + if not seeded[0] <= tables or _fingerprint(connection, seeded[0]) != seeded[1]: + return False + to_delete = sorted(tables - seeded[0]) + if to_delete: + # Foreign keys are deferred, so the order of the deletes does + # not matter. The fence keeps the outbox write audit from + # flagging the deletes. + qn = connection.ops.quote_name + with unguarded_write(using=db_name), connection.cursor() as cursor: + cursor.execute("; ".join(f"DELETE FROM {qn(t)}" for t in to_delete)) except OperationalError as e: - if not _is_lock_failure(e): + # lock_not_available or deadlock_detected + if getattr(e.__cause__, "pgcode", None) not in ("55P03", "40P01"): raise + return False + return True + + +def flush_database(db_name: str) -> None: + seeded = _post_migrate_rows.get(db_name) + if seeded is None or not _delete_rows(db_name, seeded): _django_flush(db_name) @@ -256,8 +150,6 @@ def _fixture_teardown(self: TransactionTestCase) -> None: def pytest_configure(config: pytest.Config) -> None: - if os.environ.get(LEGACY_ENV_VAR) == "1": - return TransactionTestCase._fixture_teardown = _fixture_teardown # type: ignore[attr-defined] diff --git a/tests/sentry/spans/test_buffer.py b/tests/sentry/spans/test_buffer.py index ebca3c0e0438..21df2832d886 100644 --- a/tests/sentry/spans/test_buffer.py +++ b/tests/sentry/spans/test_buffer.py @@ -216,12 +216,7 @@ def process_spans(spans: Sequence[Span | _SplitBatch], buffer: SpansBuffer, now) def _fresh_buffer(buffer: SpansBuffer) -> SpansBuffer: - """ - Reset Redis and Python-side state between subtests. The fixture's setup - (options, Redis cluster client) is what the subtests share; a new - SpansBuffer drops attributes such as `any_shard_at_limit` and the cached - store that an earlier permutation may have changed. - """ + # An earlier permutation can leave state such as `any_shard_at_limit` behind. _delete_span_buffer_keys(buffer.client) return SpansBuffer(assigned_shards=buffer.assigned_shards, slice_id=buffer.slice_id) diff --git a/tests/sentry/testutils/pytest/test_json_report_reruns.py b/tests/sentry/testutils/pytest/test_json_report_reruns.py index 44f149e82af3..1a94982503f8 100644 --- a/tests/sentry/testutils/pytest/test_json_report_reruns.py +++ b/tests/sentry/testutils/pytest/test_json_report_reruns.py @@ -11,47 +11,31 @@ from sentry.utils import json -# Runs in a subprocess with the same plugin stack CI uses for backend shards. INNER_TESTS = textwrap.dedent( """ - import pytest + attempts = 0 - attempts = {"flaky": 0, "flaky_subtests": 0} - - @pytest.fixture - def resource(): - yield 1 - - def test_passing_subtests(subtests, resource): + def test_subtests(subtests): for i in range(3): with subtests.test(msg="loop", i=i): - assert i < 3 - - def test_failing_subtest(subtests, resource): - for i in range(3): - with subtests.test(i=i, kind="int"): assert i != 1, "subtest boom" - def test_flaky(): - attempts["flaky"] += 1 - assert attempts["flaky"] > 1, "first attempt fails" - def test_flaky_after_subtests(subtests): - attempts["flaky_subtests"] += 1 - for i in range(2): - with subtests.test(attempt=attempts["flaky_subtests"], i=i): - pass - assert attempts["flaky_subtests"] > 1, "first attempt fails" + global attempts + attempts += 1 + with subtests.test(attempt=attempts): + pass + assert attempts > 1, "first attempt fails" """ ) -def run_inner(tmp_path: Path, *args: str) -> dict[str, dict[str, Any]]: +@pytest.fixture(params=[(), ("-n", "2", "--dist=loadfile")], ids=["inprocess", "xdist"]) +def tests(request: pytest.FixtureRequest, tmp_path: Path) -> dict[str, dict[str, Any]]: (tmp_path / "pytest.ini").write_text("[pytest]\n") (tmp_path / "test_inner.py").write_text(INNER_TESTS) - # Keep the outer run's PYTEST_ADDOPTS (CI sets --reruns and --fail-slow), - # xdist worker variables and Django settings out of the inner run, and - # load exactly the plugins CI combines. + # Keep the outer run's options and settings out, and load exactly the + # plugins CI combines. env = { k: v for k, v in os.environ.items() if not k.startswith(("PYTEST_", "DJANGO_", "SENTRY_")) } @@ -73,7 +57,7 @@ def run_inner(tmp_path: Path, *args: str) -> dict[str, dict[str, Any]]: "--json-report", "--json-report-file=report.json", "--reruns=1", - *args, + *request.param, "test_inner.py", ], cwd=tmp_path, @@ -82,64 +66,27 @@ def run_inner(tmp_path: Path, *args: str) -> dict[str, dict[str, Any]]: text=True, timeout=120, ) - # test_failing_subtest fails, so a clean report still exits 1. assert proc.returncode == 1, proc.stdout + proc.stderr report = json.loads((tmp_path / "report.json").read_text()) return {test["nodeid"].split("::")[1]: test for test in report["tests"]} -@pytest.fixture(params=[(), ("-n", "2", "--dist=loadfile")], ids=["inprocess", "xdist"]) -def tests(request: pytest.FixtureRequest, tmp_path: Path) -> dict[str, dict[str, Any]]: - return run_inner(tmp_path, *request.param) - - -def test_passing_subtests_are_not_reruns(tests: dict[str, dict[str, Any]]) -> None: - test = tests["test_passing_subtests"] +def test_subtests_are_not_reruns(tests: dict[str, dict[str, Any]]) -> None: + test = tests["test_subtests"] assert "reruns" not in test - assert test["outcome"] == "passed" - assert [test[phase]["outcome"] for phase in ("setup", "call", "teardown")] == ["passed"] * 3 + assert test["outcome"] == "failed" assert [(s["outcome"], s["context"]) for s in test["subtests"]] == [ ("passed", {"msg": "loop", "kwargs": {"i": "0"}}), - ("passed", {"msg": "loop", "kwargs": {"i": "1"}}), + ("failed", {"msg": "loop", "kwargs": {"i": "1"}}), ("passed", {"msg": "loop", "kwargs": {"i": "2"}}), ] - assert all(s["duration"] >= 0 for s in test["subtests"]) - - -def test_failed_subtest_keeps_context_and_traceback(tests: dict[str, dict[str, Any]]) -> None: - test = tests["test_failing_subtest"] - assert "reruns" not in test - assert test["outcome"] == "failed" - assert test["setup"]["outcome"] == "passed" - assert test["teardown"]["outcome"] == "passed" - assert "contains 1 failed subtest" in test["call"]["longrepr"] - assert [s["outcome"] for s in test["subtests"]] == ["passed", "failed", "passed"] - failed = test["subtests"][1] - assert failed["context"] == {"msg": None, "kwargs": {"i": "1", "kind": "'int'"}} - assert "AssertionError: subtest boom" in failed["longrepr"] - - -def test_genuine_rerun_is_recorded(tests: dict[str, dict[str, Any]]) -> None: - test = tests["test_flaky"] - assert len(test["reruns"]) == 1 - assert test["reruns"][0]["call"]["outcome"] == "rerun" - assert "first attempt fails" in test["reruns"][0]["call"]["longrepr"] - assert [test[phase]["outcome"] for phase in ("setup", "call", "teardown")] == ["passed"] * 3 - assert "subtests" not in test + assert "AssertionError: subtest boom" in test["subtests"][1]["longrepr"] -def test_subtests_stay_with_their_attempt(tests: dict[str, dict[str, Any]]) -> None: +def test_rerun_keeps_its_own_subtests(tests: dict[str, dict[str, Any]]) -> None: test = tests["test_flaky_after_subtests"] - assert len(test["reruns"]) == 1 - rerun = test["reruns"][0] + [rerun] = test["reruns"] assert rerun["call"]["outcome"] == "rerun" - assert [s["context"]["kwargs"] for s in rerun["subtests"]] == [ - {"attempt": "1", "i": "0"}, - {"attempt": "1", "i": "1"}, - ] - assert [s["context"]["kwargs"] for s in test["subtests"]] == [ - {"attempt": "2", "i": "0"}, - {"attempt": "2", "i": "1"}, - ] - assert test["setup"]["outcome"] == "passed" + assert [s["context"]["kwargs"] for s in rerun["subtests"]] == [{"attempt": "1"}] + assert [s["context"]["kwargs"] for s in test["subtests"]] == [{"attempt": "2"}] assert test["call"]["outcome"] == "passed" diff --git a/tests/sentry/testutils/pytest/test_relay.py b/tests/sentry/testutils/pytest/test_relay.py index 202184466ff7..99a710f69b1c 100644 --- a/tests/sentry/testutils/pytest/test_relay.py +++ b/tests/sentry/testutils/pytest/test_relay.py @@ -1,106 +1,50 @@ -from __future__ import annotations - -from collections.abc import Iterator -from dataclasses import dataclass, field +from unittest import mock import pytest import requests -from sentry.testutils.pytest.relay import RelayNotReadyError, wait_until_ready - -URL = "http://relay/api/relay/healthcheck/ready/" +from sentry.testutils.pytest import relay -@dataclass class FakeRelay: - """A clock plus a readiness endpoint whose answers take simulated time.""" + def __init__(self, *answers: int | requests.RequestException) -> None: + self.answers = list(answers) + self.now = 0.0 + self.probe_timeouts: list[float] = [] - answers: Iterator[int | requests.RequestException] - probe_seconds: float = 0.01 - now: float = 0.0 - probe_timeouts: list[float] = field(default_factory=list) - sleeps: list[float] = field(default_factory=list) + def get(self, url: str, timeout: float) -> mock.Mock: + self.probe_timeouts.append(timeout) + self.now += timeout + answer = self.answers.pop(0) if len(self.answers) > 1 else self.answers[0] + if isinstance(answer, Exception): + raise answer + return mock.Mock(status_code=answer) - def clock(self) -> float: + def monotonic(self) -> float: return self.now def sleep(self, seconds: float) -> None: - self.sleeps.append(seconds) + assert seconds >= 0 self.now += seconds - def probe(self, url: str, *, timeout: float) -> requests.Response: - assert url == URL - self.probe_timeouts.append(timeout) - answer = next(self.answers) - if isinstance(answer, requests.Timeout): - self.now += timeout - raise answer - self.now += self.probe_seconds - if isinstance(answer, requests.RequestException): - raise answer - response = requests.Response() - response.status_code = answer - return response - - def wait(self, timeout: float = 2.0) -> None: - wait_until_ready(URL, timeout=timeout, clock=self.clock, sleep=self.sleep, probe=self.probe) - - -def forever(answer: int | requests.RequestException) -> Iterator[int | requests.RequestException]: - while True: - yield answer - - -def test_ready_immediately() -> None: - relay = FakeRelay(iter([200])) - relay.wait() - assert relay.probe_timeouts == [1.0] - assert relay.sleeps == [] - - -def test_unavailable_then_ready() -> None: - relay = FakeRelay(iter([503, 503, 200])) - relay.wait() - assert len(relay.probe_timeouts) == 3 - assert relay.sleeps == [0.05, 0.05] - - -def test_connection_error_then_ready() -> None: - relay = FakeRelay(iter([requests.ConnectionError("refused"), 200])) - relay.wait() - assert len(relay.probe_timeouts) == 2 - - -def test_permanently_unavailable() -> None: - relay = FakeRelay(forever(503)) - with pytest.raises(RelayNotReadyError) as excinfo: - relay.wait(timeout=2.0) - assert excinfo.value.last_result == "HTTP 503" - assert relay.now == pytest.approx(2.0) - - -def test_request_timeout_is_reported() -> None: - relay = FakeRelay(forever(requests.Timeout("read timed out"))) - with pytest.raises(RelayNotReadyError) as excinfo: - relay.wait(timeout=2.5) - assert excinfo.value.last_result == "Timeout: read timed out" - # Two full-length probes, then one capped by the 0.4s left after the sleeps. - assert relay.probe_timeouts == pytest.approx([1.0, 1.0, 0.4]) - assert relay.now == pytest.approx(2.5) + def wait(self, timeout: float) -> None: + with ( + mock.patch.object(requests, "get", self.get), + mock.patch.object(relay, "time", mock.Mock(monotonic=self.monotonic, sleep=self.sleep)), + ): + relay._wait_until_ready("http://relay/ready", timeout=timeout) -def test_near_deadline_probe_is_capped_and_its_answer_counts() -> None: - relay = FakeRelay(iter([503, 200]), probe_seconds=1.9) - relay.wait(timeout=2.0) - # The second probe starts with 0.05s left and answers after the deadline. - assert relay.probe_timeouts == pytest.approx([1.0, 0.05]) - assert relay.sleeps == pytest.approx([0.05]) - assert relay.now > 2.0 +def test_retries_until_ready() -> None: + fake = FakeRelay(requests.ConnectionError("refused"), 503, 200) + fake.wait(timeout=25.5) + assert len(fake.probe_timeouts) == 3 -def test_sleep_never_passes_the_deadline() -> None: - relay = FakeRelay(forever(503), probe_seconds=1.98) - with pytest.raises(RelayNotReadyError): - relay.wait(timeout=2.0) - assert relay.probe_timeouts == [1.0] - assert relay.sleeps == pytest.approx([0.02]) +def test_gives_up_at_deadline_with_last_result() -> None: + fake = FakeRelay(requests.Timeout("read timed out")) + with pytest.raises(TimeoutError, match="Timeout: read timed out"): + fake.wait(timeout=2.5) + # The last probe is capped by the time left, so the deadline isn't overshot. + assert fake.probe_timeouts == pytest.approx([1, 1, 0.4]) + assert fake.now == pytest.approx(2.5) diff --git a/tests/sentry/testutils/pytest/test_sentry.py b/tests/sentry/testutils/pytest/test_sentry.py index a93f8af63411..f4550218a18e 100644 --- a/tests/sentry/testutils/pytest/test_sentry.py +++ b/tests/sentry/testutils/pytest/test_sentry.py @@ -1,11 +1,3 @@ -""" -Full backend runs shard by file: .github/workflows/scripts/pytest-selected.py -plans the shards, the sentry plugin's pytest_ignore_collect and -pytest_collection_modifyitems keep each shard's files, and pytest_shard_plugin -keeps each shard's part of a split file. These tests run that stack for real -in a throwaway project and check every test lands on exactly one shard. -""" - from __future__ import annotations import os @@ -15,16 +7,14 @@ from collections import Counter from pathlib import Path -import pytest - import sentry from sentry.utils import json SCRIPTS = Path(sentry.__file__).parents[2] / ".github" / "workflows" / "scripts" -SHARDS = 3 +SHARDS = 4 -# Runs pytest-selected.py like `python3 pytest-selected.py ARGS` in CI, with a -# durations file for the throwaway project and every slow-ish file split. +# Runs pytest-selected.py the way CI does, with this project's durations +# and no file split. DRIVER = textwrap.dedent( """ import importlib, os, sys @@ -38,189 +28,71 @@ """ ) -FILES = { - "conftest.py": ( - "from sentry.testutils.pytest.sentry import ( # noqa: F401\n" - " pytest_collection_modifyitems,\n" - " pytest_ignore_collect,\n" - ")\n" - ), - "pytest.ini": "[pytest]\nmarkers =\n snuba_ci\n", - "tests/shardcheck/test_big.py": textwrap.dedent( - """ - import pytest - - @pytest.mark.parametrize("n", range(40)) - def test_param(n): - pass - - class TestBig: - @pytest.mark.parametrize("n", range(20)) - def test_method(self, n): - pass - """ - ), - "tests/shardcheck/test_inherit.py": textwrap.dedent( - """ - class TestBase: - def test_shared(self): - pass - - class TestChild(TestBase): - def test_own(self): - pass - """ - ), - **{f"tests/shardcheck/test_{name}.py": "def test_a():\n pass\n" for name in "abcdef"}, - "tests/shardcheck/helpers.py": "VALUE = 1\n", - "tests/shardcheck/ignored/test_ignored.py": "def test_ignored():\n pass\n", - "other/test_outside.py": "def test_outside():\n pass\n", - # Like `-m snuba_ci`: a few marked tests in a few of many files. - **{ - f"tests/shardcheck/marked/test_m{i}.py": textwrap.dedent( - """ - import pytest - - @pytest.mark.snuba_ci - @pytest.mark.parametrize("n", range(4)) - def test_marked(n): - pass - - def test_unmarked(): - pass - """ - ) - for i in range(3) - }, - **{f"tests/shardcheck/marked/test_plain{i}.py": "def test_p():\n pass\n" for i in range(12)}, -} - -DURATIONS = { - "seconds_per_test": 0.1, - "files": {"tests/shardcheck/test_big.py": 30.0, "tests/shardcheck/test_inherit.py": 2.0}, -} +MARKED = textwrap.dedent( + """ + import pytest + @pytest.mark.snuba_ci + @pytest.mark.parametrize("n", range(10)) + def test_marked(n): + pass -@pytest.fixture -def project(tmp_path: Path) -> Path: - for path, content in FILES.items(): - (tmp_path / path).parent.mkdir(parents=True, exist_ok=True) - (tmp_path / path).write_text(content) - (tmp_path / "durations.json").write_text(json.dumps(DURATIONS)) - return tmp_path + def test_unmarked(): + pass + """ +) -def _run( - project: Path, command: list[str], env: dict[str, str] -) -> subprocess.CompletedProcess[str]: - # The outer run's own sharding, plugin and Django settings must not leak in. +def _collect(project: Path, command: list[str], env: dict[str, str]) -> Counter[str]: + # Keep the outer run's sharding, plugins and settings out. base = { k: v for k, v in os.environ.items() if not k.startswith(("PYTEST_", "DJANGO_", "SENTRY_", "TEST_GROUP", "TOTAL_TEST_GROUPS")) and k not in ("SELECTED_TESTS_FILE", "PYTHONPATH") } - base |= {"PYTEST_DISABLE_PLUGIN_AUTOLOAD": "1", "SCRIPTS": str(SCRIPTS)} + base |= { + "PYTEST_DISABLE_PLUGIN_AUTOLOAD": "1", + "SCRIPTS": str(SCRIPTS), + # How Snuba's CI filters sentry's tests. + "PYTEST_ADDOPTS": "-k 'not __In' -m snuba_ci", + } proc = subprocess.run( - command, + [*command, "-p", "no:cacheprovider", "tests", "--collect-only", "-q"], cwd=project, env=base | env, capture_output=True, text=True, timeout=300, ) + # Exit code 5 is a shard that collected nothing. assert proc.returncode == 0, proc.stdout + proc.stderr - return proc + return Counter(line for line in proc.stdout.splitlines() if "::" in line) -def _nodeids(proc: subprocess.CompletedProcess[str], collect_only: bool) -> Counter[str]: - lines = proc.stdout.splitlines() - if collect_only: - return Counter(line for line in lines if "::" in line) - return Counter(line.split(" ", 1)[1] for line in lines if line.startswith("PASSED ")) - - -def _check_sharding( - project: Path, - args: list[str], - *, - collect_only: bool = True, - env: dict[str, str] | None = None, -) -> list[str]: - mode = ["--collect-only", "-q"] if collect_only else ["-rA"] - # CI sets PY_COLORS=1, which would prefix the PASSED lines with escapes. - common = ["-p", "no:cacheprovider", "--color=no", *args, *mode] - env = env or {} - unsharded = _nodeids( - _run(project, [sys.executable, "-m", "pytest", *common], env), collect_only - ) - assert unsharded +def test_filtered_run_spreads_selected_tests_over_every_shard(tmp_path: Path) -> None: + files = { + "conftest.py": "from sentry.testutils.pytest.sentry import pytest_collection_modifyitems, pytest_ignore_collect # noqa: F401\n", + "pytest.ini": "[pytest]\nmarkers =\n snuba_ci\n", + "durations.json": json.dumps({"seconds_per_test": 0.1, "files": {}}), + **{f"tests/test_marked{i}.py": MARKED for i in range(2)}, + **{f"tests/test_plain{i}.py": "def test_p():\n pass\n" for i in range(12)}, + } + for path, content in files.items(): + (tmp_path / path).parent.mkdir(parents=True, exist_ok=True) + (tmp_path / path).write_text(content) + unsharded = _collect(tmp_path, [sys.executable, "-m", "pytest"], {}) sharded: Counter[str] = Counter() - logs = [] for group in range(SHARDS): - proc = _run( - project, - [sys.executable, "-c", DRIVER, *common], - env - | { + sharded += _collect( + tmp_path, + [sys.executable, "-c", DRIVER], + { "TOTAL_TEST_GROUPS": str(SHARDS), "TEST_GROUP": str(group), "TEST_GROUP_STRATEGY": "roundrobin", }, ) - shard = _nodeids(proc, collect_only) - assert shard, f"shard {group} ran nothing" - sharded += shard - logs.append(proc.stderr) - + assert len(unsharded) == 20 assert sharded == unsharded - return logs - - -IGNORE = ["--ignore", "tests/shardcheck/ignored"] - - -def test_relative_root_runs_each_test_once(project: Path) -> None: - logs = _check_sharding(project, ["tests", *IGNORE]) - assert all("1 of them split" in log for log in logs) - - -def test_relative_root_runs_each_test_once_with_xdist(project: Path) -> None: - logs = _check_sharding( - project, ["-p", "xdist.plugin", "-n", "2", "tests", *IGNORE], collect_only=False - ) - assert all("1 of them split" in log for log in logs) - - -def test_root_outside_tests_runs_each_test_once(project: Path) -> None: - logs = _check_sharding(project, ["tests", "other", *IGNORE]) - assert all("runs" in log for log in logs) - - -def test_absolute_root_falls_back_to_hash_sharding(project: Path) -> None: - logs = _check_sharding(project, [str(project / "tests"), *IGNORE]) - assert all("keeps its hash share" in log for log in logs) - - -def test_rootdir_option_falls_back_to_hash_sharding(project: Path) -> None: - logs = _check_sharding(project, ["--rootdir=.", "tests", *IGNORE]) - assert all("keeps its hash share" in log for log in logs) - - -@pytest.mark.parametrize( - ("args", "env"), - [ - (["tests", "-m", "snuba_ci"], {}), - (["tests", "-k", "test_marked"], {}), - # How Snuba's CI passes it. - (["tests"], {"PYTEST_ADDOPTS": "-k 'not __In' -m snuba_ci"}), - ], -) -def test_filtered_run_spreads_selected_tests_over_every_shard( - project: Path, args: list[str], env: dict[str, str] -) -> None: - # Planning by file would put the 3 marked files on at most 3 of the - # shards and leave the rest with nothing selected. - logs = _check_sharding(project, [*args, *IGNORE], collect_only=False, env=env) - assert all("keeps its hash share" in log for log in logs) diff --git a/tests/sentry/testutils/pytest/test_transactional_flush.py b/tests/sentry/testutils/pytest/test_transactional_flush.py index 07d238a10e2a..ddc6dab27dca 100644 --- a/tests/sentry/testutils/pytest/test_transactional_flush.py +++ b/tests/sentry/testutils/pytest/test_transactional_flush.py @@ -3,17 +3,16 @@ import os import threading import time -from collections.abc import Callable, Iterator, Sequence +from collections.abc import Callable, Iterator from contextlib import contextmanager from typing import Any from unittest import mock -import pytest from django.contrib.auth.models import Permission from django.contrib.contenttypes.models import ContentType from django.contrib.sites.models import Site from django.db import connections -from django.test import TransactionTestCase as DjangoTransactionTestCase +from django.db.backends.base.base import BaseDatabaseWrapper from sentry.testutils.cases import TransactionTestCase from sentry.testutils.pytest import transactional_flush as tf @@ -24,44 +23,34 @@ def _wait_for(condition: Callable[[], bool], what: str) -> None: - """Poll a condition that another connection's progress makes true.""" deadline = time.monotonic() + WAIT_SECONDS while not condition(): assert time.monotonic() < deadline, f"timed out waiting for {what}" - threading.Event().wait(0.01) + time.sleep(0.01) -def _scalar(alias: str, sql: str, params: Sequence[Any] | None = None) -> object: - with connections[alias].cursor() as cursor: +def _scalar(sql: str, params: list[Any] | None = None) -> Any: + with connections["default"].cursor() as cursor: cursor.execute(sql, params) row = cursor.fetchone() assert row is not None return row[0] -def _backend_pid(alias: str) -> int: - pid = _scalar(alias, "SELECT pg_backend_pid()") - assert isinstance(pid, int) - return pid - - -def _option_rows() -> object: - return _scalar("default", "SELECT count(*) FROM sentry_option") - - -def _insert_option_sql(key: str) -> str: - return ( - "INSERT INTO sentry_option (key, value, last_updated, last_updated_by) " - f"VALUES ('{key}', '1', now(), 'unknown')" - ) +def _insert_option(key: str) -> None: + with connections["default"].cursor() as cursor: + cursor.execute( + "INSERT INTO sentry_option (key, value, last_updated, last_updated_by) " + "VALUES (%s, to_jsonb(%s::text), now(), 'unknown')", + [key, os.urandom(320 * 1024).hex()], + ) def _tables_with_rows(alias: str) -> list[str]: - """Every Django table in the database that holds a row, pages or not.""" connection = connections[alias] qn = connection.ops.quote_name + tables = connection.introspection.django_table_names(only_existing=True, include_views=False) with connection.cursor() as cursor: - tables = tf._existing_tables(connection, cursor) cursor.execute( " UNION ALL ".join(f"SELECT %s WHERE EXISTS (SELECT 1 FROM {qn(t)})" for t in tables), tables, @@ -69,15 +58,6 @@ def _tables_with_rows(alias: str) -> list[str]: return sorted(row[0] for row in cursor.fetchall()) -def _post_migrate_contents() -> tuple[list[tuple[str, str]], list[tuple[str, str]], list[str]]: - """The post_migrate rows without their ids.""" - return ( - sorted(ContentType.objects.values_list("app_label", "model")), - sorted(Permission.objects.values_list("content_type__app_label", "codename")), - sorted(Site.objects.values_list("domain", flat=True)), - ) - - @contextmanager def _count_django_flushes() -> Iterator[list[str]]: flushed: list[str] = [] @@ -93,147 +73,96 @@ def spy(db_name: str) -> None: @no_silo_test class TransactionalFlushTest(TransactionTestCase): - def flush_all(self) -> None: + def setUp(self) -> None: + super().setUp() + # Start from a recorded flush so no test depends on what ran before. for alias in ALIASES: tf.flush_database(alias) def assert_flushed(self) -> None: - """Every table is empty apart from the rows a real Django flush leaves.""" for alias in ALIASES: - seeded = tf._post_migrate_rows[tf._cache_key(connections[alias])] - assert _tables_with_rows(alias) == sorted(seeded.tables), alias - with connections[alias].cursor() as cursor: - fingerprint = tf._fingerprint(connections[alias], cursor, seeded.tables) - assert fingerprint == seeded.fingerprint, alias + seeded_tables, _ = tf._post_migrate_rows[alias] + assert _tables_with_rows(alias) == sorted(seeded_tables), alias - def setUp(self) -> None: - super().setUp() - # Each test starts from a recorded flush so none depends on what ran - # before it in the session. - self.flush_all() - self.baseline = _post_migrate_contents() - - def test_orm_and_raw_sql_writes_are_deleted(self) -> None: + def test_deletes_rows_in_every_database(self) -> None: org = self.create_organization(owner=self.create_user()) - project = self.create_project(organization=org) - self.create_monitor(organization=org, project=project) - with connections["default"].cursor() as cursor: - cursor.execute(_insert_option_sql("raw-sql")) - assert _tables_with_rows("secondary") == ["sentry_monitor"] - assert "sentry_organization" in _tables_with_rows("default") - assert "auth_user" in _tables_with_rows("control") + self.create_monitor(organization=org, project=self.create_project(organization=org)) + _insert_option("raw-sql-and-toast") with _count_django_flushes() as flushed: - self.flush_all() + for alias in ALIASES: + tf.flush_database(alias) assert flushed == [] self.assert_flushed() - def test_toasted_value_is_deleted(self) -> None: - large = os.urandom(320 * 1024).hex() - with connections["default"].cursor() as cursor: - cursor.execute( - "INSERT INTO sentry_option (key, value, last_updated, last_updated_by) " - "VALUES ('toast', to_jsonb(%s::text), now(), 'unknown')", - [large], - ) - toast_size = _scalar( - "default", - "SELECT pg_relation_size(reltoastrelid) FROM pg_class WHERE relname = 'sentry_option'", - ) - assert isinstance(toast_size, int) and toast_size > 0 + def test_changed_post_migrate_rows_fall_back_to_django_flush(self) -> None: + baseline = sorted(Permission.objects.values_list("codename", "name")) + Permission.objects.filter(pk=Permission.objects.order_by("pk")[0].pk).update(name="x") + ContentType.objects.create(app_label="flushtest", model="thing") + Site.objects.all().delete() with _count_django_flushes() as flushed: - self.flush_all() - - assert flushed == [] - assert _option_rows() == 0 - self.assert_flushed() - - def test_changed_post_migrate_rows_fall_back_to_django_flush(self) -> None: - changes: list[Callable[[], object]] = [ - lambda: ContentType.objects.create(app_label="flushtest", model="thing"), - lambda: Permission.objects.filter(pk=Permission.objects.order_by("pk")[0].pk).update( - name="renamed" - ), - lambda: Site.objects.all().delete(), - ] - for change in changes: - change() - with _count_django_flushes() as flushed: - self.flush_all() - assert flushed == ["control"] - assert _post_migrate_contents() == self.baseline - self.assert_flushed() - - def test_cache_is_keyed_by_database_name(self) -> None: - connection = connections["control"] - seeded = tf._post_migrate_rows[tf._cache_key(connection)] - other = {("control", "some_other_database"): seeded} - with mock.patch.object(tf, "_post_migrate_rows", other), _count_django_flushes() as flushed: tf.flush_database("control") + assert flushed == ["control"] + assert sorted(Permission.objects.values_list("codename", "name")) == baseline + assert not ContentType.objects.filter(app_label="flushtest").exists() + assert Site.objects.exists() - def test_write_during_cleanup_waits_for_it(self) -> None: - # No pages, so the table is left out of the inventory taken below. + def test_write_during_cleanup_is_deleted(self) -> None: + # An empty table has no pages, so it's left out of the inventory. with connections["default"].cursor() as cursor: cursor.execute("TRUNCATE sentry_option") - cleanup_pid = _backend_pid("default") - writer_pid: list[int] = [] - committed = threading.Event() + cleanup_pid = _scalar("SELECT pg_backend_pid()") + writer_pid: list[object] = [] def writer() -> None: try: - writer_pid.append(_backend_pid("default")) - with connections["default"].cursor() as cursor: - cursor.execute(_insert_option_sql("concurrent")) - committed.set() + writer_pid.append(_scalar("SELECT pg_backend_pid()")) + _insert_option("concurrent") finally: connections["default"].close() thread = threading.Thread(target=writer) - blocked_by_cleanup: list[bool] = [] - - def during_cleanup(db_name: str) -> None: - if db_name != "default": - return - thread.start() - - def settled() -> bool: - if committed.is_set(): - return True - if not writer_pid: - return False - blocked = _scalar( - "default", "SELECT %s = ANY(pg_blocking_pids(%s))", [cleanup_pid, writer_pid[0]] + original = tf._tables_with_pages + + def inventory_then_write(connection: BaseDatabaseWrapper) -> frozenset[str]: + tables = original(connection) + if connection.alias == "default": + thread.start() + # Without the lock the insert would commit right here. + _wait_for( + lambda: bool(writer_pid) + and bool( + _scalar( + "SELECT %s = ANY(pg_blocking_pids(%s))", [cleanup_pid, writer_pid[0]] + ) + ), + "the writer to wait for cleanup", ) - blocked_by_cleanup.append(bool(blocked)) - return bool(blocked) + return tables - _wait_for(settled, "the writer to commit or wait for cleanup") - - with mock.patch.object(tf, "_after_inventory", during_cleanup): + with mock.patch.object(tf, "_tables_with_pages", inventory_then_write): tf.flush_database("default") thread.join(WAIT_SECONDS) - # Without the lock the insert commits between the inventory and the - # DELETE, and its row outlives the cleanup. - assert not thread.is_alive() - assert blocked_by_cleanup[-1:] == [True] - assert committed.is_set() - assert _option_rows() == 1 + assert _scalar("SELECT count(*) FROM sentry_option") == 1 tf.flush_database("default") - assert _option_rows() == 0 + assert _scalar("SELECT count(*) FROM sentry_option") == 0 - def _flush_with_open_writer(self) -> list[str]: - cleanup_pid = _backend_pid("default") + def test_open_writer_falls_back_to_django_flush(self) -> None: + cleanup_pid = _scalar("SELECT pg_backend_pid()") inserted = threading.Event() def writer() -> None: try: with connections["default"].cursor() as cursor: cursor.execute("BEGIN") - cursor.execute(_insert_option_sql("open-writer")) + cursor.execute( + "INSERT INTO sentry_option (key, value, last_updated, last_updated_by) " + "VALUES ('open-writer', '1', now(), 'unknown')" + ) inserted.set() def truncate_waits() -> bool: @@ -252,87 +181,10 @@ def truncate_waits() -> bool: thread = threading.Thread(target=writer) thread.start() - try: - assert inserted.wait(WAIT_SECONDS) - with _count_django_flushes() as flushed: - tf.flush_database("default") - finally: - thread.join(WAIT_SECONDS) - assert not thread.is_alive() - return flushed - - def test_open_writer_falls_back_to_django_flush(self) -> None: - flushed = self._flush_with_open_writer() - - assert flushed == ["default"] - assert _option_rows() == 0 - self.assert_flushed() - - def test_lock_timeout_falls_back_to_django_flush(self) -> None: - # Skip the open-transaction checks so the writer is only noticed by - # the lock timing out. - with ( - mock.patch.object(tf, "_other_transactions_open", return_value=False), - mock.patch.object(tf, "LOCK_TIMEOUT", "50ms"), - ): - flushed = self._flush_with_open_writer() + assert inserted.wait(WAIT_SECONDS) + with _count_django_flushes() as flushed: + tf.flush_database("default") + thread.join(WAIT_SECONDS) assert flushed == ["default"] - assert _option_rows() == 0 - self.assert_flushed() - - -class _Case(DjangoTransactionTestCase): - databases = {"default"} - - def test_nothing(self) -> None: - pass - - -@pytest.mark.parametrize( - ("attribute", "value"), [("available_apps", ["sentry"]), ("serialized_rollback", True)] -) -def test_unsupported_options_use_django_teardown(attribute: str, value: object) -> None: - case = _Case("test_nothing") - setattr(case, attribute, value) - with ( - mock.patch.object(tf, "_original_fixture_teardown") as original, - mock.patch.object(tf, "flush_database") as flush_database, - ): - tf._fixture_teardown(case) - original.assert_called_once_with(case) - flush_database.assert_not_called() - - -def test_default_options_use_fast_flush() -> None: - case = _Case("test_nothing") - with ( - mock.patch.object(tf, "_original_fixture_teardown") as original, - mock.patch.object(tf, "flush_database") as flush_database, - ): - tf._fixture_teardown(case) - original.assert_not_called() - flush_database.assert_called_once_with("default") - - -def test_legacy_env_var_keeps_django_teardown(request: pytest.FixtureRequest) -> None: - original = tf._original_fixture_teardown - with mock.patch.object(DjangoTransactionTestCase, "_fixture_teardown", original): - with mock.patch.dict(os.environ, {tf.LEGACY_ENV_VAR: "1"}): - tf.pytest_configure(request.config) - assert DjangoTransactionTestCase._fixture_teardown is original # type: ignore[attr-defined] - - with mock.patch.dict(os.environ, {tf.LEGACY_ENV_VAR: ""}): - tf.pytest_configure(request.config) - assert DjangoTransactionTestCase._fixture_teardown is tf._fixture_teardown # type: ignore[attr-defined] - - -def test_unconfigure_restores_django_teardown(request: pytest.FixtureRequest) -> None: - cached = {("default", "test_region"): tf._SeededRows(frozenset(), "")} - with ( - mock.patch.object(DjangoTransactionTestCase, "_fixture_teardown", tf._fixture_teardown), - mock.patch.object(tf, "_post_migrate_rows", cached), - ): - tf.pytest_unconfigure(request.config) - assert DjangoTransactionTestCase._fixture_teardown is tf._original_fixture_teardown # type: ignore[attr-defined] - assert cached == {} + assert _scalar("SELECT count(*) FROM sentry_option") == 0