From fb937a5e3f5fa0a45f89d14ed7c4b52c7b88f4ac Mon Sep 17 00:00:00 2001 From: martin-velay Date: Tue, 18 Aug 2026 13:54:11 +0200 Subject: [PATCH 1/5] fix: strip credentials from the recorded git origin url CI checkouts leave the access token in the remote url, like https://oauth2:@host/org/repo. We record that url in the run metadata and print it in the JSON and HTML reports, and those get archived, so the token leaks with them. ssh urls keep their user. git@ is part of the address, so dropping it would give a url that no longer reaches the remote. Passwords are stripped whatever the scheme. AI-assisted (Claude Code) - reviewed and approved by author Signed-off-by: martin-velay --- src/dvsim/utils/git.py | 29 +++++++++++++++++-- tests/utils/test_git.py | 64 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/dvsim/utils/git.py b/src/dvsim/utils/git.py index ce537bdf..0776d9de 100644 --- a/src/dvsim/utils/git.py +++ b/src/dvsim/utils/git.py @@ -5,6 +5,7 @@ """Git utility functions.""" from pathlib import Path +from urllib.parse import urlsplit, urlunsplit from git import Repo @@ -13,6 +14,26 @@ __all__ = ("repo_root",) +def strip_url_credentials(url: str) -> str: + """Remove any username/password from a URL, keeping the rest of it intact. + + A CI checkout leaves its access token in the remote URL as ``https://oauth2:@host/org/repo``. + That URL is recorded in the run metadata and published in the reports, which get archived, so the + token would outlive the run that leaked it. + + An ssh URL keeps its user: ``git@`` names an ssh account rather than an identity to authenticate + as, so dropping it gives a URL that no longer addresses the remote. A password is stripped + whatever the scheme carries it. + """ + parts = urlsplit(url) + userinfo, _, host = parts.netloc.rpartition("@") + if not userinfo: + return url + if ":" not in userinfo and parts.scheme not in ("http", "https"): + return url + return urlunsplit((parts.scheme, host, parts.path, parts.query, parts.fragment)) + + def repo_root(path: Path) -> Path | None: """Given a sub dir in a git repo provide the root path. @@ -56,7 +77,11 @@ def git_is_dirty(path: Path | None = None) -> bool: def git_origin_url(path: Path | None = None) -> str | None: - """Get the git remote origin url, or None if no ``origin`` remote is configured.""" + """Get the git remote origin url, or None if no ``origin`` remote is configured. + + Any credentials the remote carries are stripped, so that the url is safe to record in run + metadata and reports. + """ root = repo_root(path=path or Path.cwd()) if root is None: @@ -68,7 +93,7 @@ def git_origin_url(path: Path | None = None) -> str | None: if "origin" not in [remote.name for remote in r.remotes]: return None - return r.remote("origin").url + return strip_url_credentials(r.remote("origin").url) def git_https_url_with_commit(path: Path | None = None) -> str | None: diff --git a/tests/utils/test_git.py b/tests/utils/test_git.py index 06bde5ce..5516fe22 100644 --- a/tests/utils/test_git.py +++ b/tests/utils/test_git.py @@ -132,6 +132,66 @@ def test_git_origin_url(tmp_path: Path) -> None: equal_to(url), ) + @staticmethod + @pytest.mark.parametrize( + ("url", "expected"), + [ + ( + "https://github.com/lowRISC/test.git", + "https://github.com/lowRISC/test.git", + ), + ("git@github.com:lowRISC/test.git", "git@github.com:lowRISC/test.git"), + # An ssh user is part of the address rather than a credential, so it stays. Dropping + # it would leave a url that no longer reaches the remote. + ( + "ssh://git@github.com/lowRISC/test.git", + "ssh://git@github.com/lowRISC/test.git", + ), + ( + "git+ssh://git@github.com/lowRISC/test.git", + "git+ssh://git@github.com/lowRISC/test.git", + ), + ( + "https://oauth2:ghs_secrettoken@github.com/lowRISC/test.git", + "https://github.com/lowRISC/test.git", + ), + ( + "https://someuser@github.com/lowRISC/test.git", + "https://github.com/lowRISC/test.git", + ), + # A password is a credential whatever the scheme carries it. + ( + "ssh://user:secretpw@github.com/lowRISC/test.git", + "ssh://github.com/lowRISC/test.git", + ), + ], + ids=[ + "plain_https", + "ssh_scp_form", + "ssh_url_form", + "git_ssh_scheme", + "token", + "user_only", + "ssh_with_password", + ], + ) + def test_git_origin_url_strips_credentials(tmp_path: Path, url: str, expected: str) -> None: + """A token in the remote url never reaches the caller, whatever the url's shape. + + The url is recorded in the run metadata and published in the reports, so a credential + left in it outlives the run. Both ssh forms carry an '@' without being credentialed, and + have to survive untouched or the recorded url stops addressing the remote. + """ + r = Repo.init(path=tmp_path) + + file = tmp_path / "a" + file.write_text("file to commit") + r.index.add([file]) + r.index.commit("initial commit") + r.create_remote("origin", url) + + assert_that(git_origin_url(tmp_path), equal_to(expected)) + @staticmethod @pytest.mark.parametrize( ("url", "expected"), @@ -141,6 +201,10 @@ def test_git_origin_url(tmp_path: Path) -> None: "https://github.com/lowRISC/test.git", "https://github.com/lowRISC/test/tree/{commit}", ), + ( + "https://oauth2:ghs_secrettoken@github.com/lowRISC/test.git", + "https://github.com/lowRISC/test/tree/{commit}", + ), ], ) def test_git_https_url_with_commit(tmp_path: Path, url: str, expected: str) -> None: From c9dc3755a4615ac1455e88a90e96c0a877292227 Mon Sep 17 00:00:00 2001 From: martin-velay Date: Tue, 18 Aug 2026 13:54:11 +0200 Subject: [PATCH 2/5] feat: add a scheduler callback for jobs reaching a terminal state The status-change callback carries no reason, so an observer cannot tell a failed job from one the scheduler cancelled before dispatching it. The new callback gets the reason and fires once the status is settled. The existing callback is untouched, since its only consumer is the status printer and that has no use for the reason. Flows opt in by overriding FlowCfg.on_job_completed, which does nothing by default. AI-assisted (Claude Code) - reviewed and approved by author Signed-off-by: martin-velay --- src/dvsim/flow/base.py | 13 ++++++++++++- src/dvsim/scheduler/core.py | 16 ++++++++++++++++ src/dvsim/scheduler/runner.py | 8 +++++++- 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index 497e8a42..9a0f199c 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -18,7 +18,7 @@ import dvsim.instrumentation.runtime as instrumentation from dvsim.flow.hjson import set_target_attribute -from dvsim.job.data import CompletedJobStatus, JobSpec, WorkspaceConfig +from dvsim.job.data import CompletedJobStatus, JobSpec, JobStatusInfo, WorkspaceConfig from dvsim.job.status import JobStatus from dvsim.logging import log from dvsim.scheduler.resources import UnknownResourcePolicy @@ -491,9 +491,20 @@ def deploy_objects(self) -> Sequence[CompletedJobStatus]: interactive=self.interactive, backend=backend, resource_manager=resource_manager, + on_job_completed=self.on_job_completed, ) ) + def on_job_completed( + self, spec: JobSpec, status: JobStatus, reason: JobStatusInfo | None + ) -> None: + """Observe a job reaching a terminal state. Flows that care override this. + + Called by the scheduler as each job finishes, so a flow can record an outcome while the + run is still going. Not abstract, since observing this is opt-in. + """ + del spec, status, reason + @abstractmethod def gen_results(self, results: Sequence[CompletedJobStatus]) -> None: """Generate flow results. diff --git a/src/dvsim/scheduler/core.py b/src/dvsim/scheduler/core.py index 026ddd0e..3b61bb9f 100644 --- a/src/dvsim/scheduler/core.py +++ b/src/dvsim/scheduler/core.py @@ -23,6 +23,7 @@ __all__ = ( "JobPriorityFn", "JobRecord", + "OnJobCompletionCb", "OnJobStatusChangeCb", "OnRunEndCb", "OnRunStartCb", @@ -63,6 +64,12 @@ class JobRecord: # The arguments are: (job spec, old status, new status). OnJobStatusChangeCb: TypeAlias = Callable[[JobSpec, JobStatus, JobStatus], None] +# Callbacks for observers, for when a job reaches a terminal state. +# The arguments are: (job spec, terminal status, the reason recorded with it). +# Separate from the status-change callback, which carries no reason and so cannot tell a +# cancelled job from one killed while running. +OnJobCompletionCb: TypeAlias = Callable[[JobSpec, JobStatus, JobStatusInfo | None], None] + # Callbacks for observers, for when the scheduler receives a kill signal (termination). OnSchedulerKillCb: TypeAlias = Callable[[], None] @@ -153,6 +160,7 @@ def __init__( # noqa: PLR0913 self._on_run_start: list[OnRunStartCb] = [] self._on_run_end: list[OnRunEndCb] = [] self._on_job_status_change: list[OnJobStatusChangeCb] = [] + self._on_job_completion: list[OnJobCompletionCb] = [] self._on_kill_signal: list[OnSchedulerKillCb] = [] self._jobs = self.build_graph(jobs, self._backends, self._default_backend) @@ -165,6 +173,10 @@ def add_run_end_callback(self, cb: OnRunEndCb) -> None: """Register an observer to notify when the scheduler run ends.""" self._on_run_end.append(cb) + def add_job_completion_callback(self, cb: OnJobCompletionCb) -> None: + """Register an observer to be notified as each job reaches a terminal state.""" + self._on_job_completion.append(cb) + def add_job_status_change_callback(self, cb: OnJobStatusChangeCb) -> None: """Register an observer to notify when the status of a job in the scheduler changes.""" self._on_job_status_change.append(cb) @@ -322,6 +334,10 @@ def _mark_job_completed( ) self._change_job_status(job, status, reason) + # Notified after the status is settled, so an observer sees what the scheduler concluded + for cb in self._on_job_completion: + cb(job.spec, status, reason) + # If the job was running, mark it as no longer running. if job.spec.id in self._running: self._running.remove(job.spec.id) diff --git a/src/dvsim/scheduler/runner.py b/src/dvsim/scheduler/runner.py index 0244fff3..1d80902d 100644 --- a/src/dvsim/scheduler/runner.py +++ b/src/dvsim/scheduler/runner.py @@ -11,7 +11,7 @@ from dvsim.runtime.backend import RuntimeBackend from dvsim.runtime.fake import FakePolicy, FakeRuntimeBackend from dvsim.runtime.registry import backend_registry -from dvsim.scheduler.core import Scheduler +from dvsim.scheduler.core import OnJobCompletionCb, Scheduler from dvsim.scheduler.log_manager import LogManager from dvsim.scheduler.resources import ( ResourceManager, @@ -76,6 +76,7 @@ async def run_scheduler( interactive: bool, backend: RuntimeBackend, resource_manager: ResourceManager | None, + on_job_completed: OnJobCompletionCb | None = None, ) -> list[CompletedJobStatus]: """Run the scheduler with the given set of job specifications. @@ -85,6 +86,8 @@ async def run_scheduler( interactive: run the tool in interactive mode? backend: the scheduler backend to use resource_manager: the scheduler resource manager to use, if any. + on_job_completed: observer notified as each job reaches a terminal state, with the + reason the scheduler recorded for it. Returns: List of completed job status objects. @@ -112,6 +115,9 @@ async def run_scheduler( ), ) + if on_job_completed is not None: + scheduler.add_job_completion_callback(on_job_completed) + if not interactive: status_printer = create_status_printer(jobs) From 7f2bd4d0e3734f077adcb3bfcc6cb6bd01a7282c Mon Sep 17 00:00:00 2001 From: martin-velay Date: Tue, 18 Aug 2026 13:54:11 +0200 Subject: [PATCH 3/5] feat: back-annotate a DVPlan vPlan from the regression's own results A cov_vplan job runs dvplan over the coverage report and a dv_evidence.json this flow writes, so a nightly says how much of the verification plan is met and not just how much of the design was covered. Nothing runs unless the sim cfg names a vplan. The evidence comes from the scheduler's completion hook as each run finishes, so it covers jobs cancelled before dispatch too. A test the plan asked for and the regression never ran then shows up as a hole instead of going missing. Both sources go to one dvplan invocation, because dvplan writes an item off as unmeasurable when nothing it was given can measure it, and that sticks in the annotated file. Runs without --cov score from the evidence alone. Deploy gains a log_path property for the path it already built inline, which is what the LSF launcher reaches for. AI-assisted (Claude Code) - reviewed and approved by author Signed-off-by: martin-velay --- src/dvsim/job/deploy.py | 159 ++++++++++-------- src/dvsim/report/dv_evidence.py | 203 +++++++++++++++++++++++ src/dvsim/report/vplan.py | 172 +++++++++++++++++++ src/dvsim/sim/flow.py | 80 ++++++--- tests/job/test_cov_vplan.py | 114 +++++++++++++ tests/report/test_dv_evidence.py | 272 +++++++++++++++++++++++++++++++ tests/report/test_vplan.py | 186 +++++++++++++++++++++ 7 files changed, 1098 insertions(+), 88 deletions(-) create mode 100644 src/dvsim/report/dv_evidence.py create mode 100644 src/dvsim/report/vplan.py create mode 100644 tests/job/test_cov_vplan.py create mode 100644 tests/report/test_dv_evidence.py create mode 100644 tests/report/test_vplan.py diff --git a/src/dvsim/job/deploy.py b/src/dvsim/job/deploy.py index 6cdac479..358b411b 100644 --- a/src/dvsim/job/deploy.py +++ b/src/dvsim/job/deploy.py @@ -17,6 +17,13 @@ from dvsim.job.time import JobTime from dvsim.logging import log from dvsim.report.data import IPMeta, ToolMeta +from dvsim.report.dv_evidence import write_evidence +from dvsim.report.vplan import ( + VPLAN_DIR, + VPlanInputs, + overall_coverage, + shell_command, +) from dvsim.test import Test from dvsim.tool.utils import get_sim_tool_plugin from dvsim.utils import ( @@ -177,7 +184,7 @@ def get_job_spec(self) -> "JobSpec": interactive=self.sim_cfg.interactive, odir=self.odir, renew_odir=self.renew_odir, - log_path=Path(f"{self.odir}/{self.target}.log"), + log_path=self.log_path, pre_launch=self.pre_launch(), post_finish=self.post_finish(), pass_patterns=self.pass_patterns, @@ -379,6 +386,11 @@ def is_equivalent_job(self, item: "Deploy") -> bool: log.verbose('Deploy job "%s" is equivalent to "%s"', item.name, self.name) return True + @property + def log_path(self) -> Path: + """Path to the log this job writes.""" + return Path(f"{self.odir}/{self.target}.log") + def pre_launch(self) -> Callable[[], None]: """Get pre-launch callback.""" @@ -1038,104 +1050,113 @@ def _set_attrs(self) -> None: class CovVPlan(Deploy): - """Abstraction for generating a Verification Plan (vPlan) report using DVPlan.""" + """Back-annotate the DVPlan verification plan, as a job of its own. + + Scheduled like any other job, so the step gets a row in the status table and can depend on the + runs it annotates. + """ target = "cov_vplan" weight = 10 - def __init__(self, cov_report_job, sim_cfg) -> None: - self.report_job = cov_report_job + def __init__(self, dependencies: "Iterable[Deploy]", sim_cfg: "SimCfg") -> None: + """Construct the job, depending on whatever must finish before the plan can be scored.""" + # Register a copy of sim_cfg which is explicitly the SimCfg type + self._typed_sim_cfg: SimCfg = sim_cfg + # Extracted from the hjson cfg by _set_attrs, and declared here so a type checker knows + # they exist, as the base class does for its own + self.proj_root: str = "" + self.vplan: str = "" + self.dut_instance: str = "" + self.dvplan_inspect: str = "" # Populated by post_finish() once the job completes successfully. self.vplan_coverage: float | None = None super().__init__(sim_cfg) - self.dependencies.append(cov_report_job) + # Every run it scores has to be terminal first, so the collector's evidence is complete + self.dependencies.extend(dependencies) + # A failed or killed run is still evidence, so score what happened rather than skipping + self.needs_all_dependencies_passing = False def _define_attrs(self) -> None: super()._define_attrs() - self.mandatory_cmd_attrs.update( - { - "proj_root": False, - "vplan": False, - } - ) + self.mandatory_cmd_attrs.update({"proj_root": False, "vplan": False}) self.mandatory_misc_attrs.update( { "dut_instance": False, + # Optional. Unlike the coverage report and the test results, inspection records + # are written by hand and live in the tree, so dvsim only points dvplan at them + "dvplan_inspect": False, } ) def _set_attrs(self) -> None: - self.cov_vplan_dir = f"{self.sim_cfg.scratch_path}/{self.target}" + # The base class derives `odir` from an attribute named after the target, and it does so + # inside the super() call below, so this has to be set first. + self.cov_vplan_dir = f"{self.sim_cfg.scratch_path}/{VPLAN_DIR}" super()._set_attrs() self.qual_name = self.target self.full_name = f"{self.sim_cfg.name}{self._variant_suffix}:{self.qual_name}" + self.output_dirs = [self.odir] - self.prepare_opts = self.sim_cfg.cov_vplan_prepare_opts - self.process_opts = self.sim_cfg.cov_vplan_process_opts + @property + def annotated_hjson(self) -> Path: + """Where the annotated plan is written.""" + return self._inputs().annotated + + @property + def report_page(self) -> Path: + """Where the plan's HTML report is written.""" + return self._inputs().report + + def _inputs(self) -> VPlanInputs: + """Describe the annotation, so `report.vplan` needs nothing from the flow config.""" + cfg = self._typed_sim_cfg + return VPlanInputs( + vplan=Path(self.vplan), + out_dir=Path(self.odir), + dut_entity=cfg.name, + dut_instance=self.dut_instance, + cov_report_dir=Path(cfg.cov_report_dir) if cfg.cov else None, + tool=cfg.tool or "", + inspect=self.dvplan_inspect, + prepare_opts=list(cfg.cov_vplan_prepare_opts), + process_opts=list(cfg.cov_vplan_process_opts), + ) - # Calculate IP root. - vplan_path = Path(self.vplan) - self.ip_root = str(vplan_path.parent.parent) + def _construct_cmd(self) -> str: + """Build the dvplan invocation this job runs.""" + return shell_command(self._inputs()) - # Use fixed output filenames so the report location is always predictable. - self.annotated_hjson = f"{self.odir}/vplan_annotated.hjson" - self.gen_html = f"{self.odir}/vplan_annotated.html" - self.output_dirs = [self.odir] + def pre_launch(self) -> Callable[[], None]: + """Get pre-launch callback.""" + + def callback() -> None: + """Write the evidence dvplan annotates the vPlan from. + + Every run this job depends on is terminal by now, so the collector holds them all. + Written here rather than with the end-of-run reports because dvplan needs every coverage + source in one invocation, as `report.vplan._process_command` explains. + """ + cfg = self._typed_sim_cfg + write_evidence( + self._inputs().evidence, + cfg.run_evidence.evidence( + block=cfg.block_meta(), + tool=cfg.tool, + timestamp=cfg.run_timestamp().isoformat(), + ), + ) + + return callback def post_finish(self) -> Callable[[JobStatus], None]: """Get post finish callback.""" def callback(status: JobStatus) -> None: - """Extract the overall vPlan normalised coverage from the annotated HJSON.""" + """Read the plan's overall score back, for the flow's own report to quote.""" if self.dry_run or status != JobStatus.PASSED: return - hjson_path = Path(self.annotated_hjson) - if not hjson_path.exists(): - return - try: - import hjson # noqa: PLC0415 - - with hjson_path.open() as f: - data = hjson.load(f) - # HJSON vPlans are keyed: {dut_name: {fields...}} - root_node = next(iter(data.values()), {}) - raw = root_node.get("Normalized_Coverage") - if raw is not None: - self.vplan_coverage = float(str(raw).rstrip(" %")) - except Exception: # noqa: BLE001 - log.debug("Could not extract vPlan coverage from '%s'.", hjson_path) + self.vplan_coverage = overall_coverage(self.annotated_hjson) return callback - - def _construct_cmd(self) -> str: - """Construct the pure bash shell command, bypassing the base Makefile assumption.""" - import shlex - import shutil - - if shutil.which("dvplan") is None: - fallback = ( - "echo 'WARNING: dvplan tool not installed in PATH. Skipping vPlan generation.'" - ) - return f"/usr/bin/env bash -c {shlex.quote(fallback)}" - - def format_opts(opts): - return " ".join(opts) if isinstance(opts, list) else str(opts) - - prepare_opts_str = format_opts(self.prepare_opts) - process_opts_str = format_opts(self.process_opts) - - prepare_cmd = f"dvplan prepare_vplan {prepare_opts_str} {self.ip_root} {self.vplan} {self.annotated_hjson}" - prepare_cmd = " ".join(prepare_cmd.split()) - - vendor_tool = f"{self.sim_cfg.tool}_report" - report_path = self.report_job.cov_report_dir - - process_cmd = ( - f"dvplan process_results {process_opts_str} --coverage {vendor_tool} {report_path} " - f"-R {self.gen_html} -s {self.sim_cfg.name} {self.dut_instance} {self.annotated_hjson}" - ) - process_cmd = " ".join(process_cmd.split()) - - full_command = f"set -e; mkdir -p {self.odir}; {prepare_cmd} && {process_cmd}" - return f"/usr/bin/env bash -c {shlex.quote(full_command)}" diff --git a/src/dvsim/report/dv_evidence.py b/src/dvsim/report/dv_evidence.py new file mode 100644 index 00000000..363d795c --- /dev/null +++ b/src/dvsim/report/dv_evidence.py @@ -0,0 +1,203 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Regression results in the tool-neutral `lowrisc-dv-evidence` format. + +dvplan defines the format, so a vPlan can be back-annotated from any regression flow and a person +can write one by hand. What dvsim writes here is a plain serialisation of what it already knows. + +Built from what the scheduler concludes about each job, through its completion hook. That is the +same state the JSON report is derived from, so the two cannot disagree about a run, and it is +available early enough for the vPlan job to read while the run is still going. +""" + +from enum import Enum +from importlib.metadata import PackageNotFoundError, version +from pathlib import Path + +from pydantic import BaseModel, ConfigDict, Field + +from dvsim.job.data import JobSpec, JobStatusInfo +from dvsim.job.status import JobStatus +from dvsim.logging import log +from dvsim.report.data import IPMeta +from dvsim.scheduler.core import ALL_FAILED_DEP, FAILED_DEP, KILLED_QUEUED, KILLED_SCHEDULED + +__all__ = ( + "EvidenceFile", + "Outcome", + "RunEvidenceCollector", + "run_outcome", + "write_evidence", +) + +# What the file calls itself. Named for the evidence it holds, which is test runs and manual +# inspections alike, rather than for either metric type +SCHEMA_ID = "lowrisc-dv-evidence" + +# The `target` the scheduler gives a job that runs a test. Builds and coverage jobs share the +# same result stream and are filtered out on this +RUN_TARGET = "run" + +# Reasons the scheduler reports for a job it cancelled rather than ran, imported rather than +# restated so a reworded message cannot silently stop matching +_CANCELLED_REASONS = frozenset( + reason.message for reason in (FAILED_DEP, ALL_FAILED_DEP, KILLED_SCHEDULED, KILLED_QUEUED) +) + + +class Outcome(Enum): + """How one run of a test ended, in the neutral format's vocabulary. + + There is no waived outcome: dvplan requires an owner and a date on a waiver, and a regression + can supply neither. A known failure is accepted there by recording an inspection instead. + """ + + PASSED = "passed" + FAILED = "failed" + KILLED = "killed" + NOT_RUN = "not_run" + + def __str__(self) -> str: + """Return the outcome as it appears in a results file.""" + return self.value + + +def run_outcome(status: JobStatus, reason: JobStatusInfo | None) -> Outcome: + """Map a job's terminal status onto the neutral format's vocabulary. + + `JobStatus.KILLED` covers both a job terminated while executing and one cancelled before it was + dispatched, which are different answers to "did this test run at all". The reason separates + them, and the scheduler records one against every job it completes. + """ + if status == JobStatus.PASSED: + return Outcome.PASSED + if status == JobStatus.FAILED: + return Outcome.FAILED + if reason is not None and reason.message in _CANCELLED_REASONS: + return Outcome.NOT_RUN + return Outcome.KILLED + + +class TestRun(BaseModel): + """One run of one test.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + __test__ = False # Named Test*, so pytest would otherwise collect it as a test class. + + status: Outcome + seed: int | None = None + log: Path | None = None + message: str | None = None + line: int | None = None + + +class EvidenceFile(BaseModel): + """A regression's results, in the tool-neutral evidence format. + + dvsim only ever fills the `testcase` half. The format also carries manual inspections, which a + person writes by hand. + """ + + model_config = ConfigDict(frozen=True, extra="forbid", populate_by_name=True) + + testcase: dict[str, list[TestRun]] + + schema_id: str = Field(default=SCHEMA_ID, alias="schema") + dut: str | None = None + tool: str | None = None + produced_by: str | None = None + revision: str | None = None + timestamp: str | None = None + + +class RunEvidenceCollector: + """Accumulates the outcome of every test run of one flow, as the scheduler concludes them. + + Fed by `Scheduler.add_job_completion_callback`, so a job cancelled before it ever started is + recorded too, and a test the plan expected reads as a hole rather than as an absent test. + """ + + def __init__(self) -> None: + """Start with nothing recorded. Runs arrive as the scheduler completes them.""" + self._runs: dict[str, list[TestRun]] = {} + + def record(self, spec: JobSpec, status: JobStatus, reason: JobStatusInfo | None) -> None: + """Record how one job ended, keeping only the ones that run a test. + + Grouped by job name, which is the name a vPlan addresses. Reseeds of one test share it and + are told apart by their seeds. + """ + if spec.target != RUN_TARGET: + return + failed = status != JobStatus.PASSED + self._runs.setdefault(spec.name, []).append( + TestRun( + status=run_outcome(status, reason), + seed=spec.seed, + log=spec.log_path, + message=reason.message if reason is not None and failed else None, + line=_first_line(reason) if failed else None, + ) + ) + + def evidence( + self, + *, + block: IPMeta, + tool: str | None = None, + timestamp: str | None = None, + ) -> EvidenceFile: + """Build the evidence document for everything recorded so far.""" + return EvidenceFile( + testcase=self._runs, + dut=block.variant_name(sep="/"), + tool=tool, + produced_by=_produced_by(), + revision=_revision(block), + timestamp=timestamp, + ) + + +def write_evidence(path: Path, evidence: EvidenceFile) -> Path: + """Write the evidence file, creating its directory if needed, and return its path. + + `IPMeta.url` is already stripped of credentials by `git_origin_url`, which matters because this + file is archived alongside the reports. + """ + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + evidence.model_dump_json(by_alias=True, indent=2, exclude_none=True), encoding="utf-8" + ) + log.debug("Wrote results for %d tests to '%s'", len(evidence.testcase), path) + return path + + +def _revision(block: IPMeta) -> str: + """Describe the revision the results were produced against, marking an uncommitted tree. + + Marked the way `sim.report` marks it, since the two describe the same run. This file outlives + the run, so it is the only chance to record it. + """ + revision = block.revision_info or block.url or block.commit + if block.dirty and "(dirty)" not in revision: + revision += " (dirty)" + return revision + + +def _produced_by() -> str: + """Name dvsim and its version, or just dvsim when it is not installed as a package.""" + try: + return f"dvsim {version('dvsim').strip()}" + except PackageNotFoundError: + log.debug("DVSim package not found, so its version is left out of the results") + return "dvsim" + + +def _first_line(reason: JobStatusInfo | None) -> int | None: + """Get the first log line a failure was reported at, where one was recorded.""" + if reason is None or not reason.lines: + return None + first = reason.lines[0] + return first if isinstance(first, int) else first[0] diff --git a/src/dvsim/report/vplan.py b/src/dvsim/report/vplan.py new file mode 100644 index 00000000..f3704b8c --- /dev/null +++ b/src/dvsim/report/vplan.py @@ -0,0 +1,172 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Back-annotate a DVPlan verification plan from a finished regression. + +This module builds the command; `job.deploy.CovVPlan` runs it as a scheduled job, so the step gets +its own row in the job status table alongside build, run, cov_merge and cov_report. + +Nothing happens at all unless the sim cfg names a `vplan`. +""" + +import glob +import shlex +import shutil +from collections.abc import Sequence +from dataclasses import dataclass, field +from pathlib import Path + +import hjson + +from dvsim.logging import log + +__all__ = ("VPlanInputs", "overall_coverage", "shell_command") + +# Scratch subdirectory the annotated plan and its report are written to. Unchanged, so an existing +# link to the report still resolves +VPLAN_DIR = "cov_vplan" + +ANNOTATED_HJSON = "vplan_annotated.hjson" +ANNOTATED_HTML = "vplan_annotated.html" +EVIDENCE_JSON = "dv_evidence.json" + + +@dataclass(frozen=True) +class VPlanInputs: + """Everything the annotation needs, so this module never reaches back into a flow config.""" + + vplan: Path + """The verification plan to annotate.""" + out_dir: Path + """Where the annotated plan, its report and the evidence file are written.""" + dut_entity: str + """Name of the DUT entity, as a vPlan addresses it.""" + dut_instance: str + """Hierarchical path to the DUT in the testbench, such as `tb.dut`.""" + cov_report_dir: Path | None + """The vendor coverage report to annotate from, if the run produced one.""" + tool: str + """Simulator name, which selects the vendor report format.""" + inspect: str = "" + """Where hand-written inspection records live, if the cfg names any. A path or a glob.""" + prepare_opts: list[str] = field(default_factory=list) + process_opts: list[str] = field(default_factory=list) + + @property + def annotated(self) -> Path: + """Where the annotated plan is written.""" + return self.out_dir / ANNOTATED_HJSON + + @property + def report(self) -> Path: + """Where the plan's HTML report is written.""" + return self.out_dir / ANNOTATED_HTML + + @property + def evidence(self) -> Path: + """Where the regression's evidence file is written, and read back from.""" + return self.out_dir / EVIDENCE_JSON + + +def shell_command(inputs: VPlanInputs) -> str: + """Build the bash command that prepares and annotates the vPlan. + + Returned as one `bash -c` string because a scheduled job runs a shell command. `set -e` and the + `&&` mean a broken annotation shows as a failed job rather than a silently missing score. + """ + if shutil.which("dvplan") is None: + # Warn and pass, so a checkout without dvplan does not fail every regression naming a vPlan + warning = "WARNING: dvplan is not installed on PATH. Skipping vPlan annotation." + return f"/usr/bin/env bash -c {shlex.quote(f'echo {shlex.quote(warning)}')}" + + # The vPlan sits at //, so its grandparent is the IP root that + # `prepare_vplan` traces specifications against. + ip_root = inputs.vplan.parent.parent + prepare = [ + "dvplan", + "prepare_vplan", + *_opts(inputs.prepare_opts), + str(ip_root), + str(inputs.vplan), + str(inputs.annotated), + ] + process = _process_command(inputs) + + script = ( + f"set -e; mkdir -p {shlex.quote(str(inputs.out_dir))}; " + f"{shlex.join(prepare)} && {shlex.join(process)}" + ) + return f"/usr/bin/env bash -c {shlex.quote(script)}" + + +def _process_command(inputs: VPlanInputs) -> list[str]: + """Build the `process_results` invocation, with every coverage source it should read. + + Every source goes to one invocation on purpose: dvplan writes an item off as unmeasurable only + when none of the sources given to it can measure its field, so a second run would find the + items only its own source answers for already written off. + """ + coverage: list[str] = [] + if inputs.cov_report_dir: + coverage += ["--coverage", f"{inputs.tool}_report", str(inputs.cov_report_dir)] + # One source for both: dvplan reads the testcase and inspection metrics out of the same format. + # A glob is expanded here because the argv is built directly, with no shell to do it + evidence = [str(inputs.evidence)] + if inputs.inspect: + evidence += _expand(inputs.inspect) + coverage += ["--coverage", "dv_evidence", *evidence] + return [ + "dvplan", + "process_results", + *_opts(inputs.process_opts), + *coverage, + "-R", + str(inputs.report), + "-s", + inputs.dut_entity, + inputs.dut_instance, + str(inputs.annotated), + ] + + +def _opts(opts: Sequence[str]) -> list[str]: + """Split cfg-supplied options into argv entries, dropping empty ones. + + Two shapes turn up in real cfgs that a bare splat would pass to dvplan as literal arguments: + `[""]` for "none", which argparse reads as an empty positional, and `["--milestone-depth 1"]` + written as one string, which it reads as a single unknown flag. Split the way a shell would, so + an option carrying a quoted value stays one argument. + """ + return [token for opt in opts for token in shlex.split(opt)] + + +def _expand(pattern: str) -> list[str]: + """Expand an inspection path, which may be a file, a directory or a glob pattern. + + A cfg naming inspections through `{proj_root}` always produces an absolute pattern, which + `Path.glob` refuses, so this is one of the places the pathlib rule does not apply. + """ + matches = sorted(glob.glob(pattern)) # noqa: PTH207 (Path.glob rejects an absolute pattern) + if not matches: + log.warning("No inspection records matched '%s', so none were annotated from.", pattern) + return matches or [pattern] + + +def overall_coverage(annotated: Path) -> float | None: + """Read the plan's overall normalised coverage back out of the annotated vPlan.""" + if not annotated.is_file(): + log.warning("No annotated vPlan at '%s', so its score is not reported.", annotated) + return None + try: + with annotated.open(encoding="utf-8") as f: + data = hjson.load(f) + # An HJSON vPlan is keyed by DUT name: {dut_name: {fields...}}. + root = next(iter(data.values()), {}) + raw = root.get("Normalized_Coverage") + if raw is None: + return None + return float(str(raw).rstrip(" %")) + except (OSError, ValueError, AttributeError, hjson.HjsonDecodeError): + log.exception("Could not read the vPlan score from '%s'.", annotated) + return None diff --git a/src/dvsim/sim/flow.py b/src/dvsim/sim/flow.py index 6dc400c4..4b8975a5 100644 --- a/src/dvsim/sim/flow.py +++ b/src/dvsim/sim/flow.py @@ -15,7 +15,7 @@ from typing import ClassVar from dvsim.flow.base import FlowCfg -from dvsim.job.data import CompletedJobStatus, JobSpec +from dvsim.job.data import CompletedJobStatus, JobSpec, JobStatusInfo from dvsim.job.deploy import ( CompileSim, CovAnalyze, @@ -29,6 +29,7 @@ from dvsim.logging import log from dvsim.modes import BuildMode, Mode, RunMode, find_mode from dvsim.regression import Regression +from dvsim.report.dv_evidence import RunEvidenceCollector from dvsim.sim.data import ( IPMeta, SimFlowResults, @@ -158,10 +159,15 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: self.cov_report_dir = "" self.cov_report_page = "" - # Options for vPlan processing + # Options for vPlan processing. Extracted from the hjson cfg, and declared here so a type + # checker knows they exist. Nothing happens unless `vplan` names a plan + self.vplan: str = "" # dut_instance is the hierarchical testbench path to the DUT (e.g. "tb.dut"), # distinct from `name`/`qual_name` which identify the sim config itself. self.dut_instance = "" + # A file, a directory of them, or a glob holding hand-written dvplan inspection records. + # No regression produces these, so dvsim only passes the path on + self.dvplan_inspect = "" self.cov_vplan_prepare_opts = [] self.cov_vplan_process_opts = [] @@ -178,6 +184,10 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: self.run_list = [] self.cov_merge_deploy = None self.cov_report_deploy = None + self.cov_vplan_deploy = None + # Filled in by the scheduler's completion hook, and read by the vPlan job once every run it + # depends on is terminal + self.run_evidence = RunEvidenceCollector() self.results_summary = OrderedDict() super().__init__(flow_cfg_file, hjson_data, args, mk_config) @@ -560,9 +570,12 @@ def _create_deploy_objects(self) -> None: self.cov_report_deploy = CovReport(self.cov_merge_deploy, self) self.deploy += [self.cov_merge_deploy, self.cov_report_deploy] - if getattr(self, "vplan", False): - self.cov_vplan_deploy = CovVPlan(self.cov_report_deploy, self) - self.deploy.append(self.cov_vplan_deploy) + if self.vplan and self.runs: + # Depends on the coverage report where there is one, so the vendor report exists + # to annotate from, and otherwise straight on the runs it scores. + vplan_deps = [self.cov_report_deploy] if self.cov_report_deploy else self.runs + self.cov_vplan_deploy = CovVPlan(vplan_deps, self) + self.deploy.append(self.cov_vplan_deploy) def _cov_analyze(self) -> None: """Open GUI tool for coverage analysis. @@ -684,6 +697,45 @@ def gen_results(self, results: Sequence[CompletedJobStatus]) -> None: path=reports_dir, ) + def on_job_completed( + self, spec: JobSpec, status: JobStatus, reason: JobStatusInfo | None + ) -> None: + """Record each run's outcome as the scheduler concludes it. + + Fed from the scheduler rather than from a job callback, because only the scheduler sees a job + it cancelled before dispatching it. + """ + self.run_evidence.record(spec, status, reason) + + def run_timestamp(self) -> datetime: + """Return when this run started, as an aware datetime. + + `self.timestamp` is a `TS_FORMAT` string, which is a dvsim convention no consumer of a report + can be expected to parse, so everything written out of this flow goes through here. + """ + return datetime.strptime(self.timestamp, TS_FORMAT).replace(tzinfo=timezone.utc) + + def block_meta(self, url: str | None = None) -> IPMeta: + """Describe the design under test, for anything this flow writes about the run. + + Shared by the reports and the vPlan evidence file, so a run cannot say it came from a clean + tree in one artefact and a dirty one in another. + + Args: + url: link to the IP in git, or None to derive it from the checkout. + + """ + return IPMeta( + name=self.name.lower(), + variant=(self.variant or "").lower() or None, + commit=self.commit, + commit_short=self.commit_short, + dirty=self.dirty, + branch=self.branch or "", + url=url if url is not None else (git_https_url_with_commit(path=Path(self.proj_root))), + revision_info=self.revision, + ) + def _gen_json_results( self, run_results: Sequence[CompletedJobStatus], @@ -704,18 +756,8 @@ def _gen_json_results( self.testplan.map_test_results(sim_results.table) # --- Metadata --- - timestamp = datetime.strptime(self.timestamp, TS_FORMAT).replace(tzinfo=timezone.utc) - - block = IPMeta( - name=self.name.lower(), - variant=(self.variant or "").lower() or None, - commit=self.commit, - commit_short=self.commit_short, - dirty=self.dirty, - branch=self.branch or "", - url=url, - revision_info=self.revision, - ) + timestamp = self.run_timestamp() + block = self.block_meta(url=url) tool = ToolMeta(name=self.tool.lower(), version="unknown") build_seed = self.build_seed if not self.run_only else None @@ -844,8 +886,8 @@ def make_test_result(tr) -> TestResult | None: vplan_report_page = None vplan_coverage = None - if getattr(self, "cov_vplan_deploy", None): - vplan_report_page = Path(self.scratch_path) / CovVPlan.target / "vplan_annotated.html" + if self.cov_vplan_deploy is not None: + vplan_report_page = self.cov_vplan_deploy.report_page vplan_coverage = self.cov_vplan_deploy.vplan_coverage failures = BucketedFailures.from_job_status(results=run_results) diff --git a/tests/job/test_cov_vplan.py b/tests/job/test_cov_vplan.py new file mode 100644 index 00000000..24b6fb78 --- /dev/null +++ b/tests/job/test_cov_vplan.py @@ -0,0 +1,114 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for the vPlan back-annotation job. + +These construct the job for real. Every other test around the vPlan exercises a helper in +isolation, which cannot catch the job failing to build itself: `Deploy.__init__` derives +attributes by name and order, so a missing one is an `AttributeError` at config time that no +amount of testing the command builder would reveal. +""" + +from pathlib import Path +from types import SimpleNamespace + +import pytest +from hamcrest import assert_that, contains_string, equal_to, is_, none + +from dvsim.job.data import WorkspaceConfig +from dvsim.job.deploy import CovVPlan +from dvsim.job.status import JobStatus +from dvsim.report.vplan import ANNOTATED_HJSON, ANNOTATED_HTML, VPLAN_DIR + + +def _cfg(**overrides: object) -> SimpleNamespace: + """Build the smallest sim cfg a `CovVPlan` can be constructed against.""" + attrs: dict[str, object] = { + "name": "hmac", + "flow": "sim", + "variant": "", + "tool": "xcelium", + "gui": False, + "interactive": False, + "dry_run": False, + "scratch_path": "/scratch/hmac", + "commit": "abc123", + "commit_short": "abc", + "branch": "main", + "revision": "", + "build_mode": "default", + "exports": [], + "flow_makefile": "sim.mk", + "proj_root": "/proj", + "vplan": "/proj/hw/ip/hmac/data/hmac_vplan.hjson", + "dut_instance": "tb.dut", + "dvplan_inspect": "", + "cov": True, + "cov_report_dir": "/scratch/hmac/cov_report", + "cov_vplan_prepare_opts": ["--bypass-trace"], + "cov_vplan_process_opts": [""], + "timeout_mins": None, + "max_odirs": 5, + "workspace_cfg": WorkspaceConfig( + timestamp="20260818_090000", + project_root=Path("/proj"), + scratch_root=Path("/scratch"), + scratch_path=Path("/scratch/hmac"), + ), + } + attrs.update(overrides) + return SimpleNamespace(**attrs) + + +@pytest.fixture +def job(monkeypatch: pytest.MonkeyPatch) -> CovVPlan: + """A constructed job, with dvplan present so the real command is built.""" + monkeypatch.setattr("dvsim.report.vplan.shutil.which", lambda _: "/usr/bin/dvplan") + return CovVPlan([], _cfg()) + + +def test_the_job_constructs_and_lands_in_its_own_output_directory(job: CovVPlan) -> None: + """`Deploy` derives `odir` from an attribute named after the target, inside `_set_attrs`. + + That ordering is easy to get wrong and fails at config time rather than at run time, taking + the whole invocation down before a single test starts. + """ + assert_that(job.odir, equal_to(f"/scratch/hmac/{VPLAN_DIR}")) + assert_that(job.qual_name, equal_to("cov_vplan")) + assert_that(job.full_name, equal_to("hmac:cov_vplan")) + assert_that(job.annotated_hjson, equal_to(Path("/scratch/hmac") / VPLAN_DIR / ANNOTATED_HJSON)) + assert_that(job.report_page, equal_to(Path("/scratch/hmac") / VPLAN_DIR / ANNOTATED_HTML)) + + +def test_the_job_builds_a_runnable_command(job: CovVPlan) -> None: + """The command is built during construction, so a broken builder is a config-time failure.""" + assert_that(job.cmd, contains_string("dvplan prepare_vplan --bypass-trace")) + assert_that(job.cmd, contains_string("dvplan process_results")) + assert_that(job.cmd, contains_string("--coverage xcelium_report")) + assert_that(job.cmd, contains_string("--coverage dv_evidence")) + # `cov_vplan_process_opts: [""]` is idiomatic hjson for "none" and must not become an + # empty argument, which argparse would read as the DUT name. + assert_that(job.cmd, contains_string("-s hmac tb.dut")) + + +def test_a_run_without_coverage_still_annotates(monkeypatch: pytest.MonkeyPatch) -> None: + """Without --cov there is no vendor report, and the plan is scored from the evidence alone.""" + monkeypatch.setattr("dvsim.report.vplan.shutil.which", lambda _: "/usr/bin/dvplan") + + job = CovVPlan([], _cfg(cov=False)) + + assert_that(job.cmd, contains_string("--coverage dv_evidence")) + assert_that("xcelium_report" in job.cmd, is_(False)) + + +def test_a_failing_run_still_gets_its_plan_scored(job: CovVPlan) -> None: + """A failed test is still evidence, so the job must not be skipped when a dependency fails.""" + assert_that(job.needs_all_dependencies_passing, is_(False)) + + +def test_no_score_is_read_back_when_the_job_did_not_pass(job: CovVPlan) -> None: + """Reading a plan the job failed to write would report a stale or partial figure.""" + job.post_finish()(JobStatus.FAILED) + + assert_that(job.vplan_coverage, is_(none())) diff --git a/tests/report/test_dv_evidence.py b/tests/report/test_dv_evidence.py new file mode 100644 index 00000000..fbf98fef --- /dev/null +++ b/tests/report/test_dv_evidence.py @@ -0,0 +1,272 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for the tool-neutral evidence file written for vPlan back-annotation. + +The format is a contract with dvplan and with any other flow that consumes it, so these cover +what the file says as much as how it is built: which job statuses map onto which outcome, and that a +run the scheduler cancelled is still reported rather than dropped. +""" + +import json +from pathlib import Path +from types import SimpleNamespace + +import pytest +from hamcrest import assert_that, contains_string, equal_to, has_key, is_, none, not_ + +from dvsim.job.data import JobSpec, JobStatusInfo, WorkspaceConfig +from dvsim.job.status import JobStatus +from dvsim.report.data import IPMeta, ToolMeta +from dvsim.report.dv_evidence import ( + SCHEMA_ID, + Outcome, + RunEvidenceCollector, + run_outcome, + write_evidence, +) +from dvsim.scheduler.core import ( + ALL_FAILED_DEP, + FAILED_DEP, + KILLED_QUEUED, + KILLED_RUNNING_SIGTERM, + KILLED_SCHEDULED, + OnJobCompletionCb, +) +from dvsim.sim.flow import SimCfg + +_BLOCK = IPMeta( + name="hmac", + variant=None, + commit="abc123", + commit_short="abc", + branch="main", + url="https://github.com/lowRISC/mocha/tree/abc123", + revision_info=None, +) +_TOOL = "xcelium" +_WORKSPACE = WorkspaceConfig( + timestamp="20260813_060029", + project_root=Path("/proj"), + scratch_root=Path("/scratch"), + scratch_path=Path("/scratch/hmac"), +) + + +def _spec( + name: str, + *, + seed: int | None = 0, + target: str = "run", +) -> JobSpec: + """Build the job spec the scheduler hands an observer when a job completes.""" + return JobSpec( + name=name, + job_type="RunTest", + target=target, + backend=None, + resources=None, + seed=seed, + full_name=f"hmac:{seed}.{name}", + qual_name=f"{seed}.{name}", + block=_BLOCK, + tool=ToolMeta(name=_TOOL, version="unknown"), + workspace_cfg=_WORKSPACE, + dependencies=[], + needs_all_dependencies_passing=True, + weight=1, + timeout_mins=None, + cmd="make run", + exports={}, + dry_run=False, + interactive=False, + odir=f"/scratch/hmac/{seed}.{name}", + renew_odir=True, + log_path=Path(f"/scratch/hmac/{seed}.{name}/run.log"), + pre_launch=lambda: None, + post_finish=lambda _s: None, + pass_patterns=[], + fail_patterns=[], + ) + + +def _collect(*records: tuple[JobSpec, JobStatus, JobStatusInfo | None]) -> RunEvidenceCollector: + """Feed a collector the way the scheduler's completion hook does.""" + collector = RunEvidenceCollector() + for spec, status, reason in records: + collector.record(spec, status, reason) + return collector + + +def _evidence(*records: tuple[JobSpec, JobStatus, JobStatusInfo | None]): + """Build the evidence document for a set of completed jobs.""" + return _collect(*records).evidence(block=_BLOCK, tool=_TOOL, timestamp="2026-08-13T06:00:29Z") + + +@pytest.mark.parametrize( + ("status", "reason", "expected"), + [ + (JobStatus.PASSED, None, Outcome.PASSED), + (JobStatus.FAILED, JobStatusInfo(message="UVM_ERROR"), Outcome.FAILED), + # Killed while executing is a different answer from never having started. + (JobStatus.KILLED, KILLED_RUNNING_SIGTERM, Outcome.KILLED), + (JobStatus.KILLED, None, Outcome.KILLED), + (JobStatus.KILLED, FAILED_DEP, Outcome.NOT_RUN), + (JobStatus.KILLED, ALL_FAILED_DEP, Outcome.NOT_RUN), + (JobStatus.KILLED, KILLED_SCHEDULED, Outcome.NOT_RUN), + (JobStatus.KILLED, KILLED_QUEUED, Outcome.NOT_RUN), + ], + ids=[ + "passed", + "failed", + "killed_running", + "killed_no_reason", + "dep_failed", + "all_deps_failed", + "dep_killed", + "killed_queued", + ], +) +def test_job_status_maps_onto_the_neutral_vocabulary( + status: JobStatus, reason: JobStatusInfo | None, expected: Outcome +) -> None: + """`JobStatus.KILLED` covers two outcomes, and the scheduler's reason separates them. + + The cancel reasons are imported from `scheduler.core` rather than restated, so rewording one + of them fails here instead of silently reclassifying every cancelled run as `killed`. + """ + assert_that(run_outcome(status, reason), is_(expected)) + + +def test_a_cancelled_run_is_reported_rather_than_dropped() -> None: + """A run the scheduler cancelled is a hole in the plan, so it has to appear as `not_run`. + + Dropping it would leave the test looking like it passed everything it attempted, when the + plan expected a run that never happened. + """ + evidence = _evidence( + (_spec("hmac_smoke", seed=0), JobStatus.PASSED, None), + (_spec("hmac_smoke", seed=1), JobStatus.KILLED, FAILED_DEP), + ) + + statuses = [run.status for run in evidence.testcase["hmac_smoke"]] + assert_that(statuses, equal_to([Outcome.PASSED, Outcome.NOT_RUN])) + + +def test_runs_are_grouped_by_test_name() -> None: + """Reseeds of one test share a name, which is what a vPlan addresses them by.""" + evidence = _evidence( + (_spec("hmac_smoke", seed=0), JobStatus.PASSED, None), + (_spec("hmac_smoke", seed=1), JobStatus.PASSED, None), + (_spec("hmac_stress", seed=2), JobStatus.PASSED, None), + ) + + assert_that(sorted(evidence.testcase), equal_to(["hmac_smoke", "hmac_stress"])) + assert_that(len(evidence.testcase["hmac_smoke"]), equal_to(2)) + assert_that([run.seed for run in evidence.testcase["hmac_smoke"]], equal_to([0, 1])) + + +def test_only_run_jobs_are_tests() -> None: + """Builds and coverage jobs share the result stream and are not tests. + + They carry names of their own, so including them would invent testcase items a vPlan could + never have asked for. + """ + evidence = _evidence( + (_spec("hmac_smoke", target="run"), JobStatus.PASSED, None), + (_spec("default", target="build"), JobStatus.PASSED, None), + (_spec("cov_merge", target="cov_merge"), JobStatus.PASSED, None), + ) + + assert_that(list(evidence.testcase), equal_to(["hmac_smoke"])) + + +def test_a_failing_run_records_what_reproduces_and_explains_it() -> None: + """A failing run carries the seed, the log and the failure somebody needs to read.""" + reason = JobStatusInfo(message="UVM_ERROR digest mismatch", lines=[481]) + evidence = _evidence((_spec("hmac_smoke", seed=7), JobStatus.FAILED, reason)) + + run = evidence.testcase["hmac_smoke"][0] + assert_that(run.status, is_(Outcome.FAILED)) + assert_that(run.seed, equal_to(7)) + assert_that(run.log, equal_to(Path("/scratch/hmac/7.hmac_smoke/run.log"))) + assert_that(run.message, equal_to("UVM_ERROR digest mismatch")) + assert_that(run.line, equal_to(481)) + + +def test_a_passing_run_records_no_failure() -> None: + """Failure detail is only meaningful for a run that did not pass.""" + evidence = _evidence((_spec("hmac_smoke"), JobStatus.PASSED, JobStatusInfo(message="ignored"))) + + assert_that(evidence.testcase["hmac_smoke"][0].message, is_(none())) + + +def test_written_results_name_their_schema_and_provenance(tmp_path: Path) -> None: + """The file says what it is and where it came from, which is what makes it auditable later.""" + evidence = _evidence( + (_spec("hmac_smoke", seed=0), JobStatus.PASSED, None), + (_spec("hmac_smoke", seed=1), JobStatus.FAILED, JobStatusInfo(message="boom")), + ) + + path = write_evidence(tmp_path / "reports" / "dv_evidence.json", evidence) + written = json.loads(path.read_text(encoding="utf-8")) + + assert_that(written["schema"], equal_to(SCHEMA_ID)) + assert_that(written["dut"], equal_to("hmac")) + assert_that(written["tool"], equal_to(_TOOL)) + assert_that(written["produced_by"], contains_string("dvsim")) + assert_that(written["testcase"], has_key("hmac_smoke")) + # A test maps straight to its runs, with no wrapper object in between. + statuses = [run["status"] for run in written["testcase"]["hmac_smoke"]] + assert_that(statuses, equal_to(["passed", "failed"])) + + +def test_the_written_provenance_records_a_dirty_tree(tmp_path: Path) -> None: + """A vPlan figure produced from uncommitted work must not read as coming from the commit. + + dvsim's own report marks the revision '(dirty)', so an evidence file that dropped the flag + would disagree with the report for the same run, and the disagreement would only show up + when somebody went back to reproduce the figure. + """ + collector = _collect((_spec("hmac_smoke"), JobStatus.PASSED, None)) + clean = write_evidence(tmp_path / "clean.json", collector.evidence(block=_BLOCK, tool=_TOOL)) + dirty = write_evidence( + tmp_path / "dirty.json", + collector.evidence(block=_BLOCK.model_copy(update={"dirty": True}), tool=_TOOL), + ) + + # Same block either way, so only the flag can account for the difference. + assert_that( + json.loads(clean.read_text(encoding="utf-8"))["revision"], not_(contains_string("dirty")) + ) + assert_that( + json.loads(dirty.read_text(encoding="utf-8"))["revision"], contains_string("(dirty)") + ) + + +def test_the_collector_is_reachable_from_the_scheduler_hook() -> None: + """The collector's method has to match the callback the scheduler will call it through. + + Wiring it up is the one part unit tests would otherwise miss entirely: a signature drift + here surfaces only at the end of a real regression, when the vPlan job reads an empty file. + """ + collector = RunEvidenceCollector() + scheduler_cb: OnJobCompletionCb = collector.record + + scheduler_cb(_spec("hmac_smoke", seed=3), JobStatus.PASSED, None) + + assert_that(collector.evidence(block=_BLOCK).testcase, has_key("hmac_smoke")) + + +def test_a_flow_forwards_completions_to_its_collector() -> None: + """`SimCfg.on_job_completed` is the override the scheduler drives, so it has to forward. + + Called unbound against a stand-in, since constructing a real `SimCfg` needs a whole hjson cfg + and none of it bears on the forwarding. + """ + flow = SimpleNamespace(run_evidence=RunEvidenceCollector()) + + SimCfg.on_job_completed(flow, _spec("hmac_smoke", seed=3), JobStatus.PASSED, None) + + assert_that(flow.run_evidence.evidence(block=_BLOCK).testcase, has_key("hmac_smoke")) diff --git a/tests/report/test_vplan.py b/tests/report/test_vplan.py new file mode 100644 index 00000000..1537db0b --- /dev/null +++ b/tests/report/test_vplan.py @@ -0,0 +1,186 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for back-annotating a DVPlan verification plan after a regression. + +The command built here is the interface to another tool, whose positional shape is a fixed +contract, so it is checked as carefully as anything that runs in this process. The rest covers +the promise that no vPlan problem can fail a regression that otherwise passed. +""" + +import logging +from dataclasses import replace +from pathlib import Path + +import pytest +from hamcrest import assert_that, contains_string, equal_to, is_, none + +from dvsim.report.vplan import ( + ANNOTATED_HJSON, + VPlanInputs, + _expand, + _process_command, + overall_coverage, + shell_command, +) + + +def _inputs(tmp_path: Path, **overrides: object) -> VPlanInputs: + """Build the inputs for one annotation, overriding whatever a test cares about.""" + base = VPlanInputs( + vplan=tmp_path / "hw" / "ip" / "hmac" / "doc" / "hmac_vplan.hjson", + out_dir=tmp_path / "out", + dut_entity="hmac", + dut_instance="tb.dut", + cov_report_dir=Path("/scratch/hmac/cov_report"), + tool="xcelium", + ) + return replace(base, **overrides) + + +def test_the_command_keeps_dvplan_s_positional_contract(tmp_path: Path) -> None: + """`process_results` takes its three positionals last, with `-s` as a flag before them. + + dvplan documents this shape as fixed because dvsim builds it. Getting `-s` wrong is the + error that reads as `--summary` swallowing the DUT name, so it is pinned here rather than + discovered in a nightly. + """ + inputs = _inputs(tmp_path) + + command = _process_command(inputs) + + assert_that(command[:2], equal_to(["dvplan", "process_results"])) + # -s is a switch, and the three positionals follow it in order. + assert_that(command[-4:], equal_to(["-s", "hmac", "tb.dut", str(inputs.annotated)])) + + +def test_every_coverage_source_reaches_one_invocation(tmp_path: Path) -> None: + """The vendor report, the test results and the inspections annotate in a single run. + + dvplan writes a plan item off as unmeasurable only when none of the sources it was given can + measure the item's field, so splitting these would lose whichever metric the first run lacked. + """ + inspections = tmp_path / "inspections" + inspections.mkdir() + command = _process_command(_inputs(tmp_path, inspect=str(inspections))) + + joined = " ".join(command) + assert_that(joined, contains_string("--coverage xcelium_report /scratch/hmac/cov_report")) + assert_that(joined, contains_string("--coverage dv_evidence")) + # One source, so the inspections ride along with the evidence rather than repeating the flag. + assert_that(joined, contains_string(str(inspections))) + assert_that(joined.count("--coverage"), equal_to(2)) + + +def test_the_vendor_report_is_left_out_when_there_is_none(tmp_path: Path) -> None: + """Without coverage the plan is still annotated, from the recorded evidence alone.""" + command = _process_command(_inputs(tmp_path, cov_report_dir=None)) + + assert_that(" ".join(command), contains_string("--coverage dv_evidence")) + assert_that("xcelium_report" in " ".join(command), is_(False)) + + +def test_an_absolute_inspection_glob_expands(tmp_path: Path) -> None: + """A cfg names inspections through `{proj_root}`, so the pattern is always absolute. + + `Path().glob` rejects an absolute pattern outright, so getting this wrong raises rather than + degrading, which would take a passing regression down with it. + """ + for name in ("reset", "security"): + (tmp_path / f"{name}.inspect.json").write_text("{}", encoding="utf-8") + + matches = _expand(str(tmp_path / "*.inspect.json")) + + assert_that( + matches, + equal_to([str(tmp_path / "reset.inspect.json"), str(tmp_path / "security.inspect.json")]), + ) + + +def test_a_directory_of_inspections_is_passed_through(tmp_path: Path) -> None: + """A path that exists needs no expansion, since dvplan reads a directory itself.""" + folder = tmp_path / "inspections" + folder.mkdir() + + assert_that(_expand(str(folder)), equal_to([str(folder)])) + + +def test_a_pattern_matching_nothing_warns_and_is_left_alone( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """Silently dropping the source would read as the cfg not naming one at all. + + The fixture's handler is attached to the 'dvsim' logger by hand, because that logger sets + `propagate = False` and so never reaches the root handler `caplog` installs. + """ + pattern = str(tmp_path / "nothing" / "*.json") + dvsim_log = logging.getLogger("dvsim") + dvsim_log.addHandler(caplog.handler) + try: + assert_that(_expand(pattern), equal_to([pattern])) + finally: + dvsim_log.removeHandler(caplog.handler) + + assert_that(caplog.text, contains_string("No inspection records matched")) + + +@pytest.mark.parametrize( + ("content", "expected"), + [ + ('{hmac: {Normalized_Coverage: "82.5%"}}', 82.5), + ("{hmac: {Normalized_Coverage: 82.5}}", 82.5), + # A plan with no score annotated yet is not an error, it simply has none to report. + ("{hmac: {Description: nothing scored}}", None), + ("{}", None), + ("not hjson at all {{{", None), + ], + ids=["percent_string", "bare_number", "no_score", "empty", "malformed"], +) +def test_the_overall_score_is_read_back_or_reported_as_absent( + tmp_path: Path, content: str, expected: float | None +) -> None: + """The score is quoted in the flow's report, so an unreadable plan must not raise.""" + annotated = tmp_path / ANNOTATED_HJSON + annotated.write_text(content, encoding="utf-8") + + assert_that(overall_coverage(annotated), equal_to(expected)) + + +def test_a_missing_annotated_plan_reports_no_score(tmp_path: Path) -> None: + """Nothing was produced, so there is nothing to quote and nothing to raise about.""" + assert_that(overall_coverage(tmp_path / "absent.hjson"), is_(none())) + + +def test_a_missing_dvplan_still_produces_a_runnable_command( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A checkout without dvplan installed must not fail every regression that names a vPlan. + + The job still has to run something, so it warns and passes rather than erroring. + """ + monkeypatch.setattr("dvsim.report.vplan.shutil.which", lambda _: None) + + command = shell_command(_inputs(tmp_path)) + + assert_that(command, contains_string("bash -c")) + assert_that(command, contains_string("WARNING")) + assert_that("dvplan process_results" in command, is_(False)) + + +def test_the_command_fails_the_job_when_dvplan_does( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A broken annotation shows as a failed job rather than a silently missing score. + + `set -e` and the `&&` are what carry a non-zero exit out to the scheduler, so they are + checked rather than assumed. + """ + monkeypatch.setattr("dvsim.report.vplan.shutil.which", lambda _: "/usr/bin/dvplan") + + command = shell_command(_inputs(tmp_path)) + + assert_that(command, contains_string("set -e")) + assert_that(command, contains_string("prepare_vplan")) + assert_that(command, contains_string("&&")) + assert_that(command, contains_string("process_results")) From 258e394fcf46ea6a6b2d178edc8ee515811540ff Mon Sep 17 00:00:00 2001 From: martin-velay Date: Tue, 18 Aug 2026 16:23:05 +0200 Subject: [PATCH 4/5] fix: score the vPlan whatever the regression concluded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for the vPlan back-annotation series. The plan could not be scored in the regression it most needs to describe. `needs_all_dependencies_passing` had two states and this job needs a third, so it becomes `DependencyPolicy`. `ALL_PASSING` is the default and `ANY_PASSING` is what CovMerge always did, so neither changes behaviour. CovVPlan takes `ALWAYS` and runs once its dependencies are terminal, whatever they concluded. Under either existing policy a regression where nothing passed was killed rather than scored, and with --cov the job has a single dependency, so anything that stopped the coverage report also stopped the plan. dvsim now defines the evidence format rather than deferring to dvplan. doc/dv_evidence.md specifies it and the pydantic models are normative. dvsim produces the file and is the public repo, so a consumer can be written against a spec rather than against whichever tool was built first. Whether dvplan is installed is decided by the job's own script, so it reads the PATH of the machine the job lands on rather than that of the host dvsim was launched from, which on a compute farm need not be the same. A `dvplan_inspect` pattern matching nothing is now a config error. The command is built while the jobs are, so it stops the run in seconds rather than failing inside dvplan once the regression has already gone. The vPlan report page is linked only once it exists, since a killed job or a machine without dvplan otherwise left a dead link in the HTML report. AI-assisted (Claude Code) — reviewed and approved by author Signed-off-by: martin-velay --- README.md | 1 + doc/dv_evidence.md | 92 ++++++++++++++++++++++++++++++++ src/dvsim/job/data.py | 23 +++++++- src/dvsim/job/deploy.py | 20 +++---- src/dvsim/report/dv_evidence.py | 16 +++--- src/dvsim/report/vplan.py | 32 +++++++---- src/dvsim/scheduler/core.py | 7 ++- src/dvsim/sim/flow.py | 5 +- tests/job/test_cov_vplan.py | 29 ++++++---- tests/report/test_dv_evidence.py | 4 +- tests/report/test_vplan.py | 51 +++++++----------- tests/test_scheduler.py | 45 ++++++++++++---- 12 files changed, 241 insertions(+), 84 deletions(-) create mode 100644 doc/dv_evidence.md diff --git a/README.md b/README.md index 22a6ce5f..8bb4f579 100644 --- a/README.md +++ b/README.md @@ -86,6 +86,7 @@ You can access it [online at opentitan.org/book/](https://opentitan.org/book/). * [Testplanner tool](./doc/testplanner.md) * [Design document](./doc/design_doc.md) +* [The `lowrisc-dv-evidence` format](./doc/dv_evidence.md) * [Glossary](./doc/glossary.md) ## How to contribute diff --git a/doc/dv_evidence.md b/doc/dv_evidence.md new file mode 100644 index 00000000..83bf7de3 --- /dev/null +++ b/doc/dv_evidence.md @@ -0,0 +1,92 @@ + +# The `lowrisc-dv-evidence` format + +A regression tells you which tests passed. +A verification plan asks a different question: of everything we said we would verify, how much is now backed by something that ran? +Answering it needs the regression's own outcomes in a form a planning tool can read, rather than a log directory and a human. + +This is that form. +DVSim writes one of these files per simulation flow, and it is the definition of the format rather than a description of one tool's output. +Anything that can produce it can be scored against a verification plan, whether or not it is DVSim. + +## Where DVSim writes it + +`/cov_vplan/dv_evidence.json`, produced by the `cov_vplan` job, and only when the sim config names a `vplan`. +It is written before the annotation step runs and is archived alongside the reports, so it outlives the scratch area it describes. + +## Shape + +```json +{ + "schema": "lowrisc-dv-evidence", + "dut": "hmac", + "tool": "xcelium", + "produced_by": "dvsim 1.50.1", + "revision": "https://github.com/lowRISC/opentitan/tree/a1b2c3d (dirty)", + "timestamp": "2026-08-18T09:00:00+00:00", + "testcase": { + "hmac_smoke": [ + { "status": "passed", "seed": 1234, "log": "/scratch/hmac/1234.hmac_smoke/run.log" }, + { "status": "failed", "seed": 5678, "log": "...", "message": "UVM_ERROR", "line": 812 } + ], + "hmac_stress_all": [ + { "status": "not_run" } + ] + } +} +``` + +Fields are omitted when they have no value rather than written as `null`. + +### Top level + +| Key | Meaning | +| --- | --- | +| `schema` | Always `lowrisc-dv-evidence`. Identifies the format to whatever reads the file. | +| `testcase` | Test name to the list of runs of that test. The only required key. | +| `dut` | The design the results are about, named as a verification plan addresses it. | +| `tool` | The simulator that produced them. | +| `produced_by` | What wrote the file, with its version. | +| `revision` | The tree the results were produced against, suffixed ` (dirty)` when it was not clean. | +| `timestamp` | When the run started, as an ISO 8601 datetime with an offset. | + +### A run + +Every entry under `testcase` is keyed by the test name, because that is the name a plan refers to. +Reseeds of one test share the key and are told apart by `seed`. + +| Key | Meaning | +| --- | --- | +| `status` | One of `passed`, `failed`, `killed`, `not_run`. Required. | +| `seed` | The seed the run used, where the flow randomises. | +| `log` | Path to the run's log. | +| `message` | Why it ended that way. Present only on a run that did not pass. | +| `line` | The log line the failure was first reported at. | + +`killed` and `not_run` are separate on purpose. +A killed test started and was terminated, so the design was exercised and something went wrong. +A `not_run` test never started, because the scheduler cancelled it once a dependency failed or the run was shut down. +The two are different answers to "did we verify this", and collapsing them would let a build failure read as a passing plan item. + +There is no `waived` status. +A waiver needs an owner and a date, and a regression can supply neither, so a known failure is recorded as an inspection instead. + +## Inspections + +The format also carries an `inspection` key, for claims no simulation can measure, such as a parameterisation or a structural fact. +Those records are written by hand and live in the tree next to the plan they support. +DVSim never produces them; it only passes their path through to whatever consumes this format, so they are out of scope for this document. + +## Consumers + +[DVPlan](https://github.com/lowRISC/dvplan) reads it to back-annotate a verification plan. +It is not the only thing that could: the format carries no DVPlan concepts, and a dashboard or a CI job wanting machine-readable regression results can read the same file. + +## Changing it + +The pydantic models in `src/dvsim/report/dv_evidence.py` are the normative definition, and this document describes them. +A change to either is a change to the format, so change both, and bear in mind that a consumer may be reading files this repo wrote months ago. diff --git a/src/dvsim/job/data.py b/src/dvsim/job/data.py index bb269bd3..6197a2eb 100644 --- a/src/dvsim/job/data.py +++ b/src/dvsim/job/data.py @@ -10,6 +10,7 @@ """ from collections.abc import Callable, Mapping, Sequence +from enum import Enum from pathlib import Path from typing import TypeAlias @@ -20,12 +21,30 @@ __all__ = ( "CompletedJobStatus", + "DependencyPolicy", "JobSpec", "JobStatusInfo", "WorkspaceConfig", ) +class DependencyPolicy(Enum): + """When a job may start, given how the jobs it depends on ended.""" + + ALL_PASSING = "all_passing" + """Start only if every dependency passed, which is right for a job consuming their output.""" + + ANY_PASSING = "any_passing" + """Start if at least one dependency passed, for a job gathering whatever results exist.""" + + ALWAYS = "always" + """Start once every dependency is terminal, whatever they concluded. + + For a job whose input is the outcome itself rather than an artefact a dependency produced, so + a regression where nothing passed is still the thing it has to report on. + """ + + class WorkspaceConfig(BaseModel): """Workspace configuration.""" @@ -92,8 +111,8 @@ class JobSpec(BaseModel): dependencies: list[str] """Full names of the other Jobs that this one depends on.""" - needs_all_dependencies_passing: bool - """Wait for dependent jobs to pass before scheduling.""" + dependency_policy: DependencyPolicy + """What the jobs this one depends on must have concluded before it may be scheduled.""" weight: int """Weight to apply to the scheduling priority.""" timeout_mins: float | None diff --git a/src/dvsim/job/deploy.py b/src/dvsim/job/deploy.py index 358b411b..6ff1b510 100644 --- a/src/dvsim/job/deploy.py +++ b/src/dvsim/job/deploy.py @@ -12,7 +12,7 @@ from typing import TYPE_CHECKING, ClassVar from dvsim.flow.base import FlowCfg -from dvsim.job.data import JobSpec +from dvsim.job.data import DependencyPolicy, JobSpec from dvsim.job.status import JobStatus from dvsim.job.time import JobTime from dvsim.logging import log @@ -94,10 +94,9 @@ def __init__(self, sim_cfg: "FlowCfg") -> None: # A list of jobs on which this job depends. self.dependencies = [] - # Indicates whether running this job requires all dependencies to pass. - # If this flag is set to False, any passing dependency will trigger - # this current job to run - self.needs_all_dependencies_passing = True + # What the jobs this one depends on must have concluded before it may run. The default + # suits anything consuming a dependency's output, which is most jobs + self.dependency_policy = DependencyPolicy.ALL_PASSING # These variables will be extracted from the hjson file by _set_attrs, # and then _check_attrs checks that they were indeed extracted. Define @@ -175,7 +174,7 @@ def get_job_spec(self) -> "JobSpec": ), workspace_cfg=self.sim_cfg.workspace_cfg, dependencies=[d.full_name for d in self.dependencies], - needs_all_dependencies_passing=self.needs_all_dependencies_passing, + dependency_policy=self.dependency_policy, weight=self.weight, timeout_mins=(None if self.gui else self.get_timeout_mins()), cmd=self.cmd, @@ -911,8 +910,8 @@ def __init__(self, run_items: Iterable[RunTest], sim_cfg: FlowCfg) -> None: super().__init__(sim_cfg) self.dependencies.extend(run_items) - # Run coverage merge even if one test passes. - self.needs_all_dependencies_passing = False + # Merge whatever coverage exists, so one passing test is enough to be worth merging. + self.dependency_policy = DependencyPolicy.ANY_PASSING # Append cov_db_dirs to the list of exports. self.merged_exports["cov_db_dirs"] = shlex.quote(" ".join(self.cov_db_dirs)) @@ -1074,8 +1073,9 @@ def __init__(self, dependencies: "Iterable[Deploy]", sim_cfg: "SimCfg") -> None: super().__init__(sim_cfg) # Every run it scores has to be terminal first, so the collector's evidence is complete self.dependencies.extend(dependencies) - # A failed or killed run is still evidence, so score what happened rather than skipping - self.needs_all_dependencies_passing = False + # A failed or killed run is still evidence, and a regression where nothing passed is the + # case the plan most needs to describe, so this is scored whatever the dependencies did + self.dependency_policy = DependencyPolicy.ALWAYS def _define_attrs(self) -> None: super()._define_attrs() diff --git a/src/dvsim/report/dv_evidence.py b/src/dvsim/report/dv_evidence.py index 363d795c..6c6fbd90 100644 --- a/src/dvsim/report/dv_evidence.py +++ b/src/dvsim/report/dv_evidence.py @@ -4,8 +4,12 @@ """Regression results in the tool-neutral `lowrisc-dv-evidence` format. -dvplan defines the format, so a vPlan can be back-annotated from any regression flow and a person -can write one by hand. What dvsim writes here is a plain serialisation of what it already knows. +These models are the format's definition, and `doc/dv_evidence.md` describes them. It lives here +because dvsim is what produces the file, so anything reading one can be written against a public +spec rather than against whichever consumer happened to be built first. + +The format carries no planning-tool concepts, so a verification plan can be scored from any +regression flow that emits it, and a person can write one by hand. Built from what the scheduler concludes about each job, through its completion hook. That is the same state the JSON report is derived from, so the two cannot disagree about a run, and it is @@ -50,8 +54,8 @@ class Outcome(Enum): """How one run of a test ended, in the neutral format's vocabulary. - There is no waived outcome: dvplan requires an owner and a date on a waiver, and a regression - can supply neither. A known failure is accepted there by recording an inspection instead. + There is no waived outcome. A waiver needs an owner and a date, and a regression can supply + neither, so the format only allows one on an inspection, which is written by hand. """ PASSED = "passed" @@ -96,8 +100,8 @@ class TestRun(BaseModel): class EvidenceFile(BaseModel): """A regression's results, in the tool-neutral evidence format. - dvsim only ever fills the `testcase` half. The format also carries manual inspections, which a - person writes by hand. + dvsim only ever fills the `testcase` half. The format also has an `inspection` key, for claims + no simulation can measure, and those records are written by hand. """ model_config = ConfigDict(frozen=True, extra="forbid", populate_by_name=True) diff --git a/src/dvsim/report/vplan.py b/src/dvsim/report/vplan.py index f3704b8c..6891f767 100644 --- a/src/dvsim/report/vplan.py +++ b/src/dvsim/report/vplan.py @@ -12,7 +12,6 @@ import glob import shlex -import shutil from collections.abc import Sequence from dataclasses import dataclass, field from pathlib import Path @@ -21,7 +20,7 @@ from dvsim.logging import log -__all__ = ("VPlanInputs", "overall_coverage", "shell_command") +__all__ = ("SKIP_WITHOUT_DVPLAN", "VPlanInputs", "overall_coverage", "shell_command") # Scratch subdirectory the annotated plan and its report are written to. Unchanged, so an existing # link to the report still resolves @@ -31,6 +30,14 @@ ANNOTATED_HTML = "vplan_annotated.html" EVIDENCE_JSON = "dv_evidence.json" +# Whether dvplan is installed is decided by the script, on the machine the job lands on, rather +# than by dvsim on whichever host the run was launched from. Exits 0 so that a checkout without +# dvplan does not fail every regression that names a vPlan +SKIP_WITHOUT_DVPLAN = ( + "if ! command -v dvplan >/dev/null 2>&1; then " + "echo 'WARNING: dvplan is not installed on PATH. Skipping vPlan annotation.'; exit 0; fi;" +) + @dataclass(frozen=True) class VPlanInputs: @@ -74,12 +81,10 @@ def shell_command(inputs: VPlanInputs) -> str: Returned as one `bash -c` string because a scheduled job runs a shell command. `set -e` and the `&&` mean a broken annotation shows as a failed job rather than a silently missing score. - """ - if shutil.which("dvplan") is None: - # Warn and pass, so a checkout without dvplan does not fail every regression naming a vPlan - warning = "WARNING: dvplan is not installed on PATH. Skipping vPlan annotation." - return f"/usr/bin/env bash -c {shlex.quote(f'echo {shlex.quote(warning)}')}" + The command is the same whether or not dvplan is installed here, because here is not where it + runs. See `SKIP_WITHOUT_DVPLAN`. + """ # The vPlan sits at //, so its grandparent is the IP root that # `prepare_vplan` traces specifications against. ip_root = inputs.vplan.parent.parent @@ -94,7 +99,8 @@ def shell_command(inputs: VPlanInputs) -> str: process = _process_command(inputs) script = ( - f"set -e; mkdir -p {shlex.quote(str(inputs.out_dir))}; " + f"set -e; {SKIP_WITHOUT_DVPLAN} " + f"mkdir -p {shlex.quote(str(inputs.out_dir))}; " f"{shlex.join(prepare)} && {shlex.join(process)}" ) return f"/usr/bin/env bash -c {shlex.quote(script)}" @@ -146,11 +152,17 @@ def _expand(pattern: str) -> list[str]: A cfg naming inspections through `{proj_root}` always produces an absolute pattern, which `Path.glob` refuses, so this is one of the places the pathlib rule does not apply. + + A pattern matching nothing raises, because both other answers are worse: passing it through + fails the job with dvplan's own message once the regression has already run, and dropping it + scores the plan as though the cfg had never named inspections at all. The command is built + while the jobs are, so this lands before a single test starts. """ matches = sorted(glob.glob(pattern)) # noqa: PTH207 (Path.glob rejects an absolute pattern) if not matches: - log.warning("No inspection records matched '%s', so none were annotated from.", pattern) - return matches or [pattern] + msg = f"No inspection records matched 'dvplan_inspect' pattern '{pattern}'." + raise ValueError(msg) + return matches def overall_coverage(annotated: Path) -> float | None: diff --git a/src/dvsim/scheduler/core.py b/src/dvsim/scheduler/core.py index 3b61bb9f..e81f5c82 100644 --- a/src/dvsim/scheduler/core.py +++ b/src/dvsim/scheduler/core.py @@ -13,7 +13,7 @@ from types import FrameType from typing import Any, TypeAlias -from dvsim.job.data import CompletedJobStatus, JobSpec, JobStatusInfo +from dvsim.job.data import CompletedJobStatus, DependencyPolicy, JobSpec, JobStatusInfo from dvsim.job.status import JobStatus from dvsim.logging import log from dvsim.runtime.backend import RuntimeBackend @@ -365,7 +365,10 @@ def _update_completed_job_deps(self, job: JobRecord) -> None: # Handle dependency management and marking dependents as ready if dep.remaining_deps == 0 and dep.status == JobStatus.SCHEDULED: - if dep.spec.needs_all_dependencies_passing: + policy = dep.spec.dependency_policy + if policy is DependencyPolicy.ALWAYS: + self._mark_job_ready(dep) + elif policy is DependencyPolicy.ALL_PASSING: if dep.passing_deps == len(dep.spec.dependencies): self._mark_job_ready(dep) else: diff --git a/src/dvsim/sim/flow.py b/src/dvsim/sim/flow.py index 4b8975a5..6cdbed1c 100644 --- a/src/dvsim/sim/flow.py +++ b/src/dvsim/sim/flow.py @@ -884,10 +884,13 @@ def make_test_result(tr) -> TestResult | None: cov_report_dir = self.cov_report_dir or "cov_report" cov_report_page = Path(cov_report_dir, self.cov_report_page) + # Linked only once the page is actually there. The job can be killed, and it exits without + # annotating anything where dvplan is not installed, so its output directory is not proof vplan_report_page = None vplan_coverage = None if self.cov_vplan_deploy is not None: - vplan_report_page = self.cov_vplan_deploy.report_page + page = self.cov_vplan_deploy.report_page + vplan_report_page = page if page.is_file() else None vplan_coverage = self.cov_vplan_deploy.vplan_coverage failures = BucketedFailures.from_job_status(results=run_results) diff --git a/tests/job/test_cov_vplan.py b/tests/job/test_cov_vplan.py index 24b6fb78..1425a214 100644 --- a/tests/job/test_cov_vplan.py +++ b/tests/job/test_cov_vplan.py @@ -16,7 +16,7 @@ import pytest from hamcrest import assert_that, contains_string, equal_to, is_, none -from dvsim.job.data import WorkspaceConfig +from dvsim.job.data import DependencyPolicy, WorkspaceConfig from dvsim.job.deploy import CovVPlan from dvsim.job.status import JobStatus from dvsim.report.vplan import ANNOTATED_HJSON, ANNOTATED_HTML, VPLAN_DIR @@ -62,9 +62,8 @@ def _cfg(**overrides: object) -> SimpleNamespace: @pytest.fixture -def job(monkeypatch: pytest.MonkeyPatch) -> CovVPlan: - """A constructed job, with dvplan present so the real command is built.""" - monkeypatch.setattr("dvsim.report.vplan.shutil.which", lambda _: "/usr/bin/dvplan") +def job() -> CovVPlan: + """Construct the job the way the sim flow does.""" return CovVPlan([], _cfg()) @@ -92,19 +91,31 @@ def test_the_job_builds_a_runnable_command(job: CovVPlan) -> None: assert_that(job.cmd, contains_string("-s hmac tb.dut")) -def test_a_run_without_coverage_still_annotates(monkeypatch: pytest.MonkeyPatch) -> None: +def test_a_run_without_coverage_still_annotates() -> None: """Without --cov there is no vendor report, and the plan is scored from the evidence alone.""" - monkeypatch.setattr("dvsim.report.vplan.shutil.which", lambda _: "/usr/bin/dvplan") - job = CovVPlan([], _cfg(cov=False)) assert_that(job.cmd, contains_string("--coverage dv_evidence")) assert_that("xcelium_report" in job.cmd, is_(False)) +def test_an_inspection_pattern_matching_nothing_fails_at_config_time() -> None: + """The cfg names records that are not there, so the run must stop before it burns a regression. + + The command is built in `Deploy.__init__`, so this lands while the jobs are still being + created rather than hours later inside dvplan. + """ + with pytest.raises(ValueError, match="No inspection records matched"): + CovVPlan([], _cfg(dvplan_inspect="/proj/hw/ip/hmac/dv/inspections/*.json")) + + def test_a_failing_run_still_gets_its_plan_scored(job: CovVPlan) -> None: - """A failed test is still evidence, so the job must not be skipped when a dependency fails.""" - assert_that(job.needs_all_dependencies_passing, is_(False)) + """A regression where nothing passed is the case the plan most needs to describe. + + `ANY_PASSING` would not do here. With `--cov` this job has one dependency, the coverage + report, so anything that stops the report also stops the plan being scored. + """ + assert_that(job.dependency_policy, equal_to(DependencyPolicy.ALWAYS)) def test_no_score_is_read_back_when_the_job_did_not_pass(job: CovVPlan) -> None: diff --git a/tests/report/test_dv_evidence.py b/tests/report/test_dv_evidence.py index fbf98fef..40393753 100644 --- a/tests/report/test_dv_evidence.py +++ b/tests/report/test_dv_evidence.py @@ -16,7 +16,7 @@ import pytest from hamcrest import assert_that, contains_string, equal_to, has_key, is_, none, not_ -from dvsim.job.data import JobSpec, JobStatusInfo, WorkspaceConfig +from dvsim.job.data import DependencyPolicy, JobSpec, JobStatusInfo, WorkspaceConfig from dvsim.job.status import JobStatus from dvsim.report.data import IPMeta, ToolMeta from dvsim.report.dv_evidence import ( @@ -74,7 +74,7 @@ def _spec( tool=ToolMeta(name=_TOOL, version="unknown"), workspace_cfg=_WORKSPACE, dependencies=[], - needs_all_dependencies_passing=True, + dependency_policy=DependencyPolicy.ALL_PASSING, weight=1, timeout_mins=None, cmd="make run", diff --git a/tests/report/test_vplan.py b/tests/report/test_vplan.py index 1537db0b..5e8cd658 100644 --- a/tests/report/test_vplan.py +++ b/tests/report/test_vplan.py @@ -6,10 +6,10 @@ The command built here is the interface to another tool, whose positional shape is a fixed contract, so it is checked as carefully as anything that runs in this process. The rest covers -the promise that no vPlan problem can fail a regression that otherwise passed. +the promise that no vPlan problem can fail a regression that otherwise passed, and that the one +cfg mistake which can is caught while the jobs are still being built. """ -import logging from dataclasses import replace from pathlib import Path @@ -42,9 +42,10 @@ def _inputs(tmp_path: Path, **overrides: object) -> VPlanInputs: def test_the_command_keeps_dvplan_s_positional_contract(tmp_path: Path) -> None: """`process_results` takes its three positionals last, with `-s` as a flag before them. - dvplan documents this shape as fixed because dvsim builds it. Getting `-s` wrong is the - error that reads as `--summary` swallowing the DUT name, so it is pinned here rather than - discovered in a nightly. + This pins what dvsim emits. It cannot check dvplan's side, which lives in another repo, so a + failure here means the argv moved and the two need reconciling. Getting `-s` wrong is the + error that reads as `--summary` swallowing the DUT name, hence pinning it rather than + discovering it in a nightly. """ inputs = _inputs(tmp_path) @@ -106,23 +107,16 @@ def test_a_directory_of_inspections_is_passed_through(tmp_path: Path) -> None: assert_that(_expand(str(folder)), equal_to([str(folder)])) -def test_a_pattern_matching_nothing_warns_and_is_left_alone( - tmp_path: Path, caplog: pytest.LogCaptureFixture -) -> None: - """Silently dropping the source would read as the cfg not naming one at all. +def test_a_pattern_matching_nothing_is_a_config_error(tmp_path: Path) -> None: + """Naming inspections that do not exist is a cfg mistake, and both other answers are worse. - The fixture's handler is attached to the 'dvsim' logger by hand, because that logger sets - `propagate = False` and so never reaches the root handler `caplog` installs. + Passing the pattern on would fail the job with dvplan's own message once the regression has + already run, and dropping it would score the plan as though the cfg had never named any. """ pattern = str(tmp_path / "nothing" / "*.json") - dvsim_log = logging.getLogger("dvsim") - dvsim_log.addHandler(caplog.handler) - try: - assert_that(_expand(pattern), equal_to([pattern])) - finally: - dvsim_log.removeHandler(caplog.handler) - assert_that(caplog.text, contains_string("No inspection records matched")) + with pytest.raises(ValueError, match="No inspection records matched"): + _expand(pattern) @pytest.mark.parametrize( @@ -152,32 +146,25 @@ def test_a_missing_annotated_plan_reports_no_score(tmp_path: Path) -> None: assert_that(overall_coverage(tmp_path / "absent.hjson"), is_(none())) -def test_a_missing_dvplan_still_produces_a_runnable_command( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """A checkout without dvplan installed must not fail every regression that names a vPlan. +def test_a_missing_dvplan_is_decided_where_the_job_runs(tmp_path: Path) -> None: + """A checkout without dvplan must not fail every regression that names a vPlan. - The job still has to run something, so it warns and passes rather than erroring. + Testing PATH here would answer for the host dvsim was launched from, which on a compute farm + is not the host the job lands on, so the guard goes in the script and is checked there. """ - monkeypatch.setattr("dvsim.report.vplan.shutil.which", lambda _: None) - command = shell_command(_inputs(tmp_path)) - assert_that(command, contains_string("bash -c")) + assert_that(command, contains_string("command -v dvplan")) assert_that(command, contains_string("WARNING")) - assert_that("dvplan process_results" in command, is_(False)) + assert_that(command, contains_string("exit 0")) -def test_the_command_fails_the_job_when_dvplan_does( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: +def test_the_command_fails_the_job_when_dvplan_does(tmp_path: Path) -> None: """A broken annotation shows as a failed job rather than a silently missing score. `set -e` and the `&&` are what carry a non-zero exit out to the scheduler, so they are checked rather than assumed. """ - monkeypatch.setattr("dvsim.report.vplan.shutil.which", lambda _: "/usr/bin/dvplan") - command = shell_command(_inputs(tmp_path)) assert_that(command, contains_string("set -e")) diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 8619c81a..a3b2f726 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -19,7 +19,7 @@ import pytest from hamcrest import assert_that, calling, empty, equal_to, only_contains, raises -from dvsim.job.data import CompletedJobStatus, JobSpec, WorkspaceConfig +from dvsim.job.data import CompletedJobStatus, DependencyPolicy, JobSpec, WorkspaceConfig from dvsim.job.status import JobStatus from dvsim.launcher.base import ErrorMessage, Launcher, LauncherBusyError, LauncherError from dvsim.report.data import IPMeta, ToolMeta @@ -302,7 +302,7 @@ def job_spec_factory( "resources": None, "seed": None, "dependencies": [], - "needs_all_dependencies_passing": True, + "dependency_policy": DependencyPolicy.ALL_PASSING, "weight": 1, "timeout_mins": None, "cmd": "echo 'test_cmd'", @@ -618,10 +618,10 @@ class TestSchedulingStructure: @staticmethod @pytest.mark.asyncio @pytest.mark.timeout(DEFAULT_TIMEOUT) - @pytest.mark.parametrize("needs_all_passing", [True, False]) - async def test_no_deps(fxt: Fxt, *, needs_all_passing: bool) -> None: + @pytest.mark.parametrize("policy", list(DependencyPolicy)) + async def test_no_deps(fxt: Fxt, policy: DependencyPolicy) -> None: """Tests scheduling of jobs without any listed dependencies.""" - job = job_spec_factory(fxt.tmp_path, needs_all_dependencies_passing=needs_all_passing) + job = job_spec_factory(fxt.tmp_path, dependency_policy=policy) result = await Scheduler([job], fxt.backends, MOCK_BACKEND).run() _assert_result_status(result, 1) @@ -630,14 +630,13 @@ async def _dep_test_case( fxt: Fxt, dep_list: dict[int, list[int]], passes: list[int], - *, - all_passing: bool, + policy: DependencyPolicy, ) -> None: """Run a simple dependency test, with 5 jobs where jobs 2 & 4 will fail.""" jobs = make_many_jobs( fxt.tmp_path, 5, - needs_all_dependencies_passing=all_passing, + dependency_policy=policy, interdeps=dep_list, ) fxt.mock_ctx.set_config(jobs[2], MockJob(default_status=JobStatus.FAILED)) @@ -672,7 +671,9 @@ async def test_needs_any_dep( passes: list[int], ) -> None: """Tests scheduling of jobs with dependencies that don't need all passing.""" - await TestSchedulingStructure._dep_test_case(fxt, dep_list, passes, all_passing=False) + await TestSchedulingStructure._dep_test_case( + fxt, dep_list, passes, DependencyPolicy.ANY_PASSING + ) @staticmethod @pytest.mark.asyncio @@ -694,7 +695,31 @@ async def test_needs_all_deps( passes: list[int], ) -> None: """Tests scheduling of jobs with dependencies that need all passing.""" - await TestSchedulingStructure._dep_test_case(fxt, dep_list, passes, all_passing=True) + await TestSchedulingStructure._dep_test_case( + fxt, dep_list, passes, DependencyPolicy.ALL_PASSING + ) + + @staticmethod + @pytest.mark.asyncio + @pytest.mark.timeout(DEFAULT_TIMEOUT) + @pytest.mark.parametrize( + ("dep_list", "passes"), + [ + # One failing dependency, which both of the other policies treat as a reason to skip + ({1: [2]}, [0, 1, 3]), + # Every dependency failed, which is the case a vPlan score most needs to describe + ({3: [2, 4]}, [0, 1, 3]), + # A mix, so a passing dependency is not what releases the job + ({0: [1, 2, 3, 4]}, [0, 1, 3]), + ], + ) + async def test_runs_whatever_the_deps_concluded( + fxt: Fxt, + dep_list: dict[int, list[int]], + passes: list[int], + ) -> None: + """Tests scheduling of jobs that only wait for their dependencies to be terminal.""" + await TestSchedulingStructure._dep_test_case(fxt, dep_list, passes, DependencyPolicy.ALWAYS) @staticmethod @pytest.mark.asyncio From 9c10b64654c0cee00b0ec31ce42060215ecf7e24 Mon Sep 17 00:00:00 2001 From: martin-velay Date: Tue, 18 Aug 2026 16:33:43 +0200 Subject: [PATCH 5/5] docs: scope the evidence spec to what a producer writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page claimed the pydantic models were the normative definition of the whole format. They are not: they forbid the inspection key, so they reject a file DVPlan accepts. That is correct for a writer, but it makes the claim wrong. AI-assisted (Claude Code) — reviewed and approved by author Signed-off-by: martin-velay --- doc/dv_evidence.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/doc/dv_evidence.md b/doc/dv_evidence.md index 83bf7de3..d30da1a1 100644 --- a/doc/dv_evidence.md +++ b/doc/dv_evidence.md @@ -10,7 +10,7 @@ A verification plan asks a different question: of everything we said we would ve Answering it needs the regression's own outcomes in a form a planning tool can read, rather than a log directory and a human. This is that form. -DVSim writes one of these files per simulation flow, and it is the definition of the format rather than a description of one tool's output. +DVSim writes one of these files per simulation flow, and this page specifies what any producer has to write rather than describing one tool's output. Anything that can produce it can be scored against a verification plan, whether or not it is DVSim. ## Where DVSim writes it @@ -88,5 +88,7 @@ It is not the only thing that could: the format carries no DVPlan concepts, and ## Changing it -The pydantic models in `src/dvsim/report/dv_evidence.py` are the normative definition, and this document describes them. -A change to either is a change to the format, so change both, and bear in mind that a consumer may be reading files this repo wrote months ago. +The pydantic models in `src/dvsim/report/dv_evidence.py` specify what a producer writes, and this document describes them. +They are not the whole format: the `inspection` half and the rules for scoring a file are the consumer's, and [DVPlan](https://github.com/lowRISC/dvplan) specifies those. +So the models here reject a file carrying an `inspection` key, which is correct, since DVSim writes evidence and never reads it back. +A change to the models or to this document is a change to the format, so change both, and bear in mind that a consumer may be reading files this repo wrote months ago.