Skip to content

doc: Add documentation to the Result class in testplan.py#214

Open
rswarbrick wants to merge 1 commit into
lowRISC:masterfrom
rswarbrick:result-class-docs
Open

doc: Add documentation to the Result class in testplan.py#214
rswarbrick wants to merge 1 commit into
lowRISC:masterfrom
rswarbrick:result-class-docs

Conversation

@rswarbrick
Copy link
Copy Markdown
Contributor

In particular, this gives types to all the arguments and gives units for the numbers (I had to run some tests to check what they were!)

@rswarbrick rswarbrick requested a review from AlexJones0 May 21, 2026 12:42
In particular, this gives types to all the arguments and gives units
for the numbers (I had to run some tests to check what they were!)

Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
@rswarbrick rswarbrick force-pushed the result-class-docs branch from 46aff46 to 331fdc0 Compare May 21, 2026 12:45
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

Thanks @rswarbrick!

Comment thread src/dvsim/testplan.py
Comment on lines +21 to +28
def __init__(
self,
name: str,
passing: int = 0,
total: int = 0,
job_runtime: float | None = None,
simulated_time: float | None = None,
) -> None:
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.

One suggestion I might have would be to make most of the arguments here keyword-only:

Suggested change
def __init__(
self,
name: str,
passing: int = 0,
total: int = 0,
job_runtime: float | None = None,
simulated_time: float | None = None,
) -> None:
def __init__(
self,
name: str,
*
passing: int = 0,
total: int = 0,
job_runtime: float | None = None,
simulated_time: float | None = None,
) -> None:

This is because currently you can make a Result like:

Result("example test", 3, 5, 19.20, 13.41)

which isn't particularly descriptive to read, so you need to search through the definition. This change would instead mean you need to write:

Result("example test", passing=3, total=5, job_runtime=19.20, simulated_time=13.41)

I'll leave it up to you whether you prefer that or not though.

Comment thread src/dvsim/testplan.py

job_runtime: If not None, the number of seconds taken by the job.

simulated_time: If not None, the simulated time in microseconds.
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.

It doesn't need to be changed in this PR (as I appreciate it would be a widespread change across the code base) - but IMO these parameters should be suffixed by their units (e.g. job_runtime_s and simulated_time_us) to avoid the confusion entirely.

Comment thread src/dvsim/testplan.py
Comment on lines +36 to +37
total: The number of runs that happened (will be at least as large as
passing).
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.

Another nice idea might be to add setters for passing and total to this class to enforce this invariant and error otherwise - but I appreciate that's probably outside of the scope of the improvements in this PR, which is just doing some cleanup.

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