-
Notifications
You must be signed in to change notification settings - Fork 167
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Added TESTING.md to describe the testing methodology used for the RIS…
…CV Sail model. Minor code cleanup.
- Loading branch information
1 parent
687a9d5
commit 1666e75
Showing
3 changed files
with
76 additions
and
9 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
# Testing the RISCV-Sail model | ||
|
||
This document contains information regarding the testing of the RISC-V | ||
Sail model. | ||
|
||
## Background | ||
|
||
For the longest time, the set of tests used to validate changes to the | ||
model, were a set of precompiled .elf files (along with their .dump file counterparts) | ||
that were stored in the repo under `test/riscv-tests`. The scripts used to | ||
run theses tests (and to gather test results) were `test/run_tests.sh` and | ||
`test/run_fp_tests.sh`. | ||
|
||
These tests are a compiled snapshot of tests that can be found at | ||
https://github.com/riscv-software-src/riscv-tests | ||
that date back to 2019. | ||
|
||
This methodolgy was defecient in several ways. | ||
1. Original test source is difficult to track down. | ||
1. Storing compiled code in a git repo is usually frowned upon. | ||
1. There is no easy way to add new tests to the repo when you add a new feature. | ||
1. `run_tests.sh` is difficult to enhance with new features. | ||
|
||
We anticipate that the `test/` directory will be removed once a more robust | ||
testing methodolgy is put in place. (See next section.) | ||
|
||
## Adding new tests | ||
|
||
|
||
|
||
To fix the defeciencies of the old test methodology, we have done the | ||
following: | ||
1. Created a new test directory at the repo root, `TEST_DIR_ROOT/` under which | ||
all new test collateral will be put. | ||
1. Created a `bin/` directory under which various model scripts and executables | ||
are added. | ||
1. Installed https://github.com/riscv-software-src/riscv-tests as a submodule | ||
at `TEST_DIR_ROOT/riscv-tests.git/`. We will be working on a special branch | ||
in this repository: `riscv-tests-sail`. This allows us to add tests onto our | ||
branch. And we can incorporate new tests into our testsuite as they appear | ||
in the riscv-tests repo (by merging these new tests from the master branch | ||
onto our branch). | ||
1. Re-wrote `tests/run_tests.sh` in python and added run-time switches, the main | ||
purpose of which was to be able to add command line switches to the execution of | ||
particular tests. See the script, `bin/run_tests.py`, for execution parameters. | ||
1. Updated `.github/workflows/compile.yml` to make use of the new run_tests python | ||
script. | ||
|
||
|
||
|
||
## Future Plans | ||
|
||
### Sail and Spike Crosschecking with the Architecture Compatability Tests (ACTS) | ||
|
||
We intend to run the ACTs on both Sail and Spike. Test signatures will be compared | ||
to check that the two simulators agree. | ||
|
||
### Fixing defecincies in the test environment | ||
|
||
We have the following defeciencies in the test environment that need to be fixed: | ||
|
||
1. A pass/fail can only be detected and reported from within the test itself. | ||
If a test writer wanted to check the simluator log file to see | ||
if certain strings existed (say, for example, you want to check the disassembly | ||
of newly added instructions), there is no method to do so. The ability to | ||
inspect the log file is a neccessary feature that needs to be added. | ||
|
||
1. Negative testing. We need to be able to check for proper detection of errors | ||
which would then mean that the test "passed". For example, we might want to check | ||
that if the vector extension is not enabled, that a test that uses a vector instruction | ||
would "fail". | ||
|
||
|
||
|
||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1666e75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@billmcspadden-riscv Perhaps it would be useful at the start of this to be explicit about the goal of that testing, given the nuanced status of the model as a (still somewhat notionally) authoritative reference. AIUI, this is essentially sanity checking that can happen in CI for changes to the model, complementing ACT testing, and can't replace the tests that extension authors are supposed to provide?
1666e75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like it will be a huge pain. From the
sail-riscv
PR author's point of view, to add a test to their PR they have to now make a PR in theriscv-tests
repo first, get it merged, and then make theirsail-riscv
PR. I think it would be better if we just had some tests that were local to this repo (not in a submodule) and then we can contribute them back toriscv-tests
later on theirmaster
branch. Then when we update the submodule pointer we could simultaneously remove them fromsail-riscv
.Alternatively if we use a
git subtree
it avoids the need for a branch and submodule altogether, which is nice.I would be very wary of this. It will lead to people adding printfs to their tests and checking that output, rather than failing the test properly via HTIF. Checking disassembly can be done via Sail unit tests (we have added a few of those locally; I'll make a PR at some point).
Maybe there are use cases I haven't thought of, but in general parsing logs is janky and error prone and best avoided.
1666e75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1666e75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Tests that only work on the Sail model"?..
1666e75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1666e75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting them on a branch isn't the right solution to that problem. XFAIL'ing/skipping them somehow for models that don't support those tests is the industry-standard way that's actually helpful, rather than scattering them elsewhere for clueless users to fail to find or screw up the branch juggling.
1666e75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that you can easily create a local branch in the submodule, but you can't easily push it to
riscv-tests
, because that requires creating a PR and getting it reviewed and merged. That's extra tedious, and also we would ideally want to review the test and the Sail code together in one PR, not sequentially in two PRs in different repos.Fair enough, I would say testing the CLI is a legit use for parsing logs.
Yeah I have to agree with this. It's
riscv-tests
notspike-tests
. If it's a legit test that should work on Spike (or any other device) then it should be inmaster
and Spike can waive the failure.