Skip to content

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

Open
rswarbrick wants to merge 1 commit into
lowRISC:masterfrom
rswarbrick:repo-top
Open

refactor: Remove outdated (and unused) default repo_top argument#218
rswarbrick wants to merge 1 commit into
lowRISC:masterfrom
rswarbrick:repo-top

Conversation

@rswarbrick
Copy link
Copy Markdown
Contributor

This code was based on dvsim living inside the OpenTitan repository. Fortunately, it is never used: we always pass a third argument to _parse_testplan.

Also, fail more understandably if there is an hjson parse error or an invalid filename passed to _parse_hjson.

Finally, simplify the Testplan constructor so that it always expects a name for its testplan (rather than exiting the program silently if there isn't one).

We also change that argument to explicitly have type str (as opposed to Path) and explain why: the string can have suffix values that filter the set of tests.

This code was based on dvsim living inside the OpenTitan repository.
Fortunately, it is never used: we always pass a third argument to
_parse_testplan.

Also, fail more understandably if there is an hjson parse error or an
invalid filename passed to _parse_hjson.

Finally, simplify the Testplan constructor so that it always expects a
name for its testplan (rather than exiting the program silently if
there isn't one).

We also change that argument to explicitly have type str (as opposed
to Path) and explain why: the string can have suffix values that
filter the set of tests.

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
@rswarbrick rswarbrick requested a review from AlexJones0 May 21, 2026 12:47
Comment thread src/dvsim/testplan.py
Comment on lines +247 to +248
"""Parse an hjson file at the given path and return it as a dict."""
return hjson.load(Path(filename).open())
Copy link
Copy Markdown
Contributor

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).

Comment thread src/dvsim/testplan.py
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

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).

Comment thread src/dvsim/testplan.py
Comment on lines +296 to +298
is a string, rather than a Path object, because it may be
suffixed with tags separated with a colon delimiter to
filter the testpoints.
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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants