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