From 27df69206ddb31b81b872858bd9e977b67a57d6e Mon Sep 17 00:00:00 2001 From: Harry Callahan Date: Sun, 25 Oct 2026 01:10:39 +0000 Subject: [PATCH 1/5] feat: add --cfg-extend to merge a file into a named cfg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no way to add to a cfg that dvsim loads from the project tree without editing that tree: import_cfgs and overrides only act on the file that holds them, and a primary cfg can't reach into its children. A partner or licensed-VIP testbench kept outside the tree therefore has to patch the project's hjson. --cfg-extend NAME=PATH (repeatable) loads PATH with its imports and merges it into the cfg whose name is NAME, after that cfg has merged all of its own files and before overrides and wildcards are processed. The normal rules apply: lists concatenate, so the extension's build_opts come after the tool's, which hold its filelist; a conflicting scalar is an error (tool included, unless --tool is given); the extension's overrides join the cfg's own. Because the arguments are shared with every child cfg, this works whether NAME is run on its own or is a child of a primary cfg. The extension names its own files with {file_dir}. Extensions apply in command-line order. An extension that no cfg matched is an error, which catches typos and upstream renames; so is naming the same NAME=PATH twice, naming a primary cfg, an extension that sets use_cfgs, or one that names an import that can't be resolved. AI-assisted (Claude Code) — reviewed and approved by author Co-Authored-By: Claude Signed-off-by: Harry Callahan --- doc/design_doc.md | 18 +++ src/dvsim/cli/run.py | 24 ++++ src/dvsim/flow/base.py | 42 ++++++ src/dvsim/flow/extension.py | 113 +++++++++++++++ src/dvsim/flow/factory.py | 25 +++- src/dvsim/flow/hjson.py | 9 +- tests/flow/test_extension.py | 267 +++++++++++++++++++++++++++++++++++ 7 files changed, 495 insertions(+), 3 deletions(-) create mode 100644 src/dvsim/flow/extension.py create mode 100644 tests/flow/test_extension.py diff --git a/doc/design_doc.md b/doc/design_doc.md index 7865918c..79869c28 100644 --- a/doc/design_doc.md +++ b/doc/design_doc.md @@ -149,6 +149,24 @@ Within any of these files, `{file_dir}` names the directory that holds that file `{self_dir}`, by contrast, is the directory of the configuration file that dvsim loaded, at the top of the import tree. `{file_dir}` is replaced as each file is loaded, so it can only be used in Hjson files, not in options given on the command line. +A configuration can also be extended from outside the project tree with `--cfg-extend NAME=PATH`, for example to add tests that need a licensed verification IP that the project itself can't hold: + +```hjson +// ext.hjson, merged with: dvsim top_sim_cfgs.hjson --cfg-extend foo=/path/to/ext.hjson ... +{ + import_cfgs: ["{file_dir}/vip_tests.hjson"] + build_modes: [{name: "vip", build_opts: ["-f {file_dir}/vip.f"]}] + overrides: [{name: "foo_setting", value: "with_vip"}] +} +``` + +The Hjson file at `PATH`, with its imports, is merged into the configuration whose `name` is `NAME` once that configuration has loaded all of its own files; this works whether that configuration is run on its own or is a child of a primary configuration. +`NAME` is matched against the name that the configuration's own files give it, before any `overrides`. +The usual merge rules apply: the extension's list entries follow the configuration's own, and a conflicting scalar is an error, so an extension changes a scalar with `overrides` (but not one that the configuration already overrides). +In an extension, `{self_dir}` is still the directory of the configuration file that dvsim loaded, so the extension names its own files with `{file_dir}`. +Several extensions apply in command-line order. +An extension whose `NAME` matches no loaded configuration is an error, as is naming one extension twice. + ### Parser stage The **parser stage** parses all Hjson files in the tree and constructs a **flow manager object**. diff --git a/src/dvsim/cli/run.py b/src/dvsim/cli/run.py index 60d36877..63eae362 100644 --- a/src/dvsim/cli/run.py +++ b/src/dvsim/cli/run.py @@ -37,6 +37,7 @@ load_config_file, proj_root_from_config, ) +from dvsim.flow.extension import CfgExtension from dvsim.flow.factory import make_cfg from dvsim.fusesoc import CONFIG_SECTION as FUSESOC_CONFIG_SECTION from dvsim.fusesoc import resolve_options as resolve_fusesoc_options @@ -185,6 +186,14 @@ def parse_resource(s: str) -> tuple[str, int | None]: raise argparse.ArgumentTypeError(msg) from e +def parse_cfg_extension(s: str) -> CfgExtension: + """Parse a cfg extension (NAME=PATH) from the argparse CLI.""" + try: + return CfgExtension.parse(s) + except ValueError as e: + raise argparse.ArgumentTypeError(str(e)) from e + + def resolve_branch(branch, cwd=None): """Choose a branch name for output files. @@ -453,6 +462,21 @@ def parse_args(argv: list[str] | None = None): ), ) + whatg.add_argument( + "--cfg-extend", + action="append", + default=[], + type=parse_cfg_extension, + metavar="NAME=PATH", + help=( + "Merge the hjson file PATH (and its imports) into the cfg whose " + "'name' is NAME, after the cfg's own files. NAME may be a child of " + "a primary config. Lists are appended to; scalars are changed " + "with 'overrides'. PATH names its own files with {file_dir}. " + "Repeatable; applied in order. It is an error if NAME matches no cfg." + ), + ) + disg = parser.add_argument_group("Dispatch options") disg.add_argument( diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index c6badf9a..152ddc35 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -17,6 +17,7 @@ import hjson import dvsim.instrumentation.runtime as instrumentation +from dvsim.flow.extension import CfgExtension, load_cfg_extension from dvsim.flow.hjson import set_target_attribute from dvsim.fusesoc import rewrite_fusesoc_opts from dvsim.job.data import CompletedJobStatus, JobSpec, WorkspaceConfig @@ -147,7 +148,15 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: # configuration". self.is_primary_cfg = "use_cfgs" in hjson_data + # The cfg extensions (--cfg-extend) merged into this cfg, and the name + # that they were matched against. + self.cfg_extensions: list[CfgExtension] = [] + self.cfg_extend_name: str | None = None + if not self.is_primary_cfg: + # Extensions merge after everything the cfg loads itself, and + # before overrides and wildcards are processed. + self._merge_cfg_extensions() self.cfgs.append(self) else: for entry in self.use_cfgs: @@ -205,6 +214,39 @@ def _merge_hjson(self, hjson_data: Mapping) -> None: for key, value in hjson_data.items(): set_target_attribute(self.flow_cfg_file, self.__dict__, key, value) + def _merge_cfg_extensions(self) -> None: + """Merge in the cfg extensions (--cfg-extend) that name this cfg. + + They apply in command-line order. Their list values follow the cfg's + own, and their overrides join the cfg's own. + """ + # The name as the cfg's own files give it. Overrides could still change + # it, but those are processed after the extensions merge. + name = subst_wildcards(self.name, self.__dict__, ignore_error=True) + self.cfg_extend_name = name + + # What load_hjson needs to resolve the names of imported files. As in + # the cfg's own files, {self_dir} is the cfg's directory: an extension + # names its own files with {file_dir}. + initial_values = {"proj_root": self.proj_root, "self_dir": self.self_dir} + if self.tool: + initial_values["tool"] = self.tool + # Only a tool given on the command line can't be changed by the files. + arg_keys = {"tool"} if self.args.tool is not None else set() + + for ext in self.args.cfg_extend: + if ext.cfg_name != name: + continue + log.info("[cfg_extend]: %s: %s", name, ext.path) + try: + ext_data = load_cfg_extension(ext, initial_values, arg_keys) + for key, value in ext_data.items(): + set_target_attribute(str(ext.path), self.__dict__, key, value) + except RuntimeError as err: + log.error("Cannot extend cfg %r: %s", name, err) + sys.exit(1) + self.cfg_extensions.append(ext) + def _expand(self) -> None: """Expand wildcards after merging hjson. diff --git a/src/dvsim/flow/extension.py b/src/dvsim/flow/extension.py new file mode 100644 index 00000000..dba1ecc6 --- /dev/null +++ b/src/dvsim/flow/extension.py @@ -0,0 +1,113 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Cfg extensions: hjson files merged into a named flow cfg (--cfg-extend). + +See the "DUT configuration" section of doc/design_doc.md. +""" + +import os +from collections.abc import Iterable, Mapping +from dataclasses import dataclass +from pathlib import Path + +from dvsim.flow.hjson import load_hjson + +__all__ = ( + "CfgExtension", + "check_cfg_extensions_used", + "load_cfg_extension", +) + + +@dataclass(frozen=True) +class CfgExtension: + """An extension file, and the name of the cfg it extends.""" + + cfg_name: str + path: Path + + @classmethod + def parse(cls, value: str) -> "CfgExtension": + """Parse NAME=PATH, as given to --cfg-extend. + + A relative PATH is taken from the current directory. Symlinks are + kept, as they are in {file_dir}. + """ + name, sep, path = value.partition("=") + name, path = name.strip(), path.strip() + if not sep or not name or not path: + msg = f"Invalid cfg extension {value!r}: expected NAME=PATH." + raise ValueError(msg) + return cls(name, Path(os.path.normpath(Path(path).absolute()))) + + +def load_cfg_extension( + ext: CfgExtension, initial_values: Mapping[str, object], arg_keys: Iterable[str] +) -> dict: + """Load an extension file and its imports. + + initial_values holds what is needed to resolve the names of imported files, + and arg_keys those of its keys that came from the command line (as for + load_hjson). The returned data holds only what the extension sets. + + Raises a RuntimeError if the file can't be loaded or isn't allowed. + """ + path = str(ext.path) + if not ext.path.is_file(): + msg = f"{path!r}: Not a file." + raise RuntimeError(msg) + + try: + data = load_hjson(path, initial_values, set(arg_keys)) + except ValueError as err: + # An import_cfgs entry with a wildcard that can't be resolved. + msg = f"{path!r}: Can't resolve the name of an imported file: {err}" + raise RuntimeError(msg) from err + + if "use_cfgs" in data: + msg = f"{path!r}: An extension can't set use_cfgs." + raise RuntimeError(msg) + + # The initial values are the cfg's own. If the extension set one of them, + # it set it to the same value (or load_hjson reported a conflict), so + # merging it would change nothing. + for key in initial_values: + data.pop(key, None) + + return data + + +def check_cfg_extensions_used( + extensions: Iterable[CfgExtension], + used: Iterable[CfgExtension], + cfg_names: Iterable[str], + primary_name: str | None, +) -> list[str]: + """Check that every extension was used by a loaded cfg. + + used are the extensions that some cfg matched. cfg_names are the names that + the cfgs that can be extended had when they matched them (before any + overrides); a primary cfg can't be extended, so its name is passed + separately to give a clearer message. Returns a list of error messages + (empty if all is well). + """ + used = set(used) + names = set(cfg_names) + errors = [] + for ext in extensions: + if ext in used: + continue + if ext.cfg_name == primary_name: + errors.append( + f"Cfg extension {ext.path} names the primary cfg {ext.cfg_name!r}: " + "only a cfg that runs jobs (a child, or a cfg run on its own) can be extended." + ) + else: + known = ", ".join(sorted(names)) or "none" + errors.append( + f"Cfg extension {ext.path} names {ext.cfg_name!r}, but no loaded cfg has " + f"that name (loaded: {known})." + ) + return errors diff --git a/src/dvsim/flow/factory.py b/src/dvsim/flow/factory.py index 166db158..be76401c 100644 --- a/src/dvsim/flow/factory.py +++ b/src/dvsim/flow/factory.py @@ -7,6 +7,7 @@ from dvsim.flow.base import FlowCfg from dvsim.flow.cdc import CdcCfg +from dvsim.flow.extension import check_cfg_extensions_used from dvsim.flow.formal import FormalCfg from dvsim.flow.hjson import load_hjson from dvsim.flow.lint import LintCfg @@ -113,6 +114,13 @@ def make_cfg(path, args, proj_root) -> FlowCfg: of the project. """ + # An extension named twice would be merged twice. + repeated = [ext for ext in dict.fromkeys(args.cfg_extend) if args.cfg_extend.count(ext) > 1] + for ext in repeated: + log.error("Cfg extension %s=%s is given more than once.", ext.cfg_name, ext.path) + if repeated: + sys.exit(1) + initial_values = { "proj_root": proj_root, "self_dir": pathlib.Path(path).parent, @@ -131,4 +139,19 @@ def factory(child_path): child_ivs["flow"] = hjson_data["flow"] return _make_child_cfg(child_path, args, child_ivs) - return cls(path, hjson_data, args, factory) + cfg = cls(path, hjson_data, args, factory) + + # Every extension must have found its cfg. A name that matches nothing is + # a typo or a cfg that has been renamed, and would otherwise pass silently. + errors = check_cfg_extensions_used( + args.cfg_extend, + [ext for child in cfg.cfgs for ext in child.cfg_extensions], + [child.cfg_extend_name for child in cfg.cfgs], + cfg.name if cfg.is_primary_cfg else None, + ) + for error in errors: + log.error("%s", error) + if errors: + sys.exit(1) + + return cfg diff --git a/src/dvsim/flow/hjson.py b/src/dvsim/flow/hjson.py index 37187667..9d946c0d 100644 --- a/src/dvsim/flow/hjson.py +++ b/src/dvsim/flow/hjson.py @@ -17,7 +17,7 @@ _FILE_DIR_WILDCARD = "{file_dir}" -def load_hjson(path, initial_values): +def load_hjson(path, initial_values, arg_keys=None): """Load an hjson file and any includes. Combines them all into a single dictionary, which is then returned. This @@ -29,6 +29,10 @@ def load_hjson(path, initial_values): is not modified). It needs to contain values for anything needed to resolve include files (typically, this is 'proj_root' and 'tool' (if set)). + arg_keys are the keys of initial_values that came from the command line, + which the files can't change. By default, that is any of them that the + command line can set. + """ worklist = [path] seen = {path} @@ -38,7 +42,8 @@ def load_hjson(path, initial_values): # Figure out a list of fields that had a value from the command line. These # should have been passed in as part of initial_values and we need to know # that we can safely ignore updates. - arg_keys = _CMDLINE_FIELDS & initial_values.keys() + if arg_keys is None: + arg_keys = _CMDLINE_FIELDS & initial_values.keys() while worklist: next_path = worklist.pop() diff --git a/tests/flow/test_extension.py b/tests/flow/test_extension.py new file mode 100644 index 00000000..abcacd44 --- /dev/null +++ b/tests/flow/test_extension.py @@ -0,0 +1,267 @@ +# 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 cfg extensions (--cfg-extend).""" + +import subprocess +from collections.abc import Sequence +from pathlib import Path + +import pytest + +from dvsim.cli.run import parse_args +from dvsim.flow.base import FlowCfg +from dvsim.flow.extension import CfgExtension, check_cfg_extensions_used +from dvsim.flow.factory import make_cfg + +__all__ = () + +# A sim cfg with just enough in it to load. Like a real block cfg, it gets its +# tool options from an imported file, so the test can check that an +# extension's list entries come after those. +CHILD_CFG = """{ + name: blk + flow: sim + tool: vcs + scratch_path: "{scratch_root}/blk" + import_cfgs: ["{proj_root}/hw/tool.hjson"] + build_modes: [{name: "waves_off"}, {name: "xprop"}, {name: "build_seed"}] + tests: [{name: "t1"}] + regressions: [] + testplan: "" + blk_var: "orig" + blk_empty: "" +}""" + +TOOL_CFG = '{build_opts: ["-f {proj_root}/sv.f"], run_opts: ["+tool"]}' + +PRIMARY_CFG = """{ + name: top + flow: sim + scratch_path: "{scratch_root}/top" + use_cfgs: ["{proj_root}/hw/blk_sim_cfg.hjson"] +}""" + + +@pytest.fixture +def proj(tmp_path: Path) -> Path: + """A git repo holding a primary cfg and its one child, "blk".""" + root = tmp_path.resolve() / "proj" + (root / "hw").mkdir(parents=True) + (root / "hw" / "blk_sim_cfg.hjson").write_text(CHILD_CFG) + (root / "hw" / "tool.hjson").write_text(TOOL_CFG) + (root / "hw" / "top_sim_cfgs.hjson").write_text(PRIMARY_CFG) + git = ["git", "-C", str(root)] + subprocess.run([*git, "init", "-q"], check=True) + subprocess.run([*git, "commit", "-q", "--allow-empty", "-m", "init"], check=True) + return root + + +def _write(path: Path, text: str) -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + return path + + +def _load(proj: Path, cfg: str, *extends: str, argv: Sequence[str] = ()) -> FlowCfg: + argv = [str(proj / "hw" / cfg), "--scratch-root", str(proj.parent / "scratch"), *argv] + argv += ["--branch", "b"] + for ext in extends: + argv += ["--cfg-extend", ext] + args = parse_args(argv) + args.timestamp = args.timestamp_long = "ts" + return make_cfg(args.cfg, args, str(proj)) + + +def _blk(cfg: FlowCfg) -> FlowCfg: + (blk,) = [c for c in cfg.cfgs if c.name == "blk"] + return blk + + +class TestParse: + """Parsing NAME=PATH.""" + + def test_name_and_path(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.chdir(tmp_path) + assert CfgExtension.parse("blk=ext/x.hjson") == CfgExtension( + "blk", tmp_path.resolve() / "ext" / "x.hjson" + ) + + def test_symlinks_are_kept(self, tmp_path: Path) -> None: + """The path is spelt as {file_dir} spells it, through any symlink.""" + (tmp_path / "real").mkdir() + (tmp_path / "link").symlink_to(tmp_path / "real") + + ext = CfgExtension.parse(f"blk={tmp_path}/link/../link/x.hjson") + + assert ext.path == tmp_path / "link" / "x.hjson" + + @pytest.mark.parametrize("value", ["blk", "=x.hjson", "blk=", " = "]) + def test_rejects_malformed(self, value: str) -> None: + with pytest.raises(ValueError, match="NAME=PATH"): + CfgExtension.parse(value) + + def test_cli_rejects_malformed(self, proj: Path) -> None: + with pytest.raises(SystemExit): + parse_args([str(proj / "hw" / "top_sim_cfgs.hjson"), "--cfg-extend", "blk"]) + + +class TestMerge: + """Merging an extension into its cfg.""" + + @pytest.mark.parametrize("top", ["top_sim_cfgs.hjson", "blk_sim_cfg.hjson"]) + def test_lists_follow_the_cfgs_own(self, proj: Path, tmp_path: Path, top: str) -> None: + """The extension reaches the cfg whether it is a child or run alone. + + Its list entries land after everything the cfg imports, so build + options that need the tool's filelist come after it. + """ + ext = _write( + tmp_path / "overlay" / "ext.hjson", + """{ + build_opts: ["+incdir+/overlay", "/overlay/bind.sv"] + tests: [{name: "t2"}] + }""", + ) + + blk = _blk(_load(proj, top, f"blk={ext}")) + + assert blk.build_opts[:3] == [f"-f {proj}/sv.f", "+incdir+/overlay", "/overlay/bind.sv"] + assert blk.run_opts[:1] == ["+tool"] + assert [t.name for t in blk.tests] == ["t1", "t2"] + assert blk.cfg_extensions == [CfgExtension("blk", ext)] + + def test_imports_and_order(self, proj: Path, tmp_path: Path) -> None: + """Extensions apply in command-line order, each with its own imports.""" + first = _write( + tmp_path / "a" / "ext.hjson", + '{import_cfgs: ["{file_dir}/more.hjson"], run_opts: ["+a"]}', + ) + _write(tmp_path / "a" / "more.hjson", '{run_opts: ["+a_more"]}') + second = _write(tmp_path / "b" / "ext.hjson", '{run_opts: ["+b"]}') + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={first}", f"blk={second}")) + + assert blk.run_opts[:4] == ["+tool", "+a", "+a_more", "+b"] + assert blk.cfg_extensions == [CfgExtension("blk", first), CfgExtension("blk", second)] + + def test_named_twice_is_an_error( + self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture + ) -> None: + """The same NAME=PATH twice would merge the extension twice.""" + ext = _write(tmp_path / "ext.hjson", '{run_opts: ["+x"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={ext}", f"blk={ext}") + + assert f"blk={ext} is given more than once" in dvsim_log.text + + def test_overrides_change_a_scalar(self, proj: Path, tmp_path: Path) -> None: + ext = _write( + tmp_path / "ext.hjson", + '{overrides: [{name: "blk_var", value: "new"}]}', + ) + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={ext}")) + + assert blk.blk_var == "new" + + def test_sets_a_scalar_the_cfg_leaves_empty(self, proj: Path, tmp_path: Path) -> None: + ext = _write(tmp_path / "ext.hjson", '{blk_empty: "set"}') + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={ext}")) + + assert blk.blk_empty == "set" + + @pytest.mark.parametrize( + ("key", "value"), [("tool", "xcelium"), ("scratch_path", "/elsewhere")] + ) + def test_conflicting_scalar_is_an_error( + self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture, key: str, value: str + ) -> None: + ext = _write(tmp_path / "ext.hjson", f'{{{key}: "{value}"}}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={ext}") + + assert f"Cannot extend cfg 'blk': '{ext}': Value for key '{key}'" in dvsim_log.text + + def test_tool_from_the_command_line_wins(self, proj: Path, tmp_path: Path) -> None: + """As in the cfg's own files, the extension's tool gives way to --tool.""" + ext = _write(tmp_path / "ext.hjson", '{tool: "xcelium"}') + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={ext}", argv=["--tool", "vcs"])) + + assert blk.tool == "vcs" + + def test_unresolved_import_is_an_error( + self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture + ) -> None: + ext = _write(tmp_path / "ext.hjson", '{import_cfgs: ["{nope}/x.hjson"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={ext}") + + assert "Can't resolve the name of an imported file" in dvsim_log.text + + def test_use_cfgs_is_an_error(self, proj: Path, tmp_path: Path) -> None: + ext = _write(tmp_path / "ext.hjson", '{use_cfgs: ["/x.hjson"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={ext}") + + def test_missing_file_is_an_error(self, proj: Path, tmp_path: Path) -> None: + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk={tmp_path / 'nope.hjson'}") + + +class TestNames: + """Matching an extension to a cfg by name.""" + + def test_unknown_name_is_an_error(self, proj: Path, tmp_path: Path) -> None: + ext = _write(tmp_path / "ext.hjson", '{run_opts: ["+x"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blkk={ext}") + + def test_name_as_the_cfgs_own_files_give_it( + self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture + ) -> None: + """An extension matches the name before overrides; one that matched nothing is an error. + + Here the cfg's own overrides rename it from 'blk' to 'blk2' after the extensions merge. + """ + cfg = proj / "hw" / "blk_sim_cfg.hjson" + cfg.write_text( + CHILD_CFG.replace("name: blk", 'name: "{n}"\n n: blk').replace( + "testplan:", 'overrides: [{name: "n", value: "blk2"}]\n testplan:' + ) + ) + ext = _write(tmp_path / "ext.hjson", '{run_opts: ["+x"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"blk2={ext}") + assert "(loaded: blk)" in dvsim_log.text + + (blk2,) = _load(proj, "top_sim_cfgs.hjson", f"blk={ext}").cfgs + assert (blk2.name, blk2.run_opts.count("+x")) == ("blk2", 1) + + def test_primary_name_is_an_error(self, proj: Path, tmp_path: Path) -> None: + ext = _write(tmp_path / "ext.hjson", '{run_opts: ["+x"]}') + + with pytest.raises(SystemExit): + _load(proj, "top_sim_cfgs.hjson", f"top={ext}") + + def test_messages(self) -> None: + exts = [CfgExtension("a", Path("/a")), CfgExtension("top", Path("/t"))] + + assert check_cfg_extensions_used(exts, exts[:1], ["a", "b"], "top") == [ + ( + "Cfg extension /t names the primary cfg 'top': only a cfg that runs jobs " + "(a child, or a cfg run on its own) can be extended." + ) + ] + assert check_cfg_extensions_used(exts[:1], [], ["b"], None) == [ + "Cfg extension /a names 'a', but no loaded cfg has that name (loaded: b)." + ] From 9649c09a2464efd7e8b0671467eb2dea785723e6 Mon Sep 17 00:00:00 2001 From: Harry Callahan Date: Mon, 26 Oct 2026 10:24:53 +0000 Subject: [PATCH 2/5] feat: record the git revision of each cfg extension in the reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A report says which project commit its results came from, but not which version of a --cfg-extend file was merged in. When the extension lives in its own repository, as it usually does, the results of the tests it adds can't be traced back to it. When a cfg merges an extension, dvsim now reads the git revision of the checkout that holds the extension file: the origin URL, with any credentials stripped as for the project's own, the commit, and whether the checkout is dirty, which includes the file itself being untracked or ignored. The sim flow JSON results list each cfg's extensions in command-line order with that revision (null outside git), the Markdown block report lists them with their revisions, and the HTML block report shows them in its header. This adds a key, extensions, to the JSON results: to each flow's .json and to each entry of flow_results in index.json. A flow that merged no extension leaves the key out, so the JSON of a run that doesn't use --cfg-extend is unchanged. A run that does use it writes JSON that an older dvsim can't load: SimFlowResults and SimResultsSummary forbid unknown keys (extra="forbid"), so for example dvsim-admin report gen of an earlier release fails to validate it. Readers that parse the JSON leniently are not affected. The JSON has no schema version that a reader could check; making the models ignore unknown keys, or adding a version, is left to a later change. AI-assisted (Claude Code) — reviewed and approved by author Co-Authored-By: Claude Signed-off-by: Harry Callahan --- doc/design_doc.md | 3 + src/dvsim/flow/base.py | 13 +- src/dvsim/report/data.py | 36 ++++++ src/dvsim/sim/data.py | 30 ++++- src/dvsim/sim/flow.py | 3 + src/dvsim/sim/report.py | 15 ++- src/dvsim/templates/reports/block_report.html | 9 ++ src/dvsim/utils/git.py | 30 +++++ tests/flow/test_extension.py | 27 +++- tests/report/test_extensions.py | 117 ++++++++++++++++++ tests/utils/test_git.py | 49 ++++++++ 11 files changed, 322 insertions(+), 10 deletions(-) create mode 100644 tests/report/test_extensions.py diff --git a/doc/design_doc.md b/doc/design_doc.md index 79869c28..6fa789fa 100644 --- a/doc/design_doc.md +++ b/doc/design_doc.md @@ -166,6 +166,9 @@ The usual merge rules apply: the extension's list entries follow the configurati In an extension, `{self_dir}` is still the directory of the configuration file that dvsim loaded, so the extension names its own files with `{file_dir}`. Several extensions apply in command-line order. An extension whose `NAME` matches no loaded configuration is an error, as is naming one extension twice. +The sim reports and JSON results list the extensions that each configuration merged, with the git revision of the checkout that holds each extension file: its `origin` (without credentials), its commit, and whether it was dirty, which includes the extension file itself not being tracked. +Files that an extension imports from elsewhere are not recorded. +The JSON results of a configuration that merged no extension have no `extensions` key, as before extensions were recorded. ### Parser stage diff --git a/src/dvsim/flow/base.py b/src/dvsim/flow/base.py index 152ddc35..51aad8ef 100644 --- a/src/dvsim/flow/base.py +++ b/src/dvsim/flow/base.py @@ -23,6 +23,7 @@ from dvsim.job.data import CompletedJobStatus, JobSpec, WorkspaceConfig from dvsim.job.status import JobStatus from dvsim.logging import log +from dvsim.report.data import CfgExtensionMeta from dvsim.scheduler.resources import UnknownResourcePolicy from dvsim.scheduler.runner import ( build_default_scheduler_backend, @@ -34,7 +35,7 @@ rm_path, subst_wildcards, ) -from dvsim.utils.git import git_commit_hash, git_is_dirty +from dvsim.utils.git import git_commit_hash, git_is_dirty, git_revision if TYPE_CHECKING: from dvsim.job.deploy import Deploy @@ -148,9 +149,9 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: # configuration". self.is_primary_cfg = "use_cfgs" in hjson_data - # The cfg extensions (--cfg-extend) merged into this cfg, and the name - # that they were matched against. - self.cfg_extensions: list[CfgExtension] = [] + # The cfg extensions (--cfg-extend) merged into this cfg, in order, as + # the reports describe them, and the name that they were matched against. + self.cfg_extensions: dict[CfgExtension, CfgExtensionMeta] = {} self.cfg_extend_name: str | None = None if not self.is_primary_cfg: @@ -245,7 +246,9 @@ def _merge_cfg_extensions(self) -> None: except RuntimeError as err: log.error("Cannot extend cfg %r: %s", name, err) sys.exit(1) - self.cfg_extensions.append(ext) + self.cfg_extensions[ext] = CfgExtensionMeta( + path=str(ext.path), revision=git_revision(ext.path) + ) def _expand(self) -> None: """Expand wildcards after merging hjson. diff --git a/src/dvsim/report/data.py b/src/dvsim/report/data.py index e385d204..08aadd0f 100644 --- a/src/dvsim/report/data.py +++ b/src/dvsim/report/data.py @@ -4,9 +4,13 @@ """Report data models.""" +from pathlib import Path + from pydantic import BaseModel, ConfigDict __all__ = ( + "CfgExtensionMeta", + "GitRevisionMeta", "IPMeta", "ToolMeta", ) @@ -50,3 +54,35 @@ class ToolMeta(BaseModel): """Name of the tool.""" version: str """Version of the tool.""" + + +class CfgExtensionMeta(BaseModel): + """A cfg extension (--cfg-extend) merged into a flow's cfg.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + + path: str + """Path of the extension file.""" + revision: "GitRevisionMeta | None" = None + """The git revision of the checkout that holds the extension file (not of + any files it imports from elsewhere), or None if it isn't in git.""" + + @property + def name(self) -> str: + """The name of the extension file.""" + return Path(self.path).name + + +class GitRevisionMeta(BaseModel): + """The git revision of a checkout, when the run started.""" + + model_config = ConfigDict(frozen=True, extra="forbid") + + origin: str | None + """URL of the ``origin`` remote (credentials stripped), or None.""" + commit: str + """Git commit.""" + commit_short: str + """Shortened git commit.""" + dirty: bool + """Whether the checkout had uncommitted changes, or didn't track the file.""" diff --git a/src/dvsim/sim/data.py b/src/dvsim/sim/data.py index 888dfad1..2f61874b 100644 --- a/src/dvsim/sim/data.py +++ b/src/dvsim/sim/data.py @@ -7,10 +7,11 @@ from collections.abc import Mapping from datetime import datetime from pathlib import Path +from typing import Any -from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, SerializerFunctionWrapHandler, model_serializer -from dvsim.report.data import IPMeta, ToolMeta +from dvsim.report.data import CfgExtensionMeta, IPMeta, ToolMeta from dvsim.sim_results import BucketedFailures __all__ = ( @@ -169,6 +170,17 @@ def flattened(self) -> dict[str, float | None]: return items +def _without_empty_extensions(data: dict[str, Any]) -> dict[str, Any]: + """Leave out the extensions of a flow that has none. + + A flow run without --cfg-extend then has the keys it had before extensions were recorded, so a + reader built for those (whose models forbid unknown keys) can still load it. + """ + if not data.get("extensions"): + data.pop("extensions", None) + return data + + class SimFlowResults(BaseModel): """Flow results data.""" @@ -207,6 +219,13 @@ class SimFlowResults(BaseModel): percent: float """Percentage test pass rate.""" + extensions: tuple[CfgExtensionMeta, ...] = () + """The cfg extensions (--cfg-extend) named for this flow.""" + + @model_serializer(mode="wrap") + def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: + return _without_empty_extensions(handler(self)) + def summary(self) -> "SimFlowSummary": """Load results from JSON file. @@ -247,6 +266,13 @@ class SimFlowSummary(BaseModel): percent: float """Percentage test pass rate.""" + extensions: tuple[CfgExtensionMeta, ...] = () + """The cfg extensions (--cfg-extend) named for this flow.""" + + @model_serializer(mode="wrap") + def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: + return _without_empty_extensions(handler(self)) + @staticmethod def load(path: Path) -> "SimFlowSummary": """Load results from JSON file. diff --git a/src/dvsim/sim/flow.py b/src/dvsim/sim/flow.py index a907a832..9a0fc75a 100644 --- a/src/dvsim/sim/flow.py +++ b/src/dvsim/sim/flow.py @@ -877,6 +877,8 @@ def make_test_result(tr) -> TestResult | None: if failures.buckets: self.errors_seen = True + extensions = self.cfg_extensions.values() + # --- Final result --- return SimFlowResults( block=block, @@ -893,6 +895,7 @@ def make_test_result(tr) -> TestResult | None: passed=total_passed, total=total_runs, percent=100.0 * total_passed / total_runs if total_runs else 0.0, + extensions=tuple(extensions), ) def _fake_policy(self, job: JobSpec) -> JobStatus: diff --git a/src/dvsim/sim/report.py b/src/dvsim/sim/report.py index bdda6311..0574a588 100644 --- a/src/dvsim/sim/report.py +++ b/src/dvsim/sim/report.py @@ -14,7 +14,7 @@ from dvsim.logging import log from dvsim.report.artifacts import ReportArtifacts, display_report, render_static_content -from dvsim.report.data import IPMeta +from dvsim.report.data import CfgExtensionMeta, IPMeta from dvsim.sim.data import SimFlowResults, SimResultsSummary from dvsim.templates.render import render_template from dvsim.utils import TS_FORMAT_LONG @@ -159,6 +159,16 @@ def render( return artifacts +def _revision(ext: CfgExtensionMeta) -> str: + """Describe the git revision of a cfg extension, for the Markdown reports.""" + rev = ext.revision + if rev is None: + return "" + dirty = ", dirty" if rev.dirty else "" + origin = f", from {rev.origin}" if rev.origin else "" + return f" (revision `{rev.commit_short}`{dirty}{origin})" + + class MarkdownReportRenderer: """Renders a Markdown report of the sim results.""" @@ -213,6 +223,9 @@ def render_block(self, results: SimFlowResults) -> ReportArtifacts: if len(results.stages) > 0 and testplan_ref: report_md += f"\n### [Testplan]({testplan_ref})" report_md += f"\n### Simulator: {results.tool.name.upper()}" + if results.extensions: + report_md += "\n### Cfg extensions:\n" + report_md += "".join(f"\n* `{ext.path}`{_revision(ext)}" for ext in results.extensions) # Record a summary of the simulation results, coverage, and failure buckets, if applicable. result_summary = self.render_block_results(results) diff --git a/src/dvsim/templates/reports/block_report.html b/src/dvsim/templates/reports/block_report.html index 4f6177fd..fbf31341 100644 --- a/src/dvsim/templates/reports/block_report.html +++ b/src/dvsim/templates/reports/block_report.html @@ -58,6 +58,15 @@

Simulation Results: {{ block.variant_name(sep='/') }}

Tool: {{ tool.name }} [{{ tool.version }}] + {% for ext in results.extensions %} + {% set rev = ext.revision %} + + Extension: {{ ext.name }} + {%- if rev %} sha: {{ rev.commit_short }}{% endif %} + {%- if rev and rev.dirty %} (dirty){% endif %} + + {% endfor %} {% if build_seed %} Build seed: {{ build_seed }} diff --git a/src/dvsim/utils/git.py b/src/dvsim/utils/git.py index 0776d9de..a896f1bb 100644 --- a/src/dvsim/utils/git.py +++ b/src/dvsim/utils/git.py @@ -8,8 +8,10 @@ from urllib.parse import urlsplit, urlunsplit from git import Repo +from git.exc import GitCommandError, GitError from dvsim.logging import log +from dvsim.report.data import GitRevisionMeta __all__ = ("repo_root",) @@ -124,3 +126,31 @@ def git_https_url_with_commit(path: Path | None = None) -> str | None: url = "https://github.com/" + url.removeprefix(prefix) return f"{url}/tree/{commit}" + + +def git_revision(path: Path) -> GitRevisionMeta | None: + """Describe the checkout that holds path, for run metadata. + + Returns None if path isn't in a git checkout or its revision can't be read. + The checkout counts as dirty if path itself isn't tracked (untracked or + ignored), as the commit doesn't hold it. + """ + root = repo_root(path) + if root is None: + return None + try: + repo = Repo(root) + try: + repo.git.ls_files("--error-unmatch", str(path)) + tracked = True + except GitCommandError: + tracked = False + return GitRevisionMeta( + origin=git_origin_url(path=root), + commit=git_commit_hash(path=root), + commit_short=git_commit_hash(path=root, short=True), + dirty=git_is_dirty(path=root) or not tracked, + ) + except (GitError, ValueError) as err: + log.debug("Cannot read the git revision of %s: %s", root, err) + return None diff --git a/tests/flow/test_extension.py b/tests/flow/test_extension.py index abcacd44..6bda90aa 100644 --- a/tests/flow/test_extension.py +++ b/tests/flow/test_extension.py @@ -130,7 +130,7 @@ def test_lists_follow_the_cfgs_own(self, proj: Path, tmp_path: Path, top: str) - assert blk.build_opts[:3] == [f"-f {proj}/sv.f", "+incdir+/overlay", "/overlay/bind.sv"] assert blk.run_opts[:1] == ["+tool"] assert [t.name for t in blk.tests] == ["t1", "t2"] - assert blk.cfg_extensions == [CfgExtension("blk", ext)] + assert list(blk.cfg_extensions) == [CfgExtension("blk", ext)] def test_imports_and_order(self, proj: Path, tmp_path: Path) -> None: """Extensions apply in command-line order, each with its own imports.""" @@ -144,7 +144,7 @@ def test_imports_and_order(self, proj: Path, tmp_path: Path) -> None: blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={first}", f"blk={second}")) assert blk.run_opts[:4] == ["+tool", "+a", "+a_more", "+b"] - assert blk.cfg_extensions == [CfgExtension("blk", first), CfgExtension("blk", second)] + assert list(blk.cfg_extensions) == [CfgExtension("blk", first), CfgExtension("blk", second)] def test_named_twice_is_an_error( self, proj: Path, tmp_path: Path, dvsim_log: pytest.LogCaptureFixture @@ -265,3 +265,26 @@ def test_messages(self) -> None: assert check_cfg_extensions_used(exts[:1], [], ["b"], None) == [ "Cfg extension /a names 'a', but no loaded cfg has that name (loaded: b)." ] + + +class TestRevision: + """The git revision of each extension.""" + + def test_revision_is_recorded(self, proj: Path, tmp_path: Path) -> None: + overlay = tmp_path / "overlay" + ext = _write(overlay / "dv" / "ext.hjson", '{run_opts: ["+x"]}') + git = ["git", "-C", str(overlay)] + subprocess.run([*git, "init", "-q"], check=True) + subprocess.run([*git, "add", "dv"], check=True) + subprocess.run([*git, "commit", "-q", "-m", "init"], check=True) + head = subprocess.run( + [*git, "rev-parse", "HEAD"], check=True, capture_output=True, text=True + ).stdout.strip() + outside = _write(tmp_path / "loose" / "ext.hjson", '{run_opts: ["+y"]}') + + blk = _blk(_load(proj, "top_sim_cfgs.hjson", f"blk={ext}", f"blk={outside}")) + + revision = blk.cfg_extensions[CfgExtension("blk", ext)].revision + assert revision is not None + assert (revision.commit, revision.dirty) == (head, False) + assert blk.cfg_extensions[CfgExtension("blk", outside)].revision is None diff --git a/tests/report/test_extensions.py b/tests/report/test_extensions.py new file mode 100644 index 00000000..839ea6f4 --- /dev/null +++ b/tests/report/test_extensions.py @@ -0,0 +1,117 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Test how reports show the cfg extensions (--cfg-extend) of a flow.""" + +import json +from datetime import datetime, timezone +from pathlib import Path + +from dvsim.report.data import CfgExtensionMeta, GitRevisionMeta, IPMeta, ToolMeta +from dvsim.sim.data import SimFlowResults, SimResultsSummary +from dvsim.sim.report import HtmlReportRenderer, MarkdownReportRenderer +from dvsim.sim_results import BucketedFailures + +__all__ = () + +TS = datetime(2026, 1, 2, 3, 4, 5, tzinfo=timezone.utc) + + +def _results(*extensions: CfgExtensionMeta) -> SimFlowResults: + return SimFlowResults( + block=IPMeta( + name="blk", + commit="c" * 40, + commit_short="c" * 7, + branch="b", + url=None, + revision_info=None, + ), + tool=ToolMeta(name="vcs", version="1"), + timestamp=TS, + build_seed=None, + testplan_ref=None, + stages={}, + coverage=None, + cov_report_page=None, + vplan_report_page=None, + failed_jobs=BucketedFailures(buckets={}), + passed=1, + total=1, + percent=100.0, + extensions=extensions, + ) + + +def _summary(results: SimFlowResults) -> SimResultsSummary: + return SimResultsSummary( + top=None, + version=None, + timestamp=TS, + build_seed=None, + flow_results={"blk": results.summary()}, + report_path=Path("/reports"), + ) + + +NO_GIT = CfgExtensionMeta(path="/overlay/ok.hjson") +TRACKED = CfgExtensionMeta( + path="/overlay/dv/ext.hjson", + revision=GitRevisionMeta( + origin="git@example.com:org/overlay.git", + commit="a" * 40, + commit_short="aaaaaaa", + dirty=True, + ), +) + + +class TestExtensionsInReports: + """The extensions and their revisions in the results and reports.""" + + def test_summary_keeps_extensions(self) -> None: + assert _results(TRACKED, NO_GIT).summary().extensions == (TRACKED, NO_GIT) + + def test_json_round_trip(self) -> None: + results = _results(TRACKED, NO_GIT) + assert SimFlowResults.model_validate_json(results.model_dump_json()) == results + + def test_json_without_extensions_leaves_out_the_key(self) -> None: + results = _results() + + assert "extensions" not in json.loads(results.model_dump_json()) + flow = json.loads(_summary(results).model_dump_json())["flow_results"]["blk"] + assert "extensions" not in flow + assert SimFlowResults.model_validate_json(results.model_dump_json()) == results + + def test_json_with_extensions_has_the_key(self) -> None: + results = _results(NO_GIT) + + assert json.loads(results.model_dump_json())["extensions"] == [ + {"path": "/overlay/ok.hjson", "revision": None} + ] + flow = json.loads(_summary(results).model_dump_json())["flow_results"]["blk"] + assert flow["extensions"] == [{"path": "/overlay/ok.hjson", "revision": None}] + + def test_markdown_gives_the_revision(self) -> None: + block = MarkdownReportRenderer().render_block(_results(TRACKED, NO_GIT))["report.md"] + + assert ( + "### Cfg extensions:\n\n* `/overlay/dv/ext.hjson` (revision `aaaaaaa`, dirty, " + "from git@example.com:org/overlay.git)\n* `/overlay/ok.hjson`" + ) in block + + def test_markdown_without_extensions(self) -> None: + block = MarkdownReportRenderer().render_block(_results())["report.md"] + + assert "Cfg extensions" not in block + + def test_html_gives_the_revision(self) -> None: + results = _results(TRACKED) + + page = HtmlReportRenderer().render(_summary(results), {"blk": results})["blk.html"] + + assert "Extension: ext.hjson sha: aaaaaaa (dirty)" in page + assert "git@example.com:org/overlay.git" in page + assert "a" * 40 in page diff --git a/tests/utils/test_git.py b/tests/utils/test_git.py index 5516fe22..77386ecf 100644 --- a/tests/utils/test_git.py +++ b/tests/utils/test_git.py @@ -10,11 +10,13 @@ from git import Repo from hamcrest import assert_that, calling, equal_to, raises +from dvsim.report.data import GitRevisionMeta from dvsim.utils.git import ( git_commit_hash, git_https_url_with_commit, git_is_dirty, git_origin_url, + git_revision, repo_root, ) @@ -226,3 +228,50 @@ def test_git_https_url_with_commit(tmp_path: Path, url: str, expected: str) -> N git_https_url_with_commit(tmp_path), equal_to(expected.format(commit=commit)), ) + + @staticmethod + def test_git_revision(tmp_path: Path) -> None: + """Test git_revision describes the checkout holding a file.""" + ext = tmp_path / "sub" / "ext.hjson" + ext.parent.mkdir() + ext.write_text("{}") + + # Not in a git repo, or in one without a commit: nothing to say. + assert_that(git_revision(ext), equal_to(None)) + r = Repo.init(path=tmp_path) + assert_that(git_revision(ext), equal_to(None)) + + r.index.add([ext]) + r.index.commit("initial commit") + commit = r.head.commit.hexsha + + assert_that( + git_revision(ext), + equal_to( + GitRevisionMeta( + origin=None, + commit=commit, + commit_short=git_commit_hash(tmp_path, short=True), + dirty=False, + ) + ), + ) + + # An untracked or ignored file isn't in the commit, so its checkout + # counts as dirty; the other files there are still clean. + untracked = tmp_path / "sub" / "new.hjson" + untracked.write_text("{}") + ignored = tmp_path / "sub" / "ignored.hjson" + ignored.write_text("{}") + (tmp_path / ".git" / "info" / "exclude").write_text("ignored.hjson\n") + assert_that(git_revision(ext).dirty, equal_to(False)) + assert_that(git_revision(untracked).dirty, equal_to(True)) + assert_that(git_revision(ignored).dirty, equal_to(True)) + untracked.unlink() + + r.create_remote("origin", "https://oauth2:token@example.com/org/overlay.git") + ext.write_text("{x: 1}") + revision = git_revision(ext) + assert revision is not None + assert_that(revision.origin, equal_to("https://example.com/org/overlay.git")) + assert_that(revision.dirty, equal_to(True)) From 2704783ea2b73ef0b9f095d05aedf5382990549d Mon Sep 17 00:00:00 2001 From: Harry Callahan Date: Wed, 4 Nov 2026 09:41:50 +0000 Subject: [PATCH 3/5] refactor: render a set of failure buckets from a template macro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The block report's HTML table of failure buckets moves into a macro, and MarkdownReportRenderer.render_bucket_summary takes the buckets, a title and a noun rather than the whole flow results, so that both can render a second set of buckets under another heading. The reports are unchanged. AI-assisted (Claude Code) — reviewed and approved by author Co-Authored-By: Claude Signed-off-by: Harry Callahan --- src/dvsim/sim/report.py | 22 +++--- src/dvsim/templates/reports/block_report.html | 77 ++++++++++--------- 2 files changed, 53 insertions(+), 46 deletions(-) diff --git a/src/dvsim/sim/report.py b/src/dvsim/sim/report.py index 0574a588..7798ea6d 100644 --- a/src/dvsim/sim/report.py +++ b/src/dvsim/sim/report.py @@ -16,6 +16,7 @@ from dvsim.report.artifacts import ReportArtifacts, display_report, render_static_content from dvsim.report.data import CfgExtensionMeta, IPMeta from dvsim.sim.data import SimFlowResults, SimResultsSummary +from dvsim.sim_results import BucketedFailures from dvsim.templates.render import render_template from dvsim.utils import TS_FORMAT_LONG from dvsim.utils.fs import relative_to @@ -285,7 +286,7 @@ def render_block_results(self, results: SimFlowResults) -> str: report_md += "\n\n" + coverage_table if results.failed_jobs.buckets: - bucket_summary = self.render_bucket_summary(results) + bucket_summary = self.render_bucket_summary(results.failed_jobs) if bucket_summary: report_md += "\n\n" + bucket_summary @@ -379,16 +380,21 @@ def render_coverage_table(self, results: SimFlowResults) -> str: return report_md - def render_bucket_summary(self, results: SimFlowResults) -> str: + def render_bucket_summary( + self, + failures: BucketedFailures, + title: str = "Failure Buckets", + noun: str = "failure", + ) -> str: """Generate a Markdown string with a summary of the buckets (failures/killed).""" - lines = [(0, "## Failure Buckets")] + lines = [(0, f"## {title}")] for bucket, tests in sorted( - results.failed_jobs.buckets.items(), + failures.buckets.items(), key=lambda kv: len(kv[1]), reverse=True, ): - lines.append((0, f"* `{bucket}` has {len(tests)} {_plural('failure', tests)}:")) + lines.append((0, f"* `{bucket}` has {len(tests)} {_plural(noun, tests)}:")) grouped_tests = defaultdict(list) for job in tests: @@ -396,9 +402,7 @@ def render_bucket_summary(self, results: SimFlowResults) -> str: displayed = list(grouped_tests.items())[: self.MAX_TESTS_PER_BUCKET] for name, reseeds in displayed: - lines.append( - (1, f"* Test {name} has {len(reseeds)} {_plural('failure', reseeds)}.") - ) + lines.append((1, f"* Test {name} has {len(reseeds)} {_plural(noun, reseeds)}.")) for failure in reseeds[: self.MAX_RESEEDS_PER_BUCKETED_TEST]: lines.append((2, f"* {failure.qual_name}\\")) @@ -411,7 +415,7 @@ def render_bucket_summary(self, results: SimFlowResults) -> str: extra = len(reseeds) - self.MAX_RESEEDS_PER_BUCKETED_TEST if extra > 0: - lines.append((2, f"* ... and {extra} more {_plural('failure', extra)}.")) + lines.append((2, f"* ... and {extra} more {_plural(noun, extra)}.")) extra = len(grouped_tests) - self.MAX_TESTS_PER_BUCKET if extra > 0: diff --git a/src/dvsim/templates/reports/block_report.html b/src/dvsim/templates/reports/block_report.html index fbf31341..74087e20 100644 --- a/src/dvsim/templates/reports/block_report.html +++ b/src/dvsim/templates/reports/block_report.html @@ -12,6 +12,45 @@ {% set stages = results.stages %} {% set coverage = results.coverage %} {% block content %} +{% macro bucket_table(title, buckets) %} +
+
+

{{ title }}

+ + + + + + + + + + + + {% for msg, job_list in buckets.items() %} + + + + + {% for job in job_list %} + + + + + + + + {% endfor %} + {% endfor %} + +
  Testseedlinelog context
{{ msg }}{{ job_list|length }} test run{{ "s" if job_list|length > 1 else "" }}
{{ job.name }}{{ job.seed }}{{ job.line }} + {% for context_line in job.log_context %} +
{{ context_line }}
+ {% endfor %} +
+
+
+{% endmacro %}
@@ -213,43 +252,7 @@

Simulation Results: {{ block.variant_name(sep='/') }}

{% endif %} {% if failed_jobs %} -
-
-

Error Messages

- - - - - - - - - - - - {% for msg, job_list in failed_jobs.items() %} - - - - - {% for job in job_list %} - - - - - - - - {% endfor %} - {% endfor %} - -
  Testseedlinelog context
{{ msg }}{{ job_list|length }} test run{{ "s" if job_list|length > 1 else "" }}
{{ job.name }}{{ job.seed }}{{ job.line }} - {% for context_line in job.log_context %} -
{{ context_line }}
- {% endfor %} -
-
-
+ {{ bucket_table("Error Messages", failed_jobs) }} {% endif %}
{% endblock %} From fb0e5e3ecf76d0feab5e80d25d94f67e526f7d19 Mon Sep 17 00:00:00 2001 From: Harry Callahan Date: Wed, 4 Nov 2026 09:42:19 +0000 Subject: [PATCH 4/5] feat: mark the job statuses that the scheduler cancelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A job's status doesn't always say whether the job itself failed. When a signal stops the scheduler, a running job is killed and its launcher may report it as failed, though the scheduler replaces the reason with its own. Add a cancelled flag to JobStatusInfo, set on every reason that the scheduler gives when it stops a job for something outside it (a failed or killed dependency, or a signal), so that a failure of the job itself can be told apart. The next commit uses it. AI-assisted (Claude Code) — reviewed and approved by author Co-Authored-By: Claude Signed-off-by: Harry Callahan --- src/dvsim/job/data.py | 2 ++ src/dvsim/scheduler/core.py | 20 ++++++++++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/dvsim/job/data.py b/src/dvsim/job/data.py index bb269bd3..6fac0698 100644 --- a/src/dvsim/job/data.py +++ b/src/dvsim/job/data.py @@ -154,6 +154,8 @@ class JobStatusInfo(BaseModel): """Relevant line information (in the job script or the job itself).""" context: Sequence[str] | None = None """Arbitrary context strings.""" + cancelled: bool = False + """The scheduler stopped the job for a reason outside it (a failed dependency or a signal).""" class CompletedJobStatus(BaseModel): diff --git a/src/dvsim/scheduler/core.py b/src/dvsim/scheduler/core.py index 026ddd0e..6c87736e 100644 --- a/src/dvsim/scheduler/core.py +++ b/src/dvsim/scheduler/core.py @@ -69,20 +69,28 @@ class JobRecord: # Standard context messages used for killed/failed jobs in the scheduler. FAILED_DEP = JobStatusInfo( - message="Job cancelled because one of its dependencies failed or was killed." + message="Job cancelled because one of its dependencies failed or was killed.", + cancelled=True, ) ALL_FAILED_DEP = JobStatusInfo( - message="Job cancelled because all of its dependencies failed or were killed." + message="Job cancelled because all of its dependencies failed or were killed.", + cancelled=True, ) KILLED_SCHEDULED = JobStatusInfo( - message="Job cancelled because one of its dependencies was killed." + message="Job cancelled because one of its dependencies was killed.", + cancelled=True, +) +KILLED_QUEUED = JobStatusInfo( + message="Job killed whilst waiting to begin execution.", + cancelled=True, ) -KILLED_QUEUED = JobStatusInfo(message="Job killed whilst waiting to begin execution.") KILLED_RUNNING_SIGINT = JobStatusInfo( - message="Job killed by a SIGINT signal to the scheduler whilst executing." + message="Job killed by a SIGINT signal to the scheduler whilst executing.", + cancelled=True, ) KILLED_RUNNING_SIGTERM = JobStatusInfo( - message="Job killed by a SIGTERM signal to the scheduler whilst executing." + message="Job killed by a SIGTERM signal to the scheduler whilst executing.", + cancelled=True, ) From 78638e7cdc4bd6e0d588780fbd1b8c584376fffe Mon Sep 17 00:00:00 2001 From: Harry Callahan Date: Wed, 4 Nov 2026 09:42:24 +0000 Subject: [PATCH 5/5] feat: add xfail to mark tests that are expected to fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A test that fails for a known reason (a missing feature, a bug with an open issue) either stays in the regression and fails it every night, or leaves the regression and nobody notices when it starts passing. Add an xfail key to tests, giving the reason. A run of such a test that fails is XFAIL: it is listed under "Expected Failures" in its own buckets, and doesn't fail the run or its exit code. It still counts in the total and not as passed, so the pass rate stays the share of runs that passed, and XFAIL runs are counted beside it. A run that passes is XPASS and counts as a failure (strict), so the list of expected failures stays current. A run that is killed (it timed out or couldn't be launched) or that the scheduler cancelled (a failed build, a signal) is a failure as before, so a hang or a broken environment doesn't pass for the expected failure. The reports show the reason on the test, an XFail column in the Markdown tables when a flow has xfail tests, and the counts in the JSON. The new JSON keys are left out where they are zero or empty, so a run without xfail tests writes the same JSON as before, which released dvsim (whose models forbid unknown keys) can still load. AI-assisted (Claude Code) — reviewed and approved by author Co-Authored-By: Claude Signed-off-by: Harry Callahan --- doc/design_doc.md | 14 + src/dvsim/job/data.py | 27 ++ src/dvsim/job/deploy.py | 6 + src/dvsim/scheduler/core.py | 18 +- src/dvsim/sim/data.py | 62 ++- src/dvsim/sim/flow.py | 20 +- src/dvsim/sim/report.py | 52 ++- src/dvsim/sim_results.py | 84 ++-- src/dvsim/templates/reports/block_report.html | 15 +- .../templates/reports/summary_report.html | 7 +- src/dvsim/test.py | 14 + src/dvsim/testplan.py | 6 + tests/sim/__init__.py | 5 + tests/sim/test_xfail.py | 373 ++++++++++++++++++ 14 files changed, 659 insertions(+), 44 deletions(-) create mode 100644 tests/sim/__init__.py create mode 100644 tests/sim/test_xfail.py diff --git a/doc/design_doc.md b/doc/design_doc.md index 6fa789fa..d8bd7e8d 100644 --- a/doc/design_doc.md +++ b/doc/design_doc.md @@ -239,6 +239,20 @@ A version of the generated report (in Markdown format) is logged to the console. A slightly longer version of the generated report is written to an HTML file. The HTML report is optionally published to the report server. +A test can be marked as expected to fail, giving the reason: + +```hjson +tests: [ + {name: "foo_smoke", xfail: "foo doesn't support bar yet (issue #123)"} +] +``` + +A run of such a test that fails is reported as XFAIL: it has its own buckets and count, and doesn't fail the invocation. +It still counts in the total that the pass rate divides by, so the pass rate is the share of runs that passed. +A run that passes is reported as XPASS and counts as a failure (strict), so the list of expected failures stays current. +Neither counts as passed. +A run that times out, can't be launched, or is cancelled by the scheduler is a failure as usual, not an XFAIL. + # Design ## Language diff --git a/src/dvsim/job/data.py b/src/dvsim/job/data.py index 6fac0698..6e14bfeb 100644 --- a/src/dvsim/job/data.py +++ b/src/dvsim/job/data.py @@ -23,6 +23,8 @@ "JobSpec", "JobStatusInfo", "WorkspaceConfig", + "is_xfail", + "is_xpass", ) @@ -131,6 +133,9 @@ class JobSpec(BaseModel): fail_patterns: Sequence[str] """regex patterns to match on to determine if the job has failed.""" + xfail: str | None = None + """Why the job is expected to fail, or None if it is expected to pass.""" + @property def id(self) -> str: """Returns a string that uniquely identifies this job.""" @@ -203,3 +208,25 @@ class CompletedJobStatus(BaseModel): """Status of the job.""" fail_msg: JobStatusInfo | None """Error message.""" + + xfail: str | None = None + """Why the job is expected to fail, or None if it is expected to pass.""" + + +def is_xfail(xfail: str | None, status: JobStatus, info: JobStatusInfo | None) -> bool: + """Whether a job that is expected to fail failed (XFAIL). + + Only a job that ran and failed counts. A killed job (a timeout, or a job that could not be + launched) and a job the scheduler cancelled are failures, so that a hang or a broken + environment doesn't pass for the expected failure. + """ + return ( + xfail is not None + and status == JobStatus.FAILED + and not (info is not None and info.cancelled) + ) + + +def is_xpass(xfail: str | None, status: JobStatus) -> bool: + """Whether a job that is expected to fail passed (XPASS), which counts as a failure.""" + return xfail is not None and status == JobStatus.PASSED diff --git a/src/dvsim/job/deploy.py b/src/dvsim/job/deploy.py index 02146e00..f239c023 100644 --- a/src/dvsim/job/deploy.py +++ b/src/dvsim/job/deploy.py @@ -93,6 +93,9 @@ def __init__(self, sim_cfg: "FlowCfg") -> None: # this current job to run self.needs_all_dependencies_passing = True + # Why the job is expected to fail, if it is (only tests can be). + self.xfail: str | None = None + # These variables will be extracted from the hjson file by _set_attrs, # and then _check_attrs checks that they were indeed extracted. Define # placeholder values here to allow a type checker to know the class has @@ -183,6 +186,7 @@ def get_job_spec(self) -> "JobSpec": post_finish=self.post_finish(), pass_patterns=self.pass_patterns, fail_patterns=self.fail_patterns, + xfail=self.xfail, ) def _define_attrs(self) -> None: @@ -763,6 +767,8 @@ def _set_attrs(self) -> None: # When running a test, we should always renew the output directory self.renew_odir = True + self.xfail = self.test_obj.xfail or None + # 'test' is used as a substitution variable in the HJson. self.test = self.name self.build_mode = self.test_obj.build_mode.name diff --git a/src/dvsim/scheduler/core.py b/src/dvsim/scheduler/core.py index 6c87736e..2eaa4086 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, JobSpec, JobStatusInfo, is_xfail, is_xpass from dvsim.job.status import JobStatus from dvsim.logging import log from dvsim.runtime.backend import RuntimeBackend @@ -278,12 +278,23 @@ def _change_job_status( job.status_info = info if new_status != JobStatus.RUNNING: + level = log.VERBOSE + expected = "" + if is_xfail(job.spec.xfail, new_status, info): + level = log.INFO + expected = f" (expected to fail: {job.spec.xfail})" + elif is_xpass(job.spec.xfail, new_status): + level = log.WARNING + expected = f" (but expected to fail: {job.spec.xfail})" + elif new_status in (JobStatus.FAILED, JobStatus.KILLED): + level = log.ERROR log.log( - log.ERROR if new_status in (JobStatus.FAILED, JobStatus.KILLED) else log.VERBOSE, - "Status change to [%s: %s] for %s", + level, + "Status change to [%s: %s] for %s%s", new_status.shorthand, new_status.name.capitalize(), job.spec.full_name, + expected, ) for cb in self._on_job_status_change: @@ -413,6 +424,7 @@ async def run(self) -> list[CompletedJobStatus]: else 0.0, status=job.status, fail_msg=job.status_info, + xfail=job.spec.xfail, ) for job in self._jobs.values() ] diff --git a/src/dvsim/sim/data.py b/src/dvsim/sim/data.py index 2f61874b..0b35f3d4 100644 --- a/src/dvsim/sim/data.py +++ b/src/dvsim/sim/data.py @@ -42,6 +42,17 @@ class TestResult(BaseModel): percent: float """Percentage test pass rate.""" + xfailed: int = 0 + """Number of runs that failed, as expected (XFAIL). Not counted as passed.""" + xpassed: int = 0 + """Number of runs that passed, but were expected to fail (XPASS). Counted as failures.""" + xfail: str | None = None + """Why the test is expected to fail, or None if it is expected to pass.""" + + @model_serializer(mode="wrap") + def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: + return _without_unused_xfail(handler(self)) + class Testpoint(BaseModel): """Testpoint.""" @@ -58,6 +69,15 @@ class Testpoint(BaseModel): percent: float """Percentage test pass rate.""" + xfailed: int = 0 + """Number of runs that failed, as expected (XFAIL). Not counted as passed.""" + xpassed: int = 0 + """Number of runs that passed, but were expected to fail (XPASS). Counted as failures.""" + + @model_serializer(mode="wrap") + def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: + return _without_unused_xfail(handler(self)) + class TestStage(BaseModel): """Test stages.""" @@ -74,6 +94,15 @@ class TestStage(BaseModel): percent: float """Percentage test pass rate.""" + xfailed: int = 0 + """Number of runs that failed, as expected (XFAIL). Not counted as passed.""" + xpassed: int = 0 + """Number of runs that passed, but were expected to fail (XPASS). Counted as failures.""" + + @model_serializer(mode="wrap") + def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: + return _without_unused_xfail(handler(self)) + class CodeCoverageMetrics(BaseModel): """CodeCoverage metrics.""" @@ -181,6 +210,22 @@ def _without_empty_extensions(data: dict[str, Any]) -> dict[str, Any]: return data +def _without_unused_xfail(data: dict[str, Any]) -> dict[str, Any]: + """Leave out the xfail keys of results that have no xfail test. + + Results without xfail tests then have the keys they had before xfail was added, so a reader + built for those (whose models forbid unknown keys) can still load them. + """ + for key in ("xfailed", "xpassed"): + if data.get(key) == 0: + data.pop(key) + if "xfail" in data and data["xfail"] is None: + data.pop("xfail") + if "xfailed_jobs" in data and not data["xfailed_jobs"].get("buckets"): + data.pop("xfailed_jobs") + return data + + class SimFlowResults(BaseModel): """Flow results data.""" @@ -222,9 +267,17 @@ class SimFlowResults(BaseModel): extensions: tuple[CfgExtensionMeta, ...] = () """The cfg extensions (--cfg-extend) named for this flow.""" + xfailed: int = 0 + """Number of runs that failed, as expected (XFAIL). Not counted as passed.""" + xpassed: int = 0 + """Number of runs that passed, but were expected to fail (XPASS). Counted as failures.""" + + xfailed_jobs: BucketedFailures = BucketedFailures(buckets={}) + """Bucketed overview of the expected failures (XFAIL).""" + @model_serializer(mode="wrap") def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: - return _without_empty_extensions(handler(self)) + return _without_unused_xfail(_without_empty_extensions(handler(self))) def summary(self) -> "SimFlowSummary": """Load results from JSON file. @@ -269,9 +322,14 @@ class SimFlowSummary(BaseModel): extensions: tuple[CfgExtensionMeta, ...] = () """The cfg extensions (--cfg-extend) named for this flow.""" + xfailed: int = 0 + """Number of runs that failed, as expected (XFAIL). Not counted as passed.""" + xpassed: int = 0 + """Number of runs that passed, but were expected to fail (XPASS). Counted as failures.""" + @model_serializer(mode="wrap") def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: - return _without_empty_extensions(handler(self)) + return _without_unused_xfail(_without_empty_extensions(handler(self))) @staticmethod def load(path: Path) -> "SimFlowSummary": diff --git a/src/dvsim/sim/flow.py b/src/dvsim/sim/flow.py index 9a0fc75a..8f4c13d5 100644 --- a/src/dvsim/sim/flow.py +++ b/src/dvsim/sim/flow.py @@ -8,7 +8,7 @@ import random import sys from collections import OrderedDict, defaultdict -from collections.abc import Mapping, Sequence +from collections.abc import Iterable, Mapping, Sequence from datetime import datetime, timezone from importlib.metadata import PackageNotFoundError, version from pathlib import Path @@ -55,6 +55,15 @@ _MAX_TEST_RESEEDS = 2 +def _xfail_counts(results: Iterable[TestResult]) -> dict[str, int]: + """Sum the XFAIL and XPASS counts of some test results.""" + results = list(results) + return { + "xfailed": sum(r.xfailed for r in results), + "xpassed": sum(r.xpassed for r in results), + } + + class SimCfg(FlowCfg): """Simulation configuration object. @@ -779,6 +788,9 @@ def make_test_result(tr) -> TestResult | None: passed=tr.passing, total=tr.total, percent=100.0 * tr.passing / (tr.total or 1), + xfailed=tr.xfailed, + xpassed=tr.xpassed, + xfail=tr.xfail, ) # 1. Mapped testpoints — only include if at least one test ran @@ -804,6 +816,7 @@ def make_test_result(tr) -> TestResult | None: passed=tp_passed, total=tp_total, percent=100.0 * tp_passed / tp_total if tp_total else 0.0, + **_xfail_counts(test_results.values()), ) for name, tr in test_results.items(): @@ -824,6 +837,7 @@ def make_test_result(tr) -> TestResult | None: passed=tp_passed, total=tp_total, percent=100.0 * tp_passed / tp_total if tp_total else 0.0, + **_xfail_counts(unmapped_tests.values()), ) for name, tr in unmapped_tests.items(): @@ -842,6 +856,7 @@ def make_test_result(tr) -> TestResult | None: passed=stage_passed, total=stage_total, percent=100.0 * stage_passed / stage_total if stage_total else 0.0, + **_xfail_counts(stage_to_trs[stage_name].values()), ) total_passed = sum(tr.passed for tr in all_trs.values()) @@ -874,6 +889,7 @@ def make_test_result(tr) -> TestResult | None: vplan_coverage = self.cov_vplan_deploy.vplan_coverage failures = BucketedFailures.from_job_status(results=run_results) + expected_failures = BucketedFailures.from_job_status(results=run_results, expected=True) if failures.buckets: self.errors_seen = True @@ -896,6 +912,8 @@ def make_test_result(tr) -> TestResult | None: total=total_runs, percent=100.0 * total_passed / total_runs if total_runs else 0.0, extensions=tuple(extensions), + **_xfail_counts(all_trs.values()), + xfailed_jobs=expected_failures, ) def _fake_policy(self, job: JobSpec) -> JobStatus: diff --git a/src/dvsim/sim/report.py b/src/dvsim/sim/report.py index 7798ea6d..e94083c4 100644 --- a/src/dvsim/sim/report.py +++ b/src/dvsim/sim/report.py @@ -36,6 +36,14 @@ def _plural(item: str, n: int | Collection[Any], suffix: str = "s") -> str: return item if n == 1 else item + suffix +def _xfail_cell(xfailed: int, xpassed: int) -> str: + """Describe the XFAIL and XPASS runs of a test or group, for a table cell.""" + cell = str(xfailed) if xfailed else "" + if xpassed: + cell += f" ({xpassed} XPASS)" + return cell.strip() + + def _indent_by_levels(lines: Iterable[tuple[int, str]], indent_spaces: int = 4) -> str: """Format per-line indentation of (0-indexed level, msg) log messages.""" return "\n".join(" " * lvl * indent_spaces + msg for lvl, msg in lines) @@ -115,11 +123,17 @@ def render( sorted_buckets = dict( sorted(results.failed_jobs.buckets.items(), key=lambda kv: len(kv[1]), reverse=True) ) + sorted_xfail_buckets = dict( + sorted( + results.xfailed_jobs.buckets.items(), key=lambda kv: len(kv[1]), reverse=True + ) + ) artifacts[block_file] = render_template( path="reports/block_report.html", data={ "results": results, "failed_jobs": sorted_buckets, + "xfailed_jobs": sorted_xfail_buckets, "version": summary.version, }, ) @@ -290,6 +304,13 @@ def render_block_results(self, results: SimFlowResults) -> str: if bucket_summary: report_md += "\n\n" + bucket_summary + if results.xfailed_jobs.buckets: + bucket_summary = self.render_bucket_summary( + results.xfailed_jobs, title="Expected Failures", noun="expected failure" + ) + if bucket_summary: + report_md += "\n\n" + bucket_summary + return report_md def render_result_table(self, results: SimFlowResults) -> str: @@ -304,6 +325,14 @@ def render_result_table(self, results: SimFlowResults) -> str: ("Total", "center"), ("Pass Rate", "center"), ] + show_xfail = any( + t.xfail + for stage in results.stages.values() + for tp in stage.testpoints.values() + for t in tp.tests.values() + ) + if show_xfail: + column_info.append(("XFail", "center")) table = [] hidden_names = ("n.a.", "unmapped") @@ -328,19 +357,23 @@ def render_result_table(self, results: SimFlowResults) -> str: result.total, pass_rate, ] + if show_xfail: + row.append(_xfail_cell(result.xfailed, result.xpassed)) table.append(row) pass_rate = "-- %" if stage.total == 0 else f"{stage.percent:.2f} %" # TODO: note the calculated stage totals are currently not correct. - table.append( - [stage_name, None, "**TOTAL**", None, None, stage.passed, stage.total, pass_rate] - ) + row = [stage_name, None, "**TOTAL**", None, None, stage.passed, stage.total, pass_rate] + if show_xfail: + row.append(_xfail_cell(stage.xfailed, stage.xpassed)) + table.append(row) # TODO: note the calculated overall totals are currently not correct. pass_rate = "-- %" if results.total == 0 else f"{results.percent:.2f} %" - table.append( - [None, None, "**TOTAL**", None, None, results.passed, results.total, pass_rate] - ) + row = [None, None, "**TOTAL**", None, None, results.passed, results.total, pass_rate] + if show_xfail: + row.append(_xfail_cell(results.xfailed, results.xpassed)) + table.append(row) if not table: return "" @@ -464,6 +497,13 @@ def render_summary(self, summary: SimResultsSummary) -> ReportArtifacts: "Coverage": coverage, } ) + if flow_result.xfailed or flow_result.xpassed: + table[-1]["XFail"] = _xfail_cell(flow_result.xfailed, flow_result.xpassed) + + # Every row needs every column. + if any("XFail" in row for row in table): + for row in table: + row.setdefault("XFail", "") if table: colalign = ("center",) * len(table[0]) diff --git a/src/dvsim/sim_results.py b/src/dvsim/sim_results.py index c4d8e971..41526812 100644 --- a/src/dvsim/sim_results.py +++ b/src/dvsim/sim_results.py @@ -11,11 +11,12 @@ from pydantic import BaseModel, ConfigDict +from dvsim.job.data import is_xfail, is_xpass from dvsim.job.status import JobStatus from dvsim.testplan import Result if TYPE_CHECKING: - from dvsim.job.data import CompletedJobStatus + from dvsim.job.data import CompletedJobStatus, JobStatusInfo __all__ = () @@ -127,37 +128,53 @@ class BucketedFailures(BaseModel): """Mapping of common error message strings to the full job failure summary.""" @staticmethod - def from_job_status(results: Sequence["CompletedJobStatus"]) -> "BucketedFailures": - """Construct from CompletedJobStatus objects.""" + def from_job_status( + results: Sequence["CompletedJobStatus"], *, expected: bool = False + ) -> "BucketedFailures": + """Construct from CompletedJobStatus objects. + + By default, bucket the unexpected failures: failed or killed jobs that aren't + expected to fail, and runs of xfail tests that passed (XPASS). With expected=True, + bucket the expected failures (XFAIL) instead. + """ buckets = {} + def add( + bucket: str, job_status: "CompletedJobStatus", info: "JobStatusInfo | None" + ) -> None: + # TODO: expose all relevant line numbers through the bucket, not just the first. + # We only expose the first one for now to keep changes to the scheduler minimal. + first_line_num = None + if info is not None and info.lines: + lines = info.lines[0] + first_line_num = lines if isinstance(lines, int) else lines[0] + + buckets.setdefault(bucket, []).append( + JobFailureOverview( + name=job_status.name, + qual_name=job_status.qual_name, + seed=job_status.seed, + line=first_line_num, + log_path=job_status.log_path, + log_context=(info.context or []) if info is not None else [], + ), + ) + for job_status in results: - if job_status.status in (JobStatus.FAILED, JobStatus.KILLED): - if job_status.fail_msg is None: - continue - - bucket = _bucketize(job_status.fail_msg.message) - - if bucket not in buckets: - buckets[bucket] = [] - - # TODO: expose all relevant line numbers through the bucket, not just the first. - # We only expose the first one for now to keep changes to the scheduler minimal. - first_line_num = None - if job_status.fail_msg.lines: - lines = job_status.fail_msg.lines[0] - first_line_num = lines if isinstance(lines, int) else lines[0] - - buckets[bucket].append( - JobFailureOverview( - name=job_status.name, - qual_name=job_status.qual_name, - seed=job_status.seed, - line=first_line_num, - log_path=job_status.log_path, - log_context=job_status.fail_msg.context or [], - ), - ) + if is_xpass(job_status.xfail, job_status.status): + if not expected: + add(f"Passed, but expected to fail: {job_status.xfail}", job_status, None) + continue + + if job_status.status not in (JobStatus.FAILED, JobStatus.KILLED): + continue + if ( + job_status.fail_msg is None + or is_xfail(job_status.xfail, job_status.status, job_status.fail_msg) != expected + ): + continue + + add(_bucketize(job_status.fail_msg.message), job_status, job_status.fail_msg) return BucketedFailures( buckets=buckets, @@ -206,6 +223,13 @@ def _add_run(self, job_status: "CompletedJobStatus") -> None: row.job_runtime = job_status.job_runtime row.simulated_time = job_status.simulated_time - if job_status.status == JobStatus.PASSED: + if job_status.xfail is not None: + row.xfail = job_status.xfail + if is_xpass(job_status.xfail, job_status.status): + row.xpassed += 1 + elif is_xfail(job_status.xfail, job_status.status, job_status.fail_msg): + # An expected failure counts in total, but not in passing. + row.xfailed += 1 + elif job_status.status == JobStatus.PASSED: row.passing += 1 row.total += 1 diff --git a/src/dvsim/templates/reports/block_report.html b/src/dvsim/templates/reports/block_report.html index 74087e20..1a984bb6 100644 --- a/src/dvsim/templates/reports/block_report.html +++ b/src/dvsim/templates/reports/block_report.html @@ -193,6 +193,8 @@

Simulation Results: {{ block.variant_name(sep='/') }}

{{ s_name }}
{{ stage.passed }}/{{ stage.total }} passed + {%- if stage.xfailed %}, {{ stage.xfailed }} xfail{% endif %} + {%- if stage.xpassed %}, {{ stage.xpassed }} xpass{% endif %}
{{ "%.2f %%" | format(stage.percent) if stage.total > 0 else "" }} @@ -225,7 +227,14 @@

Simulation Results: {{ block.variant_name(sep='/') }}

{% for t_name, t in tp.tests.items() %} - {{ t_name }} + + {{ t_name }} + {% if t.xfail %} + + xfail + + {% endif %} + {{ "%.3fs" | format(t.max_time) if t.max_time is not none else "" }} {{ "%.3fus" | format(t.sim_time) if t.sim_time is not none else "" }} {{ t.passed }} @@ -254,5 +263,9 @@

Simulation Results: {{ block.variant_name(sep='/') }}

{% if failed_jobs %} {{ bucket_table("Error Messages", failed_jobs) }} {% endif %} + + {% if xfailed_jobs %} + {{ bucket_table("Expected Failures", xfailed_jobs) }} + {% endif %}
{% endblock %} diff --git a/src/dvsim/templates/reports/summary_report.html b/src/dvsim/templates/reports/summary_report.html index 77fbdc94..ccdd0d63 100644 --- a/src/dvsim/templates/reports/summary_report.html +++ b/src/dvsim/templates/reports/summary_report.html @@ -125,7 +125,12 @@

{{ title }}

- {{ flow.passed }} + + {{ flow.passed }} + {% if flow.xfailed %} + {{ flow.xfailed }} xfail + {% endif %} + {{ flow.total }} {{ "%.2f" | format(flow.percent) }} diff --git a/src/dvsim/test.py b/src/dvsim/test.py index 6835810d..0bf4978c 100644 --- a/src/dvsim/test.py +++ b/src/dvsim/test.py @@ -30,6 +30,20 @@ class Test(RunMode): "run_timeout_multiplier": None, } + def __init__(self, tdict) -> None: + # Why the test is expected to fail, or "" if it is expected to pass. + self.xfail = "" + + super().__init__(tdict) + + if not isinstance(self.xfail, str): + log.error( + 'Test "%s": xfail must be a string giving the reason, not %r.', + self.name, + self.xfail, + ) + sys.exit(1) + @staticmethod def create_tests(tdicts, sim_cfg): """Create Test objects from a given list of raw dicts. diff --git a/src/dvsim/testplan.py b/src/dvsim/testplan.py index 7f5ff904..60b001b6 100644 --- a/src/dvsim/testplan.py +++ b/src/dvsim/testplan.py @@ -48,6 +48,12 @@ def __init__( self.simulated_time = simulated_time_us self.mapped = False + # For a test that is expected to fail: the reason, and how many of its runs failed + # (XFAIL) or passed (XPASS). Both kinds count in total, but not in passing. + self.xfail: str | None = None + self.xfailed = 0 + self.xpassed = 0 + class Element: """An element of the testplan. diff --git a/tests/sim/__init__.py b/tests/sim/__init__.py new file mode 100644 index 00000000..4b480910 --- /dev/null +++ b/tests/sim/__init__.py @@ -0,0 +1,5 @@ +# Copyright lowRISC contributors (OpenTitan project). +# Licensed under the Apache License, Version 2.0, see LICENSE for details. +# SPDX-License-Identifier: Apache-2.0 + +"""Test sim flow.""" diff --git a/tests/sim/test_xfail.py b/tests/sim/test_xfail.py new file mode 100644 index 00000000..b3b7639a --- /dev/null +++ b/tests/sim/test_xfail.py @@ -0,0 +1,373 @@ +# 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 tests that are expected to fail (the xfail key).""" + +from datetime import datetime, timezone +from pathlib import Path + +import pytest + +from dvsim.job.data import ( + CompletedJobStatus, + JobStatusInfo, + WorkspaceConfig, + is_xfail, + is_xpass, +) +from dvsim.job.status import JobStatus +from dvsim.report.data import IPMeta, ToolMeta +from dvsim.scheduler.core import FAILED_DEP, KILLED_RUNNING_SIGINT +from dvsim.sim import data, flow +from dvsim.sim.flow import SimCfg +from dvsim.sim.report import HtmlReportRenderer, MarkdownReportRenderer +from dvsim.sim_results import BucketedFailures, SimResults +from dvsim.test import Test as DvsimTest +from dvsim.testplan import Result +from dvsim.testplan import Testplan as DvsimTestplan + +__all__ = () + +REASON = "needs feature X (issue #1)" +BLOCK = IPMeta( + name="blk", commit="c0ffee", commit_short="c0f", branch="main", url=None, revision_info=None +) + + +def _status( + name: str, + status: JobStatus, + *, + xfail: str | None = None, + message: JobStatusInfo | str | None = "Error: it broke", + seed: int = 1, +) -> CompletedJobStatus: + if isinstance(message, str): + message = JobStatusInfo(message=message, lines=[3], context=["Error: it broke"]) + return CompletedJobStatus( + name=name, + job_type="RunTest", + seed=seed, + block=BLOCK, + tool=ToolMeta(name="vcs", version=""), + workspace_cfg=WorkspaceConfig( + timestamp="t", + project_root=Path("/proj"), + scratch_root=Path("/scratch"), + scratch_path=Path("/scratch/blk"), + ), + full_name=f"blk:{seed}.{name}", + qual_name=f"{seed}.{name}", + target="run", + log_path=Path(f"/scratch/blk/{name}.log"), + job_runtime=1.0, + simulated_time=2.0, + status=status, + fail_msg=None if status == JobStatus.PASSED else message, + xfail=xfail, + ) + + +class TestXfailKey: + """The xfail key on a test.""" + + def test_default(self) -> None: + assert DvsimTest({"name": "t"}).xfail == "" + + def test_reason(self) -> None: + assert DvsimTest({"name": "t", "xfail": REASON}).xfail == REASON + + def test_not_a_string(self) -> None: + with pytest.raises(SystemExit): + DvsimTest({"name": "t", "xfail": True}) + + def test_merge_two_files(self) -> None: + """A test defined in two files gets the xfail of the one that sets it.""" + test = DvsimTest({"name": "t", "run_opts": ["+a"]}) + test.merge_mode(DvsimTest({"name": "t", "xfail": REASON})) + assert test.xfail == REASON + + +class TestClassify: + """Which runs count as XFAIL and XPASS.""" + + @pytest.mark.parametrize( + ("status", "xfail", "message", "expected"), + [ + (JobStatus.FAILED, REASON, "Error: it broke", (True, False)), + # A killed run is a failure, not the expected one: a timeout (a hang may hide the + # known failure) or a job that couldn't be launched. + (JobStatus.KILLED, REASON, "Job timed out after 1 minutes", (False, False)), + (JobStatus.KILLED, REASON, "Failed to launch job: no such file", (False, False)), + (JobStatus.KILLED, REASON, None, (False, False)), + # A run that never ran, or was stopped by a signal, hasn't failed. + (JobStatus.KILLED, REASON, FAILED_DEP, (False, False)), + (JobStatus.FAILED, REASON, FAILED_DEP, (False, False)), + (JobStatus.KILLED, REASON, KILLED_RUNNING_SIGINT, (False, False)), + (JobStatus.PASSED, REASON, None, (False, True)), + (JobStatus.FAILED, None, "Error: it broke", (False, False)), + (JobStatus.PASSED, None, None, (False, False)), + ], + ) + def test_classify(self, status, xfail, message, expected) -> None: + job = _status("t", status, xfail=xfail, message=message) + assert ( + is_xfail(job.xfail, job.status, job.fail_msg), + is_xpass(job.xfail, job.status), + ) == expected + + +class TestResults: + """How XFAIL and XPASS runs are counted and bucketed.""" + + @pytest.fixture + def results(self) -> list[CompletedJobStatus]: + return [ + _status("ok", JobStatus.PASSED), + _status("bad", JobStatus.FAILED), + _status("xf", JobStatus.FAILED, xfail=REASON, seed=1), + _status("xf", JobStatus.PASSED, xfail=REASON, seed=2), + _status("xf", JobStatus.KILLED, xfail=REASON, message=FAILED_DEP, seed=3), + ] + + def test_counts(self, results) -> None: + rows = {row.name: row for row in SimResults(results).table} + assert (rows["ok"].passing, rows["ok"].total, rows["ok"].xfail) == (1, 1, None) + assert (rows["bad"].passing, rows["bad"].total) == (0, 1) + xf = rows["xf"] + # Every run counts in total, and none of them in passing. + assert (xf.passing, xf.xfailed, xf.xpassed, xf.total, xf.xfail) == (0, 1, 1, 3, REASON) + + def test_only_xfail(self) -> None: + """A test whose runs are all XFAIL ran, and has a pass rate of 0.""" + row = SimResults([_status("xf", JobStatus.FAILED, xfail=REASON)]).table[0] + assert (row.passing, row.total, row.xfailed) == (0, 1, 1) + + def test_unexpected_buckets(self, results) -> None: + """Real failures, cancelled runs and XPASS are failures; XFAIL isn't.""" + buckets = BucketedFailures.from_job_status(results).buckets + names = {b: [j.qual_name for j in jobs] for b, jobs in buckets.items()} + assert names == { + "Error: it broke": ["1.bad"], + f"Passed, but expected to fail: {REASON}": ["2.xf"], + FAILED_DEP.message: ["3.xf"], + } + + def test_expected_buckets(self, results) -> None: + buckets = BucketedFailures.from_job_status(results, expected=True).buckets + assert {b: [j.qual_name for j in jobs] for b, jobs in buckets.items()} == { + "Error: it broke": ["1.xf"], + } + assert buckets["Error: it broke"][0].line == 3 + + +class TestTestplanProgress: + """A test whose runs are all XFAIL is written, but not passing.""" + + def test_progress(self, tmp_path: Path) -> None: + plan = tmp_path / "plan.hjson" + plan.write_text( + '{name: "blk", testpoints: [{name: "tp", desc: "d", stage: "V1", ' + 'tests: ["ok", "xf", "missing"]}]}' + ) + testplan = DvsimTestplan(str(plan), repo_top=tmp_path, name="blk") + xf = Result("xf", passing=0, total=2) + xf.xfail, xf.xfailed = REASON, 2 + testplan.map_test_results([Result("ok", passing=1, total=1), xf]) + progress = testplan.progress["V1"] + assert (progress["total"], progress["written"], progress["passing"]) == (3, 2, 1) + + +class TestExitCode: + """XFAIL runs don't make the run fail; XPASS runs do.""" + + @staticmethod + def _has_errors( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, results: list[CompletedJobStatus] + ) -> bool: + """Run the real _gen_json_results on a cfg with just the attributes it reads.""" + monkeypatch.setattr(flow, "query_tool_version", lambda _tool: "1") + plan = tmp_path / "plan.hjson" + plan.write_text( + '{name: "blk", testpoints: [{name: "tp", desc: "d", stage: "V1", tests: ["ok", "xf"]}]}' + ) + cfg = object.__new__(SimCfg) + cfg.__dict__.update( + name="blk", + variant=None, + commit="c0ffee", + commit_short="c0f", + dirty=False, + branch="main", + revision=None, + tool="vcs", + timestamp="20261025_000000", + run_only=False, + build_seed=None, + testplan=DvsimTestplan(str(plan), repo_top=tmp_path, name="blk"), + testplan_doc_path=None, + rel_path="hw/blk/dv/blk_sim_cfg.hjson", + book=None, + proj_root=str(tmp_path), + map_full_testplan=False, + cov_report_deploy=None, + cov_report_page=None, + scratch_path=str(tmp_path), + cfg_extensions={}, + errors_seen=False, + ) + cfg._gen_json_results(results, url="") + return cfg.has_errors() + + def test_xfail_passes(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + results = [ + _status("ok", JobStatus.PASSED), + _status("xf", JobStatus.FAILED, xfail=REASON), + ] + assert not self._has_errors(tmp_path, monkeypatch, results) + + def test_xpass_fails(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + results = [_status("xf", JobStatus.PASSED, xfail=REASON)] + assert self._has_errors(tmp_path, monkeypatch, results) + + +def _flow_results(results: list[CompletedJobStatus]) -> data.SimFlowResults: + """Build flow results with one unmapped testpoint, as _gen_json_results does.""" + tests = { + row.name: data.TestResult( + max_time=row.job_runtime, + sim_time=row.simulated_time, + passed=row.passing, + total=row.total, + percent=100.0 * row.passing / (row.total or 1), + xfailed=row.xfailed, + xpassed=row.xpassed, + xfail=row.xfail, + ) + for row in SimResults(results).table + } + counts = { + "passed": sum(t.passed for t in tests.values()), + "total": sum(t.total for t in tests.values()), + "percent": 50.0, + "xfailed": sum(t.xfailed for t in tests.values()), + "xpassed": sum(t.xpassed for t in tests.values()), + } + stage = data.TestStage(testpoints={"Unmapped": data.Testpoint(tests=tests, **counts)}, **counts) + return data.SimFlowResults( + block=BLOCK, + tool=ToolMeta(name="vcs", version="1"), + timestamp=datetime(2026, 10, 25, tzinfo=timezone.utc), + build_seed=None, + testplan_ref=None, + stages={"unmapped": stage}, + coverage=None, + cov_report_page=None, + vplan_report_page=None, + failed_jobs=BucketedFailures.from_job_status(results), + xfailed_jobs=BucketedFailures.from_job_status(results, expected=True), + **counts, + ) + + +class TestReports: + """XFAIL and XPASS in the reports.""" + + def test_markdown_block(self) -> None: + flow = _flow_results( + [ + _status("ok", JobStatus.PASSED), + _status("xf", JobStatus.FAILED, xfail=REASON, seed=1), + _status("xp", JobStatus.PASSED, xfail=REASON, seed=2), + ] + ) + md = MarkdownReportRenderer(None).render_block(flow)["report.md"] + assert "XFail" in md + assert "1 (1 XPASS)" in md + assert "## Failure Buckets" in md + assert f"Passed, but expected to fail: {REASON}" in md + assert "## Expected Failures" in md + assert "has 1 expected failure:" in md + + def test_markdown_no_xfail(self) -> None: + """Flows without xfail tests look as before.""" + flow = _flow_results([_status("ok", JobStatus.PASSED), _status("bad", JobStatus.FAILED)]) + md = MarkdownReportRenderer(None).render_block(flow)["report.md"] + assert "XFail" not in md + assert "Expected Failures" not in md + + def test_markdown_only_xfail(self) -> None: + """A flow whose runs are all XFAIL shows its table, with a pass rate of 0.""" + flow = _flow_results([_status("xf", JobStatus.FAILED, xfail=REASON)]) + md = MarkdownReportRenderer(None).render_block(flow)["report.md"] + assert "No results to display." not in md + assert "0.00 %" in md + assert "## Expected Failures" in md + + def test_markdown_summary(self) -> None: + flows = { + "blk": _flow_results([_status("xf", JobStatus.FAILED, xfail=REASON)]), + "other": _flow_results([_status("ok", JobStatus.PASSED)]), + } + summary = data.SimResultsSummary( + top=None, + version=None, + timestamp=datetime(2026, 10, 25, tzinfo=timezone.utc), + build_seed=None, + flow_results={k: v.summary() for k, v in flows.items()}, + report_path=Path("/scratch/reports"), + ) + md = MarkdownReportRenderer(None).render_summary(summary)["report.md"] + header = md.strip().splitlines()[0] + assert header.rstrip("| ").endswith("XFail") + + def test_html_block(self) -> None: + flow = _flow_results( + [_status("ok", JobStatus.PASSED), _status("xf", JobStatus.FAILED, xfail=REASON)] + ) + summary = data.SimResultsSummary( + top=None, + version=None, + timestamp=datetime(2026, 10, 25, tzinfo=timezone.utc), + build_seed=None, + flow_results={"blk": flow.summary()}, + report_path=Path("/scratch/reports"), + ) + html = HtmlReportRenderer().render(summary, {"blk": flow}) + block = html["blk.html"] + assert "Expected Failures" in block + assert f'title="{REASON}"' in block + assert ", 1 xfail" in block + assert "1 xfail" in html["index.html"] + + +def _keys(obj: object) -> set[str]: + """Every key in a JSON value, at any depth.""" + if isinstance(obj, dict): + return set(obj) | {k for v in obj.values() for k in _keys(v)} + if isinstance(obj, list): + return {k for v in obj for k in _keys(v)} + return set() + + +class TestResultsJson: + """The xfail keys are in the JSON only when a flow has xfail tests.""" + + XFAIL_KEYS = frozenset({"xfailed", "xpassed", "xfail", "xfailed_jobs"}) + + def test_without_xfail(self) -> None: + flow = _flow_results([_status("ok", JobStatus.PASSED), _status("bad", JobStatus.FAILED)]) + dumped = flow.model_dump(mode="json") + assert not _keys(dumped) & self.XFAIL_KEYS + assert not _keys(flow.summary().model_dump(mode="json")) & self.XFAIL_KEYS + assert data.SimFlowResults.model_validate(dumped) == flow + + def test_with_xfail(self) -> None: + flow = _flow_results( + [_status("ok", JobStatus.PASSED), _status("xf", JobStatus.FAILED, xfail=REASON)] + ) + dumped = flow.model_dump(mode="json") + assert _keys(dumped) >= {"xfailed", "xfail", "xfailed_jobs"} + assert dumped["xfailed"] == 1 + assert data.SimFlowResults.model_validate(dumped) == flow