Skip to content

refactor: Remove outdated (and unused) default repo_top argument #218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rswarbrick wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor: Remove outdated (and unused) default repo_top argument #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/dvsim/sim/flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,9 @@ def _create_objects(self) -> None:
self.regressions.extend(self.testplan.get_stage_regressions())
else:
# Create a dummy testplan with no entries.
self.testplan = Testplan(None, name=self.name)
self.testplan = Testplan(
"<dummy testplan>", repo_top=Path(self.proj_root), name=self.name
)

# Create regressions
self.regressions = Regression.create_regressions(self.regressions, self, self.tests)
Expand Down
86 changes: 24 additions & 62 deletions src/dvsim/testplan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

"""Testpoint and Testplan classes for maintaining the testplan."""

import os
import re
import sys
from collections import defaultdict
Expand Down Expand Up @@ -244,15 +243,9 @@ class Testplan:
element_cls = {"testpoint": Testpoint, "covergroup": Covergroup}

@staticmethod
def _parse_hjson(filename):
"""Parses an input file with HJson and returns a dict."""
try:
return hjson.load(Path(filename).open())
except OSError:
pass
except hjson.scanner.HjsonDecodeError:
pass
sys.exit(1)
def _parse_hjson(filename: Path) -> dict:
"""Parse an hjson file at the given path and return it as a dict."""
return hjson.load(Path(filename).open())
Comment on lines +247 to +248
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a problem in the original code, but we should probably fix it while we're here.

Reference counting probably means that this issue will not be a problem, but we should be explicitly closing the file handle after opening it rather than leaving it open.

A better approach would be:

Suggested change
"""Parse an hjson file at the given path and return it as a dict."""
return hjson.load(Path(filename).open())
"""Parse an hjson file at the given path and return it as a dict."""
with Path(filename).open("r", encoding="utf-8") as f:
return hjson.load(f)

Or maybe even better as:

Suggested change
"""Parse an hjson file at the given path and return it as a dict."""
return hjson.load(Path(filename).open())
"""Parse an hjson file at the given path and return it as a dict."""
return hjson.loads(Path(filename).read_text(encoding="utf-8"))

(to let Pathlib handle the read file mode for us).


@staticmethod
def _create_testplan_elements(kind: str, raw_dicts_list: list, tags: set) -> list[Element]:
Expand Down Expand Up @@ -295,64 +288,37 @@ def _get_percentage(value, total) -> str:
perc = value / total * 100 * 1.0
return f"{round(perc, 2):.2f} %"

@staticmethod
def get_dv_style_css() -> str:
"""Returns text with HTML CSS style for a table."""
return (
"<style>\n"
"table.dv {\n"
" border: 1px solid black;\n"
" border-collapse: collapse;\n"
" width: 100%;\n"
" text-align: left;\n"
" vertical-align: middle;\n"
" display: table;\n"
" font-size: smaller;\n"
"}\n"
"table.dv th, td {\n"
" border: 1px solid black;\n"
"}\n"
"</style>\n"
)

def __str__(self) -> str:
lines = [f"Name: {self.name}\n"]
lines += ["Testpoints:"]
lines += [f"{t}" for t in self.testpoints]
lines += ["Covergroups:"]
lines += [f"{c}" for c in self.covergroups]
return "\n".join(lines)

def __init__(self, filename, repo_top=None, name=None) -> None:
def __init__(self, filename: str, repo_top: Path, name: str) -> None:
"""Initialize the testplan.

filename is the HJson file that captures the testplan. It may be
suffixed with tags separated with a colon delimiter to filter the
testpoints. For example: path/too/foo_testplan.hjson:bar:baz
repo_top is an optional argument indicating the path to top level repo
/ project directory. It is used with filename arg.
name is an optional argument indicating the name of the testplan / DUT.
It overrides the name set in the testplan HJson.
Args:
filename: Describes the HJson file that captures the testplan. This
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
filename: Describes the HJson file that captures the testplan. This
filename: Describes the Hjson file that captures the testplan. This

(a couple more times in these changed docs - I appreciate it was in the original but it'd be nice to be consistent).

is a string, rather than a Path object, because it may be
suffixed with tags separated with a colon delimiter to
filter the testpoints.
Comment on lines +296 to +298
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanation is great, but given that we later do

        filename = Path(split[0])
        tags = set(split[1:])

I'd suggest the name isn't great anyway and we should rename it. Perhaps something like:

  • tagged_filename
  • filename_with_tags
  • file_spec / testplan_file_spec

etc. etc. - take your pick.


For example: path/too/foo_testplan.hjson:bar:baz

repo_top: The path to the top level repo / project directory. This is
combined with the filename argument.

name: The name of the testplan / DUT. It overrides any name set
in the testplan HJson.

"""
self.name = None
self.name = name
self.testpoints = []
self.covergroups = []
self.test_results_mapped = False

# Split the filename into filename and tags, if provided.
split = str(filename).split(":")
split = filename.split(":")
filename = Path(split[0])
tags = set(split[1:])

if filename.exists():
self._parse_testplan(filename, tags, repo_top)

if name:
self.name = name

if not self.name:
sys.exit(1)

# Represents current progress towards each stage. Stage = N.A.
# is used to indicate the unmapped tests.
self.progress = {}
Expand Down Expand Up @@ -414,7 +380,7 @@ def _get_imported_testplan_paths(

return result

def _parse_testplan(self, filename: Path, tags: set, repo_top=None) -> None:
def _parse_testplan(self, filename: Path, tags: set[str], repo_top: Path) -> None:
"""Parse testplan Hjson file and create the testplan elements.

It creates the list of testpoints and covergroups extracted from the
Expand All @@ -423,10 +389,6 @@ def _parse_testplan(self, filename: Path, tags: set, repo_top=None) -> None:
filename is the path to the testplan file written in HJson format.
repo_top is an optional argument indicating the path to repo top.
"""
if repo_top is None:
# Assume dvsim's original location: $REPO_TOP/util/dvsim.
repo_top = Path(__file__).parent.parent.parent.resolve()

obj = Testplan._parse_hjson(filename)

parsed = set()
Expand All @@ -442,7 +404,7 @@ def _parse_testplan(self, filename: Path, tags: set, repo_top=None) -> None:
if testplan in parsed:
sys.exit(1)
parsed.add(testplan)
data = self._parse_hjson(os.path.join(repo_top, testplan))
data = self._parse_hjson(repo_top / testplan)
imported_testplans.extend(
self._get_imported_testplan_paths(
testplan,
Expand Down Expand Up @@ -737,7 +699,7 @@ def get_test_results_summary(self):
result["Pass Rate"] = self._get_percentage(tr.passing, tr.total)
return result

def get_sim_results(self, sim_results_file, fmt="md"):
def get_sim_results(self, sim_results_file: str, fmt="md"):
"""Returns the mapped sim result tables in HTML formatted text.

The data extracted from the sim_results table HJson file is mapped into
Expand All @@ -746,7 +708,7 @@ def get_sim_results(self, sim_results_file, fmt="md"):
fmt is either 'md' (markdown) or 'html'.
"""
assert fmt in ["md", "html"]
sim_results = Testplan._parse_hjson(sim_results_file)
sim_results = Testplan._parse_hjson(Path(sim_results_file))
test_results_ = sim_results.get("test_results", None)

test_results = []
Expand Down
Loading