diff --git a/.aspect/config.axl b/.aspect/config.axl index 8a526b4e0..68c42c20b 100644 --- a/.aspect/config.axl +++ b/.aspect/config.axl @@ -1,6 +1,6 @@ load("@aspect//feature/artifacts.axl", "ArtifactUpload") load("@aspect//feature/buildkite_annotations.axl", "BuildkiteAnnotations") -load("@aspect//feature/buildkite_annotations_test.axl", "bk_annotation_snapshot_tests") +load("@aspect//feature/buildkite_annotations_test.axl", "bk_annotation_decision_tests", "bk_annotation_snapshot_tests") load("@aspect//feature/github_status_checks.axl", "GithubStatusChecks") load("@aspect//feature/github_status_comments_test.axl", "pr_comment_snapshot_tests") load("@aspect//lib/bazel_results_test.axl", "template_snapshot_tests") @@ -123,6 +123,13 @@ def config(ctx: ConfigContext): # Run with: aspect dev test-bk-annotation-snapshots ctx.tasks.add(bk_annotation_snapshot_tests) + # Buildkite annotations — unit tests for the kind/context_id + # renderability decision. Locks in the contract that empty-kind + # phase markers (setup_phase / preflight_phase) skip silently + # without emitting "no renderer for kind=" trace noise. + # Run with: aspect dev test-bk-annotation-decision + ctx.tasks.add(bk_annotation_decision_tests) + # GitHub PR comment — snapshots the aggregate-status PR comment # body across mixed task kinds, statuses, link availability, and # phase / progress shapes. diff --git a/crates/aspect-cli/src/builtins/aspect/feature/buildkite_annotations.axl b/crates/aspect-cli/src/builtins/aspect/feature/buildkite_annotations.axl index dbf6a5421..d7ec025a2 100644 --- a/crates/aspect-cli/src/builtins/aspect/feature/buildkite_annotations.axl +++ b/crates/aspect-cli/src/builtins/aspect/feature/buildkite_annotations.axl @@ -41,6 +41,40 @@ def format_body(out, kind, status, task_label = "", final = False): """Public test hook; see `_format_body` for behavior.""" return _format_body(out, kind, status, task_label = task_label, final = final) +def should_render_update(update, context_id): + """Decide whether a `TaskUpdate` is renderable by the BK annotation surface. + + Returns `("render", "")` when the update should be rendered; + `("skip", reason)` otherwise. The `reason` is a short tag that + callers can log at trace level or assert against in tests. + + Skip reasons: + `"no_kind"` — empty `update.kind`. The documented + "no template" contract (see + `lib/lifecycle.axl`): phase-marker + updates from `setup_phase` / + `preflight_phase` emit so non-template + observers (telemetry) see every state + change, but have no per-kind data to + render. Silent skip; not a bug. + `"no_context_id"` — kind is non-empty but `task_started` hasn't + populated the BK annotation context yet. + Renderable update fired too early; the + handler can't post the annotation. + `"unknown_kind"` — kind is non-empty and routed past the + empty-kind skip, but no entry exists in + `RENDERERS`. Indicates a misconfig (new + task kind added without registering a + renderer). + """ + if not update.kind: + return ("skip", "no_kind") + if not context_id: + return ("skip", "no_context_id") + if renderer_for_kind(update.kind) == None: + return ("skip", "unknown_kind") + return ("render", "") + # ─── Body formatting ────────────────────────────────────────────────────────── _LOG = feature_logger("Buildkite annotations") @@ -294,21 +328,26 @@ def _buildkite_annotations(ctx: FeatureContext): final = update.final # Keep the most recent phase label on bare refreshes (empty - # progress styles). BK renders the narrative `body` form. + # progress styles). BK renders the narrative `body` form. Done + # before the renderability check below so phase-marker updates + # (`kind=""` from `setup_phase` / `preflight_phase`) still latch + # their progress text for the next renderable kind to pick up. progress_text = update.progress.body if progress_text: _state["_progress"] = progress_text context_id = _state.get("_context_id", "") - if not context_id: - _LOG.trace("task_update fired but no context_id; ignoring (kind=%s status=%s)" % - (update.kind, status)) + decision, reason = should_render_update(update, context_id) + if decision == "skip": + # `no_kind` is the documented phase-marker contract — silent. + # `no_context_id` / `unknown_kind` are routing/config bugs: + # trace so they surface under `ASPECT_DEBUG=1`. + if reason != "no_kind": + _LOG.trace("ignoring task_update (reason=%s kind=%r status=%s)" % + (reason, update.kind, status)) return renderer = renderer_for_kind(update.kind) - if renderer == None: - _LOG.trace("no renderer for kind=%r; ignoring" % update.kind) - return data = update.data if update.data else renderer.init() # Bypass throttle on first-failing (so the red stripe lands diff --git a/crates/aspect-cli/src/builtins/aspect/feature/buildkite_annotations_test.axl b/crates/aspect-cli/src/builtins/aspect/feature/buildkite_annotations_test.axl index 93ce7df1f..0adebf56f 100644 --- a/crates/aspect-cli/src/builtins/aspect/feature/buildkite_annotations_test.axl +++ b/crates/aspect-cli/src/builtins/aspect/feature/buildkite_annotations_test.axl @@ -15,7 +15,8 @@ Coverage: all-skipped, delivery all-failed, soft-format passing with files. """ -load("./buildkite_annotations.axl", "format_body") +load("./buildkite_annotations.axl", "format_body", "should_render_update") +load("../lib/lifecycle.axl", "TaskUpdate") # Shared fixture factories — also consumed by github_status_comments_test.axl. load( @@ -179,3 +180,73 @@ bk_annotation_snapshot_tests = task( implementation = _test_impl, args = {}, ) + +# ─── should_render_update unit tests ────────────────────────────────────────── + +def _expect_render_decision(label, update, context_id, want_decision, want_reason): + got_decision, got_reason = should_render_update(update, context_id) + if got_decision != want_decision or got_reason != want_reason: + fail("%s: got (%r, %r), want (%r, %r)" % + (label, got_decision, got_reason, want_decision, want_reason)) + +def _decision_test_impl(ctx): + # Phase-marker updates (setup_phase / preflight_phase) emit + # `kind=""` per the lifecycle docstring contract. The BK surface + # MUST skip them silently — logging would spam every phase + # boundary. This case is the regression guard for the + # `no renderer for kind=""` log noise. + _expect_render_decision( + "empty kind, no context_id (setup_phase ordering)", + TaskUpdate(kind = "", status = "running"), + "", + "skip", + "no_kind", + ) + _expect_render_decision( + "empty kind, context_id set (preflight_phase ordering)", + TaskUpdate(kind = "", status = "running"), + "aspect-lint-bk", + "skip", + "no_kind", + ) + + # Renderable kind but no context yet — `task_started` hasn't run. + # Trace-worthy because a renderer-mapped update arrived before + # the BK annotation context was prepared. + _expect_render_decision( + "renderable kind, missing context_id", + TaskUpdate(kind = "lint_results", status = "running"), + "", + "skip", + "no_context_id", + ) + + # Kind present but not in RENDERERS — misconfig (new task kind + # added without a renderer registration). + _expect_render_decision( + "unknown kind", + TaskUpdate(kind = "wat_results", status = "running"), + "aspect-lint-bk", + "skip", + "unknown_kind", + ) + + # Happy path — known kind + populated context. + for kind in ["bazel_results", "lint_results", "delivery_results", "format_results", "gazelle_results"]: + _expect_render_decision( + "renderable: " + kind, + TaskUpdate(kind = kind, status = "running"), + "aspect-lint-bk", + "render", + "", + ) + + print("should_render_update: OK (9 scenarios)") + return 0 + +bk_annotation_decision_tests = task( + name = "test-bk-annotation-decision", + group = ["dev"], + implementation = _decision_test_impl, + args = {}, +)