diff --git a/.github/scripts/post_profiler_comment.py b/.github/scripts/post_profiler_comment.py index 6b015f0c..5cd5551a 100644 --- a/.github/scripts/post_profiler_comment.py +++ b/.github/scripts/post_profiler_comment.py @@ -15,13 +15,37 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[2])) from eng.profiler_benchmarks import report as reporting -ROOT = Path(__file__).resolve().parents[2] ADO = "https://dev.azure.com/sqlclientdrivers/public/_apis/build" REPOSITORY = "microsoft/mssql-python" HEADER = f"{reporting.MARKER}\n## PR Performance Report\n\n" # Allow a 160-minute ADO job plus queueing; the workflow reserves publication time. WAIT_MINUTES = 220 COMPLETED_RESULTS = {"succeeded", "partiallySucceeded", "failed"} +ARTIFACT_GRACE_SECONDS = 120 + + +def pending_message(head): + return ( + HEADER + + "**Performance assessment pending.**\n\n" + + f"Waiting for the matching performance run for head `{head}`." + ) + + +def closed_message(): + return ( + HEADER + + "**Performance could not be assessed.**\n\n" + + "Pull request closed before assessment completed. No result is available." + ) + + +def superseded_message(): + return ( + HEADER + + "**Performance assessment superseded.**\n\n" + + "The pull request revision changed before publication completed." + ) def allowed_url(url): @@ -84,17 +108,19 @@ def github(path, **kwargs): def publish(pr_number, head, body, base=None): - def current(): + def current_body(): pr = github(f"pulls/{pr_number}") - return ( - pr["state"] == "open" - and pr["head"]["sha"] == head - and (base is None or pr["base"]["sha"] == base) - ) - - if not current(): - print("Not publishing stale performance results") - return + if pr["head"]["sha"] != head or (base is not None and pr["base"]["sha"] != base): + return None + if pr["state"] == "closed" and pr.get("merged") is not True: + return closed_message() + # Exact head/base identity remains stable after merge, so a run that + # started while open may replace its pending comment with a terminal one. + return body if pr["state"] == "open" or pr.get("merged") is True else None + + # A stale head/base yields no message here, but still routes through the + # comment scan so a lingering pending comment can be superseded below. + message = current_body() page = 1 comment = None while True: @@ -112,13 +138,52 @@ def current(): break page += 1 if comment: - if not current(): + message = current_body() + if message is None: + if comment["body"] == pending_message(head): + latest = github(f"issues/comments/{comment['id']}") + if isinstance(latest, dict) and latest.get("body") == comment["body"]: + # Workflow concurrency serializes publishers per PR; the + # re-read also preserves updates from people or other tools. + github( + f"issues/comments/{comment['id']}", + method="PATCH", + data={"body": superseded_message()}, + ) + return + if ( + body == pending_message(head) + and comment["body"] != body + and f"PR head: `{head}`" in comment["body"] + ): return - github(f"issues/comments/{comment['id']}", method="PATCH", data={"body": body}) + if message == closed_message(): + if comment["body"] != pending_message(head): + return + latest = github(f"issues/comments/{comment['id']}") + if not isinstance(latest, dict) or latest.get("body") != comment["body"]: + return + github(f"issues/comments/{comment['id']}", method="PATCH", data={"body": message}) + comment_id = comment["id"] else: - if not current(): + message = current_body() + if message is None: return - github(f"issues/{pr_number}/comments", method="POST", data={"body": body}) + created = github(f"issues/{pr_number}/comments", method="POST", data={"body": message}) + comment_id = created.get("id") if isinstance(created, dict) else None + verified = current_body() + if comment_id is None: + return + if verified is not None and verified != message: + github(f"issues/comments/{comment_id}", method="PATCH", data={"body": verified}) + elif verified is None: + latest = github(f"issues/comments/{comment_id}") + if isinstance(latest, dict) and latest.get("body") == message: + github( + f"issues/comments/{comment_id}", + method="PATCH", + data={"body": (superseded_message())}, + ) def publish_with_retry(pr_number, head, body, base=None, attempts=3): @@ -191,18 +256,21 @@ def unavailable(number, head, reason, base=None): def run(number, head, wait_minutes): - publish_with_retry( - number, - head, - HEADER - + "**Performance assessment pending.**\n\n" - + f"Waiting for the matching performance run for head `{head}`.", - ) + publish_with_retry(number, head, pending_message(head)) deadline = time.monotonic() + wait_minutes * 60 build = None + artifacts = None pr_base = None + completed_at = None + selected_build_id = None + build_resumed = False + assessment_ready = False failures = 0 - while time.monotonic() < deadline: + while time.monotonic() < ( + max(deadline, completed_at + ARTIFACT_GRACE_SECONDS) + if completed_at is not None + else deadline + ): try: pr = github(f"pulls/{number}") if ( @@ -215,6 +283,18 @@ def run(number, head, wait_minutes): current_head = pr["head"].get("sha") current_base = pr["base"].get("sha") pr_base = current_base + if current_head != head: + # Supersede the pending comment through the compare-and-update + # path instead of leaving it posted for the stale head. + publish_with_retry(number, head, superseded_message()) + return + if pr["state"] == "closed" and pr.get("merged") is not True: + unavailable( + number, head, "Pull request closed before assessment completed.", pr_base + ) + return + if pr["state"] != "open" and pr.get("merged") is not True: + raise ValueError query = urlencode( { "definitions": 2128, @@ -225,13 +305,33 @@ def run(number, head, wait_minutes): } ) build = find_build(build_items(api(f"{ADO}/builds?{query}")), number, head) - if pr["state"] != "open" or current_head != head: - return - if ( - build is not None - and build.get("status") == "completed" - and build.get("result") not in COMPLETED_RESULTS | {"canceled"} - ): + if build is None: + failures = 0 + time.sleep(30) + continue + build_id = build["id"] + if selected_build_id != build_id: + selected_build_id = build_id + artifacts = None + completed_at = None + build_resumed = False + status = build.get("status") + result = build.get("result") + if status == "cancelling": + failures = 0 + artifacts = None + completed_at = None + build_resumed = True + time.sleep(30) + continue + if status == "completed" and result == "canceled": + failures = 0 + artifacts = None + completed_at = None + build_resumed = True + time.sleep(30) + continue + if status == "completed" and result not in COMPLETED_RESULTS: unavailable( number, head, @@ -239,35 +339,71 @@ def run(number, head, wait_minutes): pr_base, ) return - complete = ( - build is not None - and build.get("status") == "completed" - and build.get("result") in COMPLETED_RESULTS - ) + if status != "completed": + if completed_at is not None: + build_resumed = True + completed_at = None + if status == "completed" and completed_at is None: + completed_at = time.monotonic() + artifacts = artifact_items(api(f"{ADO}/builds/{build_id}/artifacts?api-version=7.1")) + failures = 0 + required = {"profiler-" + leg for leg in reporting.LEGS} + usable = { + item["name"] + for item in artifacts + if isinstance(item["resource"].get("downloadUrl"), str) + and item["resource"]["downloadUrl"] + } + # Artifact readiness is the report signal; unrelated matrix legs do + # not need to finish before the required profiler legs are assessed. + if required <= usable and (status == "completed" or not build_resumed): + assessment_ready = True + break + if ( + completed_at is not None + and time.monotonic() - completed_at >= ARTIFACT_GRACE_SECONDS + ): + assessment_ready = True + break except (ValueError, KeyError, TypeError, URLError, TimeoutError): failures += 1 + if ( + artifacts is not None + and completed_at is not None + and time.monotonic() - completed_at >= ARTIFACT_GRACE_SECONDS + ): + assessment_ready = True + break if failures >= 5: unavailable(number, head, "Performance data services failed repeatedly.", pr_base) return time.sleep(30) continue failures = 0 - if complete: - break time.sleep(30) if ( - build is None - or build.get("status") != "completed" - or build.get("result") not in COMPLETED_RESULTS + not assessment_ready + and completed_at is not None + and time.monotonic() >= completed_at + ARTIFACT_GRACE_SECONDS ): + assessment_ready = True + if build is None: unavailable( number, head, - f"No matching performance run completed within the {wait_minutes}-minute wait " + f"No matching performance run appeared within the {wait_minutes}-minute wait " f"for `{head}`.", pr_base, ) return + if not assessment_ready: + unavailable( + number, + head, + f"Performance artifacts did not become ready within the {wait_minutes}-minute wait.", + pr_base, + ) + return build_id = build["id"] source = build.get("sourceVersion") try: @@ -283,37 +419,9 @@ def run(number, head, wait_minutes): base_commit = github(f"git/commits/{base}") if not isinstance(commit, dict) or not isinstance(base_commit, dict): raise ValueError - source_tree_info = commit.get("tree") - base_tree_info = base_commit.get("tree") - if not isinstance(source_tree_info, dict) or not isinstance(base_tree_info, dict): - raise ValueError - source_tree_sha = source_tree_info.get("sha") - base_tree_sha = base_tree_info.get("sha") - if not re.fullmatch(r"[0-9a-f]{40}", source_tree_sha or "") or not re.fullmatch( - r"[0-9a-f]{40}", base_tree_sha or "" - ): - raise ValueError - source_tree = github(f"git/trees/{source_tree_sha}?recursive=1") - base_tree = github(f"git/trees/{base_tree_sha}?recursive=1") except (ValueError, KeyError, TypeError, URLError, TimeoutError): unavailable(number, head, "Build provenance validation failed.", pr_base) return - artifacts = None - failures = 0 - while time.monotonic() < deadline: - try: - artifacts = artifact_items(api(f"{ADO}/builds/{build_id}/artifacts?api-version=7.1")) - failures = 0 - if {"profiler-" + leg for leg in reporting.LEGS} <= { - item["name"] for item in artifacts - }: - break - except (ValueError, KeyError, TypeError, URLError, TimeoutError): - failures += 1 - if failures >= 5: - artifacts = None - break - time.sleep(30) if artifacts is None: unavailable(number, head, "Performance artifacts remained unavailable.", pr_base) return @@ -324,7 +432,7 @@ def run(number, head, wait_minutes): issues.append(leg + " (missing)") continue url = matching[0]["resource"].get("downloadUrl") - if not isinstance(url, str): + if not isinstance(url, str) or not url: issues.append(leg + " (invalid artifact)") continue artifact_urls[leg] = url @@ -341,9 +449,6 @@ def load_artifact(url): base=base, merge_commit=commit, base_commit=base_commit, - source_tree=source_tree, - base_tree=base_tree, - trusted_root=ROOT, ) publish_with_retry( number, head, reporting.assess(evidence, artifact_urls, load_artifact, issues), base diff --git a/eng/pipelines/pr-validation-pipeline.yml b/eng/pipelines/pr-validation-pipeline.yml index 85e3e2ae..1d5b9aa5 100644 --- a/eng/pipelines/pr-validation-pipeline.yml +++ b/eng/pipelines/pr-validation-pipeline.yml @@ -242,7 +242,9 @@ jobs: cd mssql_python\pybind build.bat x64 displayName: 'Build profiling .pyd file' - condition: and(succeeded(), eq(variables['Build.Reason'], 'PullRequest'), ne(variables['sqlVersion'], 'LocalDB')) + # Hosted Windows timings varied more than the regression threshold on neutral + # PRs. Keep functional Windows CI, but exclude it from routine PR profiling. + condition: false env: ENABLE_PROFILING: 1 @@ -250,11 +252,11 @@ jobs: cd mssql_python\pybind build.bat x64 displayName: 'Build .pyd file' - condition: and(succeeded(), or(ne(variables['Build.Reason'], 'PullRequest'), eq(variables['sqlVersion'], 'LocalDB'))) + condition: succeeded() - script: python -m eng.profiler_benchmarks.controller --check-build on displayName: 'Verify native configuration and recording OFF before pytest' - condition: and(succeeded(), eq(variables['Build.Reason'], 'PullRequest'), ne(variables['sqlVersion'], 'LocalDB')) + condition: false - template: steps/install-mssql-py-core.yml parameters: @@ -318,7 +320,7 @@ jobs: exit 1 } displayName: 'Download and restore AdventureWorks2022 database' - condition: and(succeeded(), or(eq(variables['sqlVersion'], 'SQL2022'), eq(variables['sqlVersion'], 'SQL2025'))) + condition: false env: DB_PASSWORD: $(DB_PASSWORD) @@ -408,7 +410,7 @@ jobs: python -m eng.profiler_benchmarks.controller --reuse-candidate --leg "Windows-$(sqlVersion)" --output profiler-results if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } displayName: 'Compare profiling builds on SQL Server 2022/2025' - condition: and(succeeded(), eq(variables['Build.Reason'], 'PullRequest'), or(eq(variables['sqlVersion'], 'SQL2022'), eq(variables['sqlVersion'], 'SQL2025'))) + condition: false continueOnError: true timeoutInMinutes: 100 env: @@ -421,7 +423,7 @@ jobs: targetPath: profiler-results artifact: 'profiler-Windows-$(sqlVersion)' displayName: 'Publish paired profiler measurements' - condition: and(succeededOrFailed(), eq(variables['Build.Reason'], 'PullRequest'), or(eq(variables['sqlVersion'], 'SQL2022'), eq(variables['sqlVersion'], 'SQL2025'))) + condition: false continueOnError: true - task: CopyFiles@2 @@ -444,7 +446,7 @@ jobs: ArtifactName: 'ddbc_bindings-profiling-$(sqlVersion)' publishLocation: 'Container' displayName: 'Publish profiling build artifacts' - condition: and(succeeded(), eq(variables['Build.Reason'], 'PullRequest'), ne(variables['sqlVersion'], 'LocalDB')) + condition: false - task: PublishBuildArtifacts@1 inputs: @@ -452,7 +454,7 @@ jobs: ArtifactName: 'ddbc_bindings' publishLocation: 'Container' displayName: 'Publish build artifacts' - condition: and(succeeded(), or(ne(variables['Build.Reason'], 'PullRequest'), eq(variables['sqlVersion'], 'LocalDB'))) + condition: succeeded() - task: PublishTestResults@2 condition: succeededOrFailed() diff --git a/eng/profiler_benchmarks/README.md b/eng/profiler_benchmarks/README.md index 8578d690..8192be78 100644 --- a/eng/profiler_benchmarks/README.md +++ b/eng/profiler_benchmarks/README.md @@ -27,14 +27,20 @@ Partial results never produce a verdict. ## Publication -Four environments publish raw samples: Windows and Unix on SQL Server 2022/2025. -Unix measurements run on Ubuntu. Routine macOS profiling is intentionally excluded: -hosted macOS plus Colima produced false regressions on a documentation-only control -PR, while macOS remains covered by functional CI. The privileged publisher runs -trusted base code, authenticates benchmark producers, validates bounded artifacts, -and ignores stale heads. A failed aggregate build can still publish when its -authenticated artifacts validate. Missing, malformed, canceled, incomplete, or -invalid data remains unavailable. +Two environments publish raw samples: Unix on Ubuntu with SQL Server 2022/2025. +Routine Windows and macOS profiling is intentionally excluded because neutral PRs +showed platform variance above the regression threshold, while both platforms +remain covered by functional CI. The privileged publisher runs +trusted base code, selects the exact PR-head ADO build, and validates bounded +artifacts as data. It publishes as soon as both profiler artifacts exist, +without waiting for unrelated matrix legs. After build completion, missing +artifacts receive a two-minute propagation grace before a partial result is +published. A failed aggregate build can still publish usable profiler artifacts. +Exact-head reports may finalize after merge; stale heads are ignored. Missing, +malformed, canceled, incomplete, or invalid data remains unavailable. + +The report highlights consistent slowdowns and improvements using the same 20% +median change, 1 ms absolute change, and 80% pair-agreement requirements. The publisher waits up to 220 minutes inside a 230-minute workflow. The first main comparison after introduction may be incomplete because its parent lacks this diff --git a/eng/profiler_benchmarks/controller.py b/eng/profiler_benchmarks/controller.py index 97376d1c..143164e2 100644 --- a/eng/profiler_benchmarks/controller.py +++ b/eng/profiler_benchmarks/controller.py @@ -17,7 +17,7 @@ import tempfile import time -from .report import LEGS, suite_hash +from .report import LEGS from . import workloads ROOT = Path(__file__).resolve().parents[2] @@ -241,7 +241,6 @@ def run(args): source_commit=candidate, head_commit=head, build_id=int(os.environ.get("BUILD_BUILDID", "0")), - suite_hash=suite_hash(ROOT), samples=args.samples, warmups=args.warmups, pairs=[], diff --git a/eng/profiler_benchmarks/report.py b/eng/profiler_benchmarks/report.py index 7d7aa217..cfe19bb8 100644 --- a/eng/profiler_benchmarks/report.py +++ b/eng/profiler_benchmarks/report.py @@ -2,7 +2,6 @@ import argparse from dataclasses import dataclass -import hashlib import html import io import json @@ -16,7 +15,7 @@ # Hosted macOS plus Colima produced false regressions on a documentation-only # control PR. Routine reports use stable Ubuntu measurements as the Unix signal. -LEGS = ("Windows-SQL2022", "Windows-SQL2025", "Linux-SQL2022", "Linux-SQL2025") +LEGS = ("Linux-SQL2022", "Linux-SQL2025") TASK_NAMES = { "connect": "Connection opening", "select": "SELECT queries", @@ -48,28 +47,6 @@ MIN_DELTA_MS = 1.0 -def suite_paths(root): - root = Path(root) - return [ - root / "eng/pipelines/pr-validation-pipeline.yml", - root / "eng/profiler_benchmarks/__init__.py", - root / "eng/profiler_benchmarks/controller.py", - root / "eng/profiler_benchmarks/report.py", - root / "eng/profiler_benchmarks/workloads.py", - root / "eng/scripts/setup_sql_container.py", - root / "requirements.txt", - *sorted((root / "profiler").glob("*.py")), - ] - - -def suite_hash(root): - digest = hashlib.sha256() - for file in suite_paths(root): - digest.update(file.name.encode()) - digest.update(file.read_bytes().replace(b"\r\n", b"\n")) - return digest.hexdigest() - - @dataclass(frozen=True) class AssessmentEvidence: build: dict @@ -77,9 +54,6 @@ class AssessmentEvidence: base: str merge_commit: dict base_commit: dict - source_tree: dict - base_tree: dict - trusted_root: Path def artifact_report(raw): @@ -108,27 +82,6 @@ def artifact_report(raw): return json.loads(archive.read(member).decode("utf-8")) -def suite_blobs(tree, root): - if ( - not isinstance(tree, dict) - or tree.get("truncated") is not False - or not isinstance(tree.get("tree"), list) - or not all(isinstance(entry, dict) for entry in tree["tree"]) - ): - raise ValueError("Incomplete commit tree") - expected = {path.relative_to(root).as_posix() for path in suite_paths(root)} - blobs = { - entry.get("path"): entry.get("sha") - for entry in tree["tree"] - if entry.get("type") == "blob" and entry.get("path") in expected - } - if set(blobs) != expected or any( - not re.fullmatch(r"[0-9a-f]{40}", sha or "") for sha in blobs.values() - ): - raise ValueError("Benchmark suite missing from commit tree") - return blobs - - def unavailable(reason): return ( f"{MARKER}\n## PR Performance Report\n\n" @@ -150,14 +103,14 @@ def text(value, limit=160): return value -def validate(report, build_id=None, head=None, source=None, base=None, suite=None): +def validate(report, build_id=None, head=None, source=None, base=None): try: - return _validate(report, build_id, head, source, base, suite) + return _validate(report, build_id, head, source, base) except KeyError as error: raise ValueError(f"Missing performance report field: {error.args[0]}") from error -def _validate(report, build_id=None, head=None, source=None, base=None, suite=None): +def _validate(report, build_id=None, head=None, source=None, base=None): if not isinstance(report, dict) or report.get("schema_version") != 1: raise ValueError("Unsupported report schema") if report.get("leg") not in LEGS or report.get("status") not in ("complete", "incomplete"): @@ -169,15 +122,12 @@ def _validate(report, build_id=None, head=None, source=None, base=None, suite=No ("head_commit", head), ("source_commit", source), ("base_commit", base), - ("suite_hash", suite), ): if expected is not None and report.get(key) != expected: raise ValueError(f"Report provenance mismatch: {key}") for key in ("head_commit", "source_commit", "base_commit"): if not re.fullmatch(r"[0-9a-f]{40}", report.get(key, "")): raise ValueError("Invalid commit identity") - if not re.fullmatch(r"[0-9a-f]{64}", report.get("suite_hash", "")): - raise ValueError("Invalid workload identity") samples = report.get("samples") if type(samples) is not int or not 3 <= samples <= 15: raise ValueError("Insufficient or excessive samples") @@ -263,8 +213,6 @@ def assess(evidence, artifact_urls, load_artifact, issues=()): not isinstance(evidence.build, dict) or not isinstance(evidence.merge_commit, dict) or not isinstance(evidence.base_commit, dict) - or not isinstance(evidence.source_tree, dict) - or not isinstance(evidence.base_tree, dict) ): raise ValueError build_id = evidence.build.get("id") @@ -281,26 +229,11 @@ def assess(evidence, artifact_urls, load_artifact, issues=()): != [evidence.base, evidence.head] ): raise ValueError - source_tree_sha = evidence.merge_commit["tree"]["sha"] - base_tree_sha = evidence.base_commit["tree"]["sha"] - if ( - not re.fullmatch(r"[0-9a-f]{40}", source_tree_sha) - or not re.fullmatch(r"[0-9a-f]{40}", base_tree_sha) - or evidence.source_tree.get("sha") != source_tree_sha - or evidence.base_tree.get("sha") != base_tree_sha - ): - raise ValueError except (KeyError, TypeError, ValueError): return unavailable("Build provenance validation failed.") - try: - suite_unchanged = suite_blobs(evidence.source_tree, evidence.trusted_root) == suite_blobs( - evidence.base_tree, evidence.trusted_root - ) - trusted_suite = suite_hash(evidence.trusted_root) - except (KeyError, TypeError, ValueError): - return unavailable("Benchmark suite validation failed because a required file changed.") - + # Match coverage's trust boundary: select the exact PR-head build and treat + # its bounded artifacts as data without requiring an identical producer tree. reports = [] for leg, url in artifact_urls.items(): try: @@ -324,9 +257,6 @@ def assess(evidence, artifact_urls, load_artifact, issues=()): ): issues.append(leg + " (invalid artifact)") - if not suite_unchanged or any(report["suite_hash"] != trusted_suite for report in reports): - reports = [] - issues.append("workload version differs from trusted base") try: return render(reports, evidence.head, build_id, issues) except ValueError: @@ -345,12 +275,17 @@ def comparisons(report): ratio = statistics.median(ratios) # Requiring 80% of paired samples to agree avoids flagging one noisy pass. agrees = sum(r > 1 + THRESHOLD for r in ratios) >= math.ceil(len(ratios) * 0.8) + improves = sum(r < 1 - THRESHOLD for r in ratios) >= math.ceil(len(ratios) * 0.8) status = ( "regression" if ratio > 1 + THRESHOLD and new - old >= MIN_DELTA_MS and agrees - else ("noisy" if ratio > 1 + THRESHOLD and new - old >= MIN_DELTA_MS else "ok") + else ( + "improvement" + if ratio < 1 - THRESHOLD and old - new >= MIN_DELTA_MS and improves + else ("noisy" if ratio > 1 + THRESHOLD and new - old >= MIN_DELTA_MS else "ok") + ) ) - phases = [] + phase_deltas = [] changed_counts = [] for layer in ("cpp", "py"): labels = set().union(*(s[layer] for s in base + candidate)) @@ -368,8 +303,13 @@ def comparisons(report): statistics.median(s["total_us"] for s in after) - statistics.median(s["total_us"] for s in before) ) / 1000 - if delta > 0: - phases.append((delta, label)) + if delta: + phase_deltas.append((delta, label)) + phases = ( + sorted((item for item in phase_deltas if item[0] < 0))[:3] + if status == "improvement" + else sorted((item for item in phase_deltas if item[0] > 0), reverse=True)[:3] + ) output.append( dict( name=name, @@ -377,7 +317,7 @@ def comparisons(report): candidate_ms=new, change_pct=(ratio - 1) * 100, status=status, - phases=sorted(phases, reverse=True)[:3], + phases=phases, counts=sorted(changed_counts)[:3], ) ) @@ -422,6 +362,12 @@ def render(reports, head, build_id, issues=()): for row in rows if row["status"] == "regression" ] + improvements = [ + (leg, row) + for leg, (_, rows) in completed.items() + for row in rows + if row["status"] == "improvement" + ] noisy = [ (leg, row) for leg, (_, rows) in completed.items() @@ -457,6 +403,19 @@ def render(reports, head, build_id, issues=()): f"No consistent slowdowns detected. {len(noisy)} inconsistent comparisons " f"need review across {tasks} database tasks and {environments} environments." ) + elif len(improvements) == 1: + leg, row = improvements[0] + opening = ( + f"This PR consistently makes {TASK_NAMES[row['name']].lower()} faster on " + f"{environment_name(leg)} by {abs(row['change_pct']):.1f}%." + ) + elif improvements: + tasks = len({row["name"] for _, row in improvements}) + environments = len({leg for leg, _ in improvements}) + opening = ( + f"This PR has {len(improvements)} consistent improvement signals across " + f"{tasks} database tasks and {environments} environments." + ) elif not completed: opening = ( "Performance could not be assessed because no environment produced a complete result." @@ -472,9 +431,9 @@ def render(reports, head, build_id, issues=()): ) lines = [MARKER, "## PR Performance Report", "", f"**{opening}**", ""] - highlighted = regressions or noisy + highlighted = regressions or noisy or improvements if highlighted: - if not regressions: + if not regressions and noisy: lines += ["Inconsistent slowdowns to review:", ""] lines += [ "| Environment | Affected task | Before | After | Change |", @@ -535,9 +494,9 @@ def render(reports, head, build_id, issues=()): lines += ["", f"### {environment_name(leg)}"] for row in visible: diagnostics += 1 - phases = "; ".join(f"{escape(label)} +{delta:.3f} ms" for delta, label in row["phases"]) + phases = "; ".join(f"{escape(label)} {delta:+.3f} ms" for delta, label in row["phases"]) counts = "; ".join(escape(label) for label in row["counts"]) - detail = phases or "no positive phase delta" + detail = phases or "no measured phase delta" if counts: detail += f". Call changes: {counts}" lines.append(f"**{TASK_NAMES[row['name']]}:** {detail}.") @@ -570,6 +529,7 @@ def render(reports, head, build_id, issues=()): for row in rows: result = { "regression": "consistent slowdown", + "improvement": "consistent improvement", "noisy": "inconsistent slowdown", "ok": "no signal", }[row["status"]] @@ -606,10 +566,10 @@ def render(reports, head, build_id, issues=()): ) lines += [ "", - "A consistent slowdown requires more than 20% median paired slowdown, at least " + "A consistent change requires more than 20% median paired movement, at least " "1 ms between the median runtimes, and at least 80% of pairs exceeding the " - "relative threshold. An inconsistent slowdown crosses the first two thresholds " - "without enough pair agreement.", + "relative threshold in the same direction. A slowdown without enough pair " + "agreement is reported as inconsistent.", "", "The displayed change is the median of paired before-and-after ratios. It is not " "recalculated from the two displayed median runtimes.", @@ -656,7 +616,6 @@ def main(): head=first["head_commit"], source=first["source_commit"], base=first["base_commit"], - suite=first["suite_hash"], ) print(render(reports, first["head_commit"], first["build_id"])) diff --git a/tests/test_036_profiler_ci.py b/tests/test_036_profiler_ci.py index 1d8c04b4..e72ad2c2 100644 --- a/tests/test_036_profiler_ci.py +++ b/tests/test_036_profiler_ci.py @@ -56,10 +56,15 @@ def ado_build(**values): return build -def pr_topology(head="c" * 40, base="a" * 40, merge_base=None): +def pr_topology(head="c" * 40, base="a" * 40, merge_base=None, state="open", merged=False): def response(path): if path.startswith("pulls/"): - return {"state": "open", "head": {"sha": head}, "base": {"sha": base}} + return { + "state": state, + "merged": merged, + "head": {"sha": head}, + "base": {"sha": base}, + } if path.startswith("git/commits/"): commit_sha = path.removeprefix("git/commits/") source = commit_sha == "b" * 40 @@ -68,20 +73,6 @@ def response(path): "parents": [{"sha": merge_base or base}, {"sha": head}] if source else [], "tree": {"sha": ("d" if source else "e") * 40}, } - if path.startswith("git/trees/"): - tree_sha = path.removeprefix("git/trees/").split("?", 1)[0] - return { - "sha": tree_sha, - "truncated": False, - "tree": [ - { - "path": file.relative_to(ROOT).as_posix(), - "type": "blob", - "sha": f"{index + 1:040x}", - } - for index, file in enumerate(reporting.suite_paths(ROOT)) - ], - } raise AssertionError(f"Unexpected GitHub path: {path}") return response @@ -110,7 +101,6 @@ def sample(scale): base_commit="a" * 40, source_commit="b" * 40, head_commit="c" * 40, - suite_hash="d" * 64, build_id=42, samples=5, warmups=1, @@ -127,7 +117,7 @@ def test_consistent_slowdown_is_advisory_regression(report): body = reporting.render([report], "c" * 40, 42) assert "20 consistent slowdown signals" in body assert "| Unix / SQL Server 2022 | Connection opening |" in body - assert "| Windows / SQL Server 2022 | No result available" in body + assert "| Unix / SQL Server 2025 | No result available" in body assert body.index("consistent slowdown signals") < body.index( "Build, commits and measurement details" ) @@ -224,14 +214,13 @@ def set_leg(report, leg): ("head_commit", "e" * 40), ("source_commit", "e" * 40), ("base_commit", "e" * 40), - ("suite_hash", "e" * 64), ], ) def test_standalone_report_rejects_mixed_provenance(report, tmp_path, monkeypatch, key, value): first = tmp_path / "linux.json" - second = tmp_path / "windows.json" + second = tmp_path / "linux-2025.json" first.write_text(json.dumps(report), encoding="utf-8") - other = set_leg(report, "Windows-SQL2022") + other = set_leg(report, "Linux-SQL2025") other[key] = value second.write_text(json.dumps(other), encoding="utf-8") monkeypatch.setattr(sys, "argv", ["report", str(first), str(second)]) @@ -258,12 +247,12 @@ def test_render_bounds_schema_valid_diagnostics(report): reporting.validate(item) body = reporting.render(reports, "c" * 40, 42) assert len(body) <= 60000 - assert "80 diagnostic rows are available in the raw ADO artifacts" in body + assert "20 additional diagnostic rows are available in the raw ADO artifacts" in body assert "All database tasks and timings" in body assert "Build, commits and measurement details" in body -@pytest.mark.parametrize("invalid", ["source commit", "base commit", "source tree"]) +@pytest.mark.parametrize("invalid", ["source commit", "base commit"]) def test_assessment_binds_all_evidence_to_authenticated_commits(invalid): evidence = reporting.AssessmentEvidence( build=ado_build(), @@ -275,16 +264,11 @@ def test_assessment_binds_all_evidence_to_authenticated_commits(invalid): "tree": {"sha": "d" * 40}, }, base_commit={"sha": "a" * 40, "tree": {"sha": "e" * 40}}, - source_tree={"sha": "d" * 40, "truncated": False, "tree": []}, - base_tree={"sha": "e" * 40, "truncated": False, "tree": []}, - trusted_root=ROOT, ) if invalid == "source commit": evidence.merge_commit["sha"] = "f" * 40 elif invalid == "base commit": evidence.base_commit["sha"] = "f" * 40 - else: - evidence = reporting.AssessmentEvidence(**{**evidence.__dict__, "source_tree": []}) body = reporting.assess(evidence, {}, lambda url: pytest.fail("must not download")) assert "Performance could not be assessed" in body assert "Build provenance validation failed" in body @@ -312,8 +296,8 @@ def test_impact_summary_handles_single_inconsistent_and_complete_clean_results(r complete = [set_leg(clear_slowdowns(copy.deepcopy(report)), leg) for leg in reporting.LEGS] clean_body = reporting.render(complete, "c" * 40, 42) - assert "**No consistent slowdowns detected across all 4 environments.**" in clean_body - assert "**Coverage:** 4 of 4 environments completed." in clean_body + assert "**No consistent slowdowns detected across all 2 environments.**" in clean_body + assert "**Coverage:** 2 of 2 environments completed." in clean_body def test_impact_summary_handles_single_regression_partial_and_no_results(report): @@ -333,11 +317,11 @@ def test_impact_summary_handles_single_regression_partial_and_no_results(report) [clear_slowdowns(copy.deepcopy(report))], "c" * 40, 42, - ["Windows-SQL2022 (missing)"], + ["Linux-SQL2025 (missing)"], ) assert "No consistent slowdowns in the 1 completed environment." in partial - assert "No result is available for 3 environments." in partial - assert "| Windows / SQL Server 2022 | No result available (missing) |" in partial + assert "No result is available for 1 environment." in partial + assert "| Unix / SQL Server 2025 | No result available (missing) |" in partial assert "pending" not in partial.lower() unavailable = reporting.render([], "c" * 40, 42, ["Linux-SQL2022 (invalid artifact)"]) @@ -345,6 +329,46 @@ def test_impact_summary_handles_single_regression_partial_and_no_results(report) assert "No consistent slowdowns" not in unavailable +def test_impact_summary_reports_consistent_improvements(report): + single = clear_slowdowns(copy.deepcopy(report)) + for pair in single["pairs"]: + pair["candidate"]["scenarios"]["fetchall"]["wall_ms"] *= 0.7 + pair["candidate"]["scenarios"]["fetchall"]["cpp"]["ddbc::query"] = dict( + calls=1, total_us=500, min_us=500, max_us=500 + ) + rows = reporting.comparisons(single) + assert rows[4]["status"] == "improvement" + assert rows[4]["phases"] == [(-0.5, "ddbc::query")] + body = reporting.render([single], "c" * 40, 42) + assert ( + "**This PR consistently makes fetch-all queries faster on Unix / SQL Server 2022 " + "by 30.0%.**" + ) in body + assert "| Unix / SQL Server 2022 | Fetch-all queries |" in body + assert "| Fetch-all queries |" in body and "| consistent improvement |" in body + assert "ddbc::query -0.500 ms" in body + + +def test_regression_headline_keeps_precedence_over_improvement(report): + mixed = clear_slowdowns(copy.deepcopy(report)) + for pair in mixed["pairs"]: + pair["candidate"]["scenarios"]["fetchall"]["wall_ms"] *= 0.7 + pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= 1.3 + body = reporting.render([mixed], "c" * 40, 42) + assert "**This PR consistently slows row-by-row fetching" in body + + +def test_inconsistent_slowdown_keeps_precedence_over_improvement(report): + mixed = clear_slowdowns(copy.deepcopy(report)) + for pair in mixed["pairs"]: + pair["candidate"]["scenarios"]["fetchall"]["wall_ms"] *= 0.7 + for pair, scale in zip(mixed["pairs"], (1.3, 1.3, 1.3, 0.8, 0.8)): + pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= scale + body = reporting.render([mixed], "c" * 40, 42) + assert "**Row-by-row fetching was slower" in body + assert "Inconsistent slowdowns to review:" in body + + @pytest.mark.parametrize( "path", [ @@ -493,11 +517,106 @@ def test_publisher_does_not_post_stale_head(monkeypatch): def api(path, **kwargs): calls.append((path, kwargs)) - return {"state": "open", "head": {"sha": "new-head"}} + if path.startswith("pulls/"): + return {"state": "open", "head": {"sha": "new-head"}} + return [] monkeypatch.setattr(publisher, "github", api) publisher.publish(123, "old-head", "anything") - assert len(calls) == 1 and calls[0][1] == {} + assert all(not kwargs for _, kwargs in calls) + assert not any(path == "issues/123/comments" and kwargs for path, kwargs in calls) + + +def test_publisher_can_finalize_exact_head_after_merge(monkeypatch): + calls = [] + + def api(path, **kwargs): + calls.append((path, kwargs)) + if path.startswith("pulls/"): + return { + "state": "closed", + "merged": True, + "head": {"sha": "head"}, + "base": {"sha": "base"}, + } + if path.startswith("issues/") and "comments" in path: + return [ + { + "id": 42, + "user": {"login": "github-actions[bot]"}, + "body": publisher.pending_message("head"), + } + ] + return {} + + monkeypatch.setattr(publisher, "github", api) + publisher.publish(123, "head", "final", "base") + assert ("issues/comments/42", {"method": "PATCH", "data": {"body": "final"}}) in calls + + +def test_pending_rerun_preserves_completed_report_for_same_head(monkeypatch): + calls = [] + completed = reporting.MARKER + "\nfinal\n\nPR head: `head`" + + def api(path, **kwargs): + calls.append((path, kwargs)) + if path.startswith("pulls/"): + return { + "state": "open", + "head": {"sha": "head"}, + "base": {"sha": "base"}, + } + if path.startswith("issues/") and "comments" in path: + return [ + { + "id": 42, + "user": {"login": "github-actions[bot]"}, + "body": completed, + } + ] + return {} + + monkeypatch.setattr(publisher, "github", api) + publisher.publish(1, "head", publisher.pending_message("head"), "base") + assert not any(kwargs for path, kwargs in calls if path == "issues/comments/42") + + +def test_publisher_finalizes_pending_comment_when_pr_is_abandoned(monkeypatch): + posted = [] + monkeypatch.setattr( + publisher, "publish", lambda number, head, body, base=None: posted.append(body) + ) + monkeypatch.setattr(publisher, "github", pr_topology(state="closed")) + publisher.run(123, "c" * 40, 1) + assert len(posted) == 2 + assert "Pull request closed before assessment completed" in posted[-1] + + +def test_publisher_preserves_completed_report_when_pr_is_abandoned(monkeypatch): + calls = [] + + def api(path, **kwargs): + calls.append((path, kwargs)) + if path.startswith("pulls/"): + return { + "state": "closed", + "merged": False, + "head": {"sha": "head"}, + "base": {"sha": "base"}, + } + if path.startswith("issues/") and "comments" in path: + return [ + { + "id": 42, + "user": {"login": "github-actions[bot]"}, + "body": "final report", + } + ] + return {} + + monkeypatch.setattr(publisher, "github", api) + publisher.publish(1, "head", "new report", "base") + assert not any(kwargs for path, kwargs in calls if path == "issues/comments/42") def test_publisher_retries_transient_comment_failures(monkeypatch): @@ -553,11 +672,6 @@ def __exit__(self, *args): def test_report_cases_match_the_executed_workload_registry(): _, workloads = controller.load_suite() assert tuple(workloads.registry()) == reporting.CASES - assert ROOT / "eng/profiler_benchmarks/__init__.py" in reporting.suite_paths(ROOT) - assert ROOT / "eng/profiler_benchmarks/report.py" in reporting.suite_paths(ROOT) - assert ROOT / "eng/pipelines/pr-validation-pipeline.yml" in reporting.suite_paths(ROOT) - assert ROOT / "eng/scripts/setup_sql_container.py" in reporting.suite_paths(ROOT) - assert ROOT / "requirements.txt" in reporting.suite_paths(ROOT) def test_query_workload_executes_and_collects(monkeypatch): @@ -606,52 +720,6 @@ def test_legacy_insert_workload_executes_both_variants(input_sizes): context.disable.assert_called_once() -def test_suite_blobs_require_complete_authenticated_tree(): - expected = [path.relative_to(ROOT).as_posix() for path in reporting.suite_paths(ROOT)] - tree = { - "truncated": False, - "tree": [ - {"path": path, "type": "blob", "sha": f"{index + 1:040x}"} - for index, path in enumerate(expected) - ], - } - assert set(reporting.suite_blobs(tree, ROOT)) == set(expected) - tree["tree"].pop() - with pytest.raises(ValueError, match="missing"): - reporting.suite_blobs(tree, ROOT) - tree["tree"].append(None) - with pytest.raises(ValueError, match="Incomplete"): - reporting.suite_blobs(tree, ROOT) - - -def test_publisher_finishes_unavailable_when_checked_suite_file_moves(monkeypatch): - posted = [] - build = ado_build() - monkeypatch.setattr( - publisher, "publish", lambda number, head, body, base=None: posted.append(body) - ) - monkeypatch.setattr(publisher, "github", pr_topology()) - artifacts = [ - {"name": "profiler-" + leg, "resource": {"downloadUrl": "https://dev.azure.com/" + leg}} - for leg in reporting.LEGS - ] - monkeypatch.setattr( - publisher, - "api", - lambda url: {"value": artifacts if "/artifacts?" in url else [build]}, - ) - monkeypatch.setattr( - reporting, - "suite_blobs", - MagicMock(side_effect=ValueError("Benchmark suite missing from commit tree")), - ) - publisher.run(123, "c" * 40, 1) - assert len(posted) == 2 - assert "Performance assessment pending" in posted[0] - assert "Performance could not be assessed" in posted[1] - assert "required file changed" in posted[1] - - @pytest.mark.parametrize("fail", [False, True]) def test_worker_checkpoints_completed_and_active_scenarios(tmp_path, monkeypatch, capsys, fail): output = tmp_path / "base-0.json" @@ -841,7 +909,7 @@ def measure(path, output, scenarios, timeout): def test_ci_deadlines_include_setup_queueing_and_publication(): pipeline = (ROOT / "eng/pipelines/pr-validation-pipeline.yml").read_text(encoding="utf-8") - for job in ("pytestonwindows", "PytestOnLinux"): + for job in ("PytestOnLinux",): section = pipeline.split(f"- job: {job}\n", 1)[1].split("\n- job:", 1)[0] job_minutes = int(re.search(r"^ timeoutInMinutes: (\d+)$", section, re.M)[1]) benchmark_step = section.split( @@ -908,6 +976,40 @@ def api(path, **kwargs): assert reads == 2 and len(calls) == 3 +def test_head_moving_before_write_supersedes_unchanged_pending_comment(monkeypatch): + calls = [] + reads = 0 + + def api(path, **kwargs): + nonlocal reads + calls.append((path, kwargs)) + if path.startswith("pulls/"): + reads += 1 + return { + "state": "open", + "head": {"sha": "head" if reads == 1 else "new-head"}, + "base": {"sha": "base"}, + } + if path == "issues/comments/42" and not kwargs: + return {"id": 42, "body": publisher.pending_message("head")} + if path.startswith("issues/") and "comments" in path: + return [ + { + "id": 42, + "user": {"login": "github-actions[bot]"}, + "body": publisher.pending_message("head"), + } + ] + return {} + + monkeypatch.setattr(publisher, "github", api) + publisher.publish(1, "head", "normal report", "base") + writes = [ + kwargs["data"]["body"] for path, kwargs in calls if path == "issues/comments/42" and kwargs + ] + assert len(writes) == 1 and "Performance assessment superseded" in writes[0] + + def test_base_moving_while_listing_comments_prevents_publish(monkeypatch): calls = [] reads = 0 @@ -930,6 +1032,108 @@ def api(path, **kwargs): assert reads == 2 and len(calls) == 3 +def test_abandoned_while_listing_comments_replaces_pending_with_terminal_state(monkeypatch): + calls = [] + reads = 0 + + def api(path, **kwargs): + nonlocal reads + calls.append((path, kwargs)) + if path.startswith("pulls/"): + reads += 1 + return { + "state": "open" if reads == 1 else "closed", + "merged": False, + "head": {"sha": "head"}, + "base": {"sha": "base"}, + } + if path == "issues/comments/42" and not kwargs: + return {"id": 42, "body": publisher.pending_message("head")} + if path.startswith("issues/") and "comments" in path: + return [ + { + "id": 42, + "user": {"login": "github-actions[bot]"}, + "body": publisher.pending_message("head"), + } + ] + return {} + + monkeypatch.setattr(publisher, "github", api) + publisher.publish(1, "head", "normal report", "base") + writes = [ + kwargs["data"]["body"] for path, kwargs in calls if path == "issues/comments/42" and kwargs + ] + assert len(writes) == 1 and "Pull request closed before assessment completed" in writes[0] + + +def test_abandoned_after_comment_write_is_immediately_terminalized(monkeypatch): + calls = [] + reads = 0 + + def api(path, **kwargs): + nonlocal reads + calls.append((path, kwargs)) + if path.startswith("pulls/"): + reads += 1 + return { + "state": "open" if reads < 3 else "closed", + "merged": False, + "head": {"sha": "head"}, + "base": {"sha": "base"}, + } + if path.startswith("issues/") and "comments" in path: + return [ + { + "id": 42, + "user": {"login": "github-actions[bot]"}, + "body": publisher.pending_message("head"), + } + ] + return {} + + monkeypatch.setattr(publisher, "github", api) + publisher.publish(1, "head", "normal report", "base") + writes = [kwargs["data"]["body"] for path, kwargs in calls if path == "issues/comments/42"] + assert writes[0] == "normal report" + assert "Pull request closed before assessment completed" in writes[1] + + +def test_head_change_after_comment_write_supersedes_only_unchanged_body(monkeypatch): + calls = [] + reads = 0 + + def api(path, **kwargs): + nonlocal reads + calls.append((path, kwargs)) + if path.startswith("pulls/"): + reads += 1 + return { + "state": "open", + "head": {"sha": "head" if reads < 3 else "new-head"}, + "base": {"sha": "base"}, + } + if path == "issues/comments/42" and not kwargs: + return {"id": 42, "body": "normal report"} + if path.startswith("issues/") and "comments" in path: + return [ + { + "id": 42, + "user": {"login": "github-actions[bot]"}, + "body": publisher.pending_message("head"), + } + ] + return {} + + monkeypatch.setattr(publisher, "github", api) + publisher.publish(1, "head", "normal report", "base") + writes = [ + kwargs["data"]["body"] for path, kwargs in calls if path == "issues/comments/42" and kwargs + ] + assert writes[0] == "normal report" + assert "Performance assessment superseded" in writes[1] + + @pytest.mark.parametrize( "corrupt", [ @@ -937,8 +1141,6 @@ def api(path, **kwargs): "zip", "timeout", "scenarios", - "suite", - "source", "base", "provenance", "recursion", @@ -948,17 +1150,11 @@ def api(path, **kwargs): ) def test_publisher_renders_validated_artifact_and_marks_missing_legs(report, monkeypatch, corrupt): posted = [] - windows = copy.deepcopy(report) - windows["leg"] = "Windows-SQL2022" - for pair in windows["pairs"]: - for sample in pair.values(): - sample["environment"]["os"] = "Windows" + linux_2025 = set_leg(report, "Linux-SQL2025") if corrupt == "scenarios": report["pairs"][0]["candidate"]["scenarios"] = list(reporting.CASES) - elif corrupt == "suite": - report["suite_hash"] = "e" * 64 data = { - "Windows-SQL2022": zip_data([("report.json", json.dumps(windows))]), + "Linux-SQL2025": zip_data([("report.json", json.dumps(linux_2025))]), "Linux-SQL2022": ( b"invalid ZIP" if corrupt == "zip" @@ -1000,13 +1196,6 @@ def api(url): monkeypatch.setattr( publisher, "publish", lambda number, head, body, base=None: posted.append(body) ) - monkeypatch.setattr(reporting, "suite_hash", lambda root: "d" * 64) - suite_versions = iter(({"suite": "source"}, {"suite": "base"})) - monkeypatch.setattr( - reporting, - "suite_blobs", - lambda *args: next(suite_versions) if corrupt == "source" else {"suite": "same"}, - ) monkeypatch.setattr( publisher, "github", @@ -1034,52 +1223,119 @@ def corrupt_deflate(raw): monkeypatch.setattr( publisher.time, "sleep", lambda seconds: clock.__setitem__(0, clock[0] + seconds) ) - publisher.run(123, "c" * 40, 1) + publisher.run(123, "c" * 40, 4) assert len(posted) == 2 assert posted[0].startswith(reporting.MARKER) if corrupt in ("base", "provenance"): assert "Build provenance validation failed" in posted[1] return - assert "| Windows / SQL Server 2025 | No result available" in posted[1] - if corrupt in ("suite", "source"): - assert "workload version differs from trusted base" in posted[1] - assert "consistent slowdown signals" not in posted[1] - elif corrupt in ("zip", "timeout", "scenarios", "recursion", "deflate"): - assert "### Windows / SQL Server 2022" in posted[1] + if corrupt in ("zip", "timeout", "scenarios", "recursion", "deflate"): + assert "### Unix / SQL Server 2025" in posted[1] assert reporting.escape("Linux-SQL2022 (invalid artifact)") in posted[1] assert "| Unix / SQL Server 2022 | No result available (invalid artifact) |" in posted[1] assert posted[1].count("20 consistent slowdown signals") == 1 else: - assert "### Windows / SQL Server 2022" in posted[1] + assert "**Coverage:** 2 of 2 environments completed." in posted[1] + assert "### Unix / SQL Server 2022" in posted[1] + assert "### Unix / SQL Server 2025" in posted[1] assert posted[1].count("40 consistent slowdown signals") == 1 def test_publisher_waits_for_newer_run_after_exact_head_build_is_canceled(report, monkeypatch): canceled = ado_build(id=41, result="canceled") replacement = {**canceled, "id": 42, "result": "failed"} - builds = iter(([canceled], [replacement])) + builds = [[canceled], [replacement]] posted = [] clock = [0] def api(url): - return {"value": next(builds)} if "/builds?" in url else {"value": []} + if "/builds?" in url: + return {"value": builds.pop(0) if len(builds) > 1 else builds[0]} + return {"value": []} monkeypatch.setattr(publisher, "api", api) monkeypatch.setattr(publisher, "github", pr_topology()) monkeypatch.setattr( publisher, "publish", lambda number, head, body, base=None: posted.append(body) ) - monkeypatch.setattr(reporting, "suite_blobs", lambda *args: {"suite": "same"}) monkeypatch.setattr(publisher.time, "monotonic", lambda: clock[0]) monkeypatch.setattr( publisher.time, "sleep", lambda seconds: clock.__setitem__(0, clock[0] + seconds) ) publisher.run(123, "c" * 40, 4) - assert clock[0] == 240 + assert clock[0] == 150 assert len(posted) == 2 assert "buildId=42" in posted[1] +def test_publisher_ignores_cancelling_build_artifacts_and_uses_replacement(monkeypatch): + posted = [] + clock = [0] + cancelling = ado_build(id=41, status="cancelling", result=None) + replacement = ado_build(id=42, status="inProgress", result=None) + builds = [[cancelling], [replacement]] + artifacts = [ + {"name": "profiler-" + leg, "resource": {"downloadUrl": "https://dev.azure.com/" + leg}} + for leg in reporting.LEGS + ] + + def api(url): + if "/builds?" in url: + return {"value": builds.pop(0) if len(builds) > 1 else builds[0]} + return {"value": artifacts} + + monkeypatch.setattr(publisher, "api", api) + monkeypatch.setattr(publisher, "github", pr_topology()) + monkeypatch.setattr( + publisher, "publish", lambda number, head, body, base=None: posted.append(body) + ) + monkeypatch.setattr( + reporting, "assess", lambda evidence, *args: f"buildId={evidence.build['id']}" + ) + monkeypatch.setattr(publisher.time, "monotonic", lambda: clock[0]) + monkeypatch.setattr( + publisher.time, "sleep", lambda seconds: clock.__setitem__(0, clock[0] + seconds) + ) + publisher.run(123, "c" * 40, 4) + assert clock[0] == 30 + assert posted[-1] == "buildId=42" + + +def test_publisher_restarts_artifact_grace_when_completed_build_resumes(monkeypatch): + posted = [] + clock = [0] + builds = [ + ado_build(), + ado_build(status="inProgress", result=None), + ado_build(), + ] + artifacts = [ + { + "name": "profiler-Linux-SQL2022", + "resource": {"downloadUrl": "https://dev.azure.com/Linux-SQL2022"}, + } + ] + + def api(url): + if "/builds?" in url: + return {"value": [builds.pop(0) if len(builds) > 1 else builds[0]]} + return {"value": artifacts} + + monkeypatch.setattr(publisher, "api", api) + monkeypatch.setattr(publisher, "github", pr_topology()) + monkeypatch.setattr( + publisher, "publish", lambda number, head, body, base=None: posted.append(body) + ) + monkeypatch.setattr(reporting, "assess", lambda *args: "partial report") + monkeypatch.setattr(publisher.time, "monotonic", lambda: clock[0]) + monkeypatch.setattr( + publisher.time, "sleep", lambda seconds: clock.__setitem__(0, clock[0] + seconds) + ) + publisher.run(123, "c" * 40, 2) + assert clock[0] == 180 + assert posted[-1] == "partial report" + + @pytest.mark.parametrize("result", [None, "unknown"]) def test_publisher_rejects_unsupported_completed_results(monkeypatch, result): posted = [] @@ -1100,18 +1356,18 @@ def api(url): @pytest.mark.parametrize("status", [None, "notStarted", "inProgress"]) -def test_publisher_deadline_finishes_without_reading_unfinished_build_metadata(monkeypatch, status): +def test_publisher_deadline_finishes_with_terminal_comment(monkeypatch, status): posted = [] clock = [0] - build = ado_build(status=status, sourceVersion=None) + build = ado_build(status=status, result=None) def github(path): - assert path == "pulls/123", "Unfinished builds must not query merge topology" - return {"state": "open", "head": {"sha": "c" * 40}, "base": {"sha": "a" * 40}} + return pr_topology()(path) def api(url): - assert "/builds?" in url, "Unfinished builds must not query artifacts" - return {"value": [] if status is None else [build]} + if "/builds?" in url: + return {"value": [] if status is None else [build]} + return {"value": []} def sleep(seconds): clock[0] += seconds @@ -1127,7 +1383,6 @@ def sleep(seconds): assert clock[0] == 60 and len(posted) == 2 assert "Performance assessment pending" in posted[0] assert "Performance assessment pending" not in posted[1] - assert "1-minute wait" in posted[1] assert "Performance could not be assessed" in posted[1] @@ -1150,6 +1405,29 @@ def test_publisher_retries_transient_polling_failures_before_finalizing(monkeypa assert "Performance could not be assessed" in posted[1] +def test_publisher_bounds_consecutive_artifact_service_failures(monkeypatch): + posted = [] + clock = [0] + + def api(url): + if "/artifacts?" in url: + raise URLError("temporary") + return {"value": [ado_build(status="inProgress", result=None)]} + + monkeypatch.setattr(publisher, "api", api) + monkeypatch.setattr(publisher, "github", pr_topology()) + monkeypatch.setattr( + publisher, "publish", lambda number, head, body, base=None: posted.append(body) + ) + monkeypatch.setattr(publisher.time, "monotonic", lambda: clock[0]) + monkeypatch.setattr( + publisher.time, "sleep", lambda seconds: clock.__setitem__(0, clock[0] + seconds) + ) + publisher.run(123, "c" * 40, 10) + assert clock[0] == 120 + assert "Performance data services failed repeatedly" in posted[-1] + + def test_publisher_retries_malformed_pr_and_artifact_responses(monkeypatch): posted = [] clock = [0] @@ -1178,7 +1456,7 @@ def test_publisher_retries_malformed_pr_and_artifact_responses(monkeypatch): def test_artifact_polling_uses_remaining_publication_budget(monkeypatch): posted = [] clock = [0] - build = ado_build() + build = ado_build(status="inProgress", result=None) artifacts = [ {"name": "profiler-" + leg, "resource": {"downloadUrl": "https://dev.azure.com/" + leg}} for leg in reporting.LEGS @@ -1201,13 +1479,123 @@ def api(url): publisher.run(123, "c" * 40, 4) assert clock[0] == 150 assert posted == [ - publisher.HEADER - + "**Performance assessment pending.**\n\n" - + f"Waiting for the matching performance run for head `{'c' * 40}`.", + publisher.pending_message("c" * 40), "final report", ] +def test_publisher_finishes_after_merge_before_aggregate_build(monkeypatch): + posted = [] + build = ado_build(status="inProgress", result=None) + artifacts = [ + {"name": "profiler-" + leg, "resource": {"downloadUrl": "https://dev.azure.com/" + leg}} + for leg in reporting.LEGS + ] + + def api(url): + return {"value": artifacts} if "/artifacts?" in url else {"value": [build]} + + monkeypatch.setattr(publisher, "api", api) + monkeypatch.setattr(publisher, "github", pr_topology(state="closed", merged=True)) + monkeypatch.setattr( + publisher, "publish", lambda number, head, body, base=None: posted.append(body) + ) + monkeypatch.setattr(reporting, "assess", lambda *args: "final report") + sleeps = [] + monkeypatch.setattr(publisher.time, "sleep", sleeps.append) + publisher.run(123, "c" * 40, 4) + assert sleeps == [] + assert posted[-1] == "final report" + + +def test_publisher_waits_for_usable_artifact_urls(monkeypatch): + posted = [] + build = ado_build(status="inProgress", result=None) + valid = [ + {"name": "profiler-" + leg, "resource": {"downloadUrl": "https://dev.azure.com/" + leg}} + for leg in reporting.LEGS + ] + invalid = copy.deepcopy(valid) + invalid[0]["resource"]["downloadUrl"] = "" + responses = [invalid, valid] + + def api(url): + if "/artifacts?" in url: + return {"value": responses.pop(0)} + return {"value": [build]} + + monkeypatch.setattr(publisher, "api", api) + monkeypatch.setattr(publisher, "github", pr_topology()) + monkeypatch.setattr( + publisher, "publish", lambda number, head, body, base=None: posted.append(body) + ) + monkeypatch.setattr(reporting, "assess", lambda *args: "final report") + sleeps = [] + monkeypatch.setattr(publisher.time, "sleep", sleeps.append) + publisher.run(123, "c" * 40, 4) + assert sleeps == [30] + assert posted[-1] == "final report" + + +def test_completed_build_publishes_partial_result_after_artifact_grace(monkeypatch): + posted = [] + clock = [0] + build = ado_build() + artifacts = [ + { + "name": "profiler-Linux-SQL2022", + "resource": {"downloadUrl": "https://dev.azure.com/Linux-SQL2022"}, + } + ] + + def api(url): + return {"value": artifacts} if "/artifacts?" in url else {"value": [build]} + + monkeypatch.setattr(publisher, "api", api) + monkeypatch.setattr(publisher, "github", pr_topology()) + monkeypatch.setattr( + publisher, "publish", lambda number, head, body, base=None: posted.append(body) + ) + monkeypatch.setattr(reporting, "assess", lambda *args: "partial report") + monkeypatch.setattr(publisher.time, "monotonic", lambda: clock[0]) + monkeypatch.setattr( + publisher.time, "sleep", lambda seconds: clock.__setitem__(0, clock[0] + seconds) + ) + publisher.run(123, "c" * 40, 1) + assert clock[0] == publisher.ARTIFACT_GRACE_SECONDS + assert posted[-1] == "partial report" + + +def test_deadline_does_not_assess_partial_running_build(monkeypatch): + posted = [] + clock = [0] + build = ado_build(status="inProgress", result=None) + artifacts = [ + { + "name": "profiler-Linux-SQL2022", + "resource": {"downloadUrl": "https://dev.azure.com/Linux-SQL2022"}, + } + ] + + def api(url): + return {"value": artifacts} if "/artifacts?" in url else {"value": [build]} + + monkeypatch.setattr(publisher, "api", api) + monkeypatch.setattr(publisher, "github", pr_topology()) + monkeypatch.setattr( + publisher, "publish", lambda number, head, body, base=None: posted.append(body) + ) + monkeypatch.setattr( + reporting, "assess", lambda *args: pytest.fail("running partial build must not assess") + ) + monkeypatch.setattr(publisher.time, "monotonic", lambda: clock[0]) + monkeypatch.setattr( + publisher.time, "sleep", lambda seconds: clock.__setitem__(0, clock[0] + seconds) + ) + publisher.run(123, "c" * 40, 1) + assert "did not become ready within the 1-minute wait" in posted[-1] + + def test_artifact_symlink_and_oversized_json_are_rejected(): symlink = zipfile.ZipInfo("report.json") symlink.create_system = 3 @@ -1237,25 +1625,19 @@ def test_ci_reuses_profiling_builds_without_changing_release_defaults(): pipeline, ) assert len(profiler_conditions) == 4 + assert profiler_conditions.count("false") == 2 assert all( "eq(variables['Build.Reason'], 'PullRequest')" in condition for condition in profiler_conditions + if condition != "false" ) for release in (ROOT / "OneBranchPipelines").rglob("*.yml"): assert "ENABLE_PROFILING" not in release.read_text(encoding="utf-8") windows = pipeline.split("- job: pytestonwindows\n", 1)[1].split("\n- job:", 1)[0] assert "##vso[task.setvariable" not in windows - assert "ENABLE_PROFILING: 1" in windows - assert "ArtifactName: 'ddbc_bindings-profiling-$(sqlVersion)'" in windows + assert windows.count("condition: false") >= 5 assert "ArtifactName: 'ddbc_bindings'" in windows - assert ( - "condition: and(succeeded(), eq(variables['Build.Reason'], 'PullRequest'), " - "ne(variables['sqlVersion'], 'LocalDB'))" - ) in windows - assert ( - "condition: and(succeeded(), or(ne(variables['Build.Reason'], 'PullRequest'), " - "eq(variables['sqlVersion'], 'LocalDB')))" - ) in windows + assert "Hosted Windows timings varied more than the regression threshold" in windows macos = pipeline.split("- job: PytestOnMacOS\n", 1)[1].split("\n- job:", 1)[0] assert "timeoutInMinutes: 90" in macos assert "ENABLE_PROFILING" not in macos