ChiselSim API for inline testing #4946
Conversation
| abstract class TestHarness[M <: RawModule, R](test: TestParameters[M, R]) | ||
| extends FixedIOModule(new TestResultBundle) | ||
| abstract class TestHarness[M <: RawModule](test: TestParameters[M]) | ||
| extends FixedIOModule(new TestHarnessInterface) |
There was a problem hiding this comment.
I removed the R type parameter (the thing returned from a test-body and consumed by the testharness).
Instead of leaving this API wide open and expecting users to implement their own test-testharness interfaces, I added a new TestConfiguration case class.
The idea is that as the test body elaborates it builds a TestConfiguration, which is more or less "how the testharness should handle this test."
Based on what that is (e.g. "run for 10 cycles", "end when this signal is asserted"), the testharness knows how to wiggle the finish/success pins.
In the future, this might also include things like "expect some assertion to fire" if we want to support tests that expect to fail.
| intf.success := successCondition.getOrElse(true.B) | ||
| failureMessage.foreach { failureMessage => | ||
| when(intf.finish && intf.success) { | ||
| printf(cf"${testName} failed: ${failureMessage}") |
There was a problem hiding this comment.
this says failed, but it prints when intf.success is true... am i missing something?
There was a problem hiding this comment.
No not missing anything. Also, I forgot I had this feature here--need to add a test for it.
| class InlineTestSignaledFailureException private[simulator] | ||
| extends RuntimeException( | ||
| dramaticMessage( | ||
| header = Some(s"The test finished and signaled failure"), | ||
| body = "" | ||
| ) | ||
| ) | ||
| with NoStackTrace |
There was a problem hiding this comment.
This should be moved into Simulator.scala's Exceptions object. I also think this is a generic TestFailed exception and not specific to inline unit tests.
src/main/scala/chisel3/simulator/stimulus/InlineTestStimulus.scala
Outdated
Show resolved
Hide resolved
src/main/scala/chisel3/simulator/stimulus/InlineTestStimulus.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This is not for this PR, but we need to move this out of experimental.
| protected final def test[R]( | ||
| protected final def test( | ||
| testName: String | ||
| )(testBody: Instance[M] => R)(implicit testHarnessGenerator: TestHarnessGenerator[M, R]): Unit = { | ||
| )(testBody: Instance[M] => TestConfiguration)(implicit testHarnessGenerator: TestHarnessGenerator[M]): Unit = { |
There was a problem hiding this comment.
At first glance, it seems like the test configuration is a parameter of the test method itself and not the return of the lambda. I.e., it seems more natural for this to be:
protected def test(testName: String, config: TestConfiguration)(body: Instance[R] => Unit)
However, the PR is using this to force the communication of how the success and finish are hooked up.
I don't see anything wrong here, just this is unexpected.
There was a problem hiding this comment.
I agree it's a bit unexpected, but it is useful to have it as a return type so that it can reference hardware in the test body (e.g. some sentinel value to watch for finishing the test).
There was a problem hiding this comment.
LGTM. Please land with the comments addressed.
The one semi-big thing which needs to get sorted out is the reporting. Though, this can happen in follow-ons. There's a pretty big difference between an assertion failing in an inline test vs. an assertion failing in a non-inline test. E.g., if a test fails an assertion inline you see:
[365] - should simulate and fail early if assertion raised *** FAILED ***
[365] chisel3.simulator.Exceptions$TestsFailed: One or more tests failed during simulation
[365] ------------------------------------------------------------------------------
[365] ModuleWithTests tests: 0 succeeded, 1 failed (assertion)
[365] ------------------------------------------------------------------------------
[365] ...
However, this would include much more information if not inline: what assertion failed, the line numbers, etc. The infrastructure for showing this is all there, it just needs to be plumbed through to this output.
Also, we should probably err on the side of providing too much information. E.g., just binning by assertion assert or timeout is likely not enough.
This isn't blocking and I am confident that this will get sorted out as we use it.
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash)and clean up the commit message.Create a merge commit.