Add simple API for generating testharnesses inline

[tmckay-sifive]

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• Feature (or new API)

Desired Merge Strategy

• Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Add an API to generate testharnesses inline that are emitted as additional public modules in the output.

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[tmckay-sifive]

Suggested change

	* - The [[desiredName]] is "[[this.desiredName]] _ [[name]]".
	* - The [[desiredName]] is "[[this.desiredName]] _ [[testName]]".

[seldridge]

This took me a while to realize, but this is actually doing a separation of the "test harness" (the body function) from the actual "test". This three-part factoring of (1) DUT, (2) test harness, and (3) test makes sense to me.

With this, have you thought about how to share test harnesses as opposed to having them implicitly inlined in the test? Put differently, is there a way to make the test harness also a module and thereby get the benefits of using D/I on the test harness as opposed to just the DUT?

[seldridge]

Can this trait be inlined into its single extension trait, HasTestWithResult?

Alternatively, this could be flattened in the other direction by making this a method on RawModule.

Pre-emptively creating inheritance hierarchies is usually an anti-pattern. (Or: more strongly, inheritance hierarchies are usually an anti-pattern _unless they are architected and/or have some theoretical basis like Scala collections or Cats [1].)

[tmckay-sifive]

Yeah it could be flattened. The use case I had in mind that lead to this separation was that the user might have a component with a bunch of tests, while still wanting to elaborate a one-off parent module that is not the same as the testharnesses for all the tests.

So, I wanted to have an open-ended elaborateParentModule available to the user for that purpose. But, it felt strange to have that in HasTests, since it's really a separate feature from unit tests; granted one that we can leverage to implement unit tests.

That's how I got here. That said, I'm not very opinionated on the organization. So, if you think it's better Scala or more Chisel-like to flatten these traits, I will happily do so.

[seldridge]

A Module with Public being immediately cast to a RawModule with Public seems odd.

[tmckay-sifive]

The intent here is to allow the implementer to override it with something that generates a RawModule.

[tmckay-sifive]

With this, have you thought about how to share test harnesses as opposed to having them implicitly inlined in the test? Put differently, is there a way to make the test harness also a module and thereby get the benefits of using D/I on the test harness as opposed to just the DUT?

Good idea, I will look into that.

[tmckay-sifive]

With this, have you thought about how to share test harnesses as opposed to having them implicitly inlined in the test? Put differently, is there a way to make the test harness also a module and thereby get the benefits of using D/I on the test harness as opposed to just the DUT?

I think this would require the ability to make a Definition of the testharness, with a hole in the middle for the test body. Is this possible with Definition/Instance? I could make the testharness and test-body/DUT sibling modules to accomplish something similar, but I think it's important that the DUT instance and test RTL live underneath the testharness hierarchy.

[jackkoenig]

I have feedback similar to Schuyler's. I think it's really important that we separate the definitions of TestHarnesses from the body of modules that need to be tested. Related to this, I think the type of the TestHarness (and of the TestResult) should not be set for a given module. It seems likely that a single module would want to have tests that have different testharnesses and results. Really, the type of the result should probably be tied to the type of the TestHarness, I don't think they're independent parameters.

I think we can do this with typeclasses, but perhaps we should sit down together to work through it (hard to do in a quick sketch).

A second order concern (and non-blocking)--I think it is useful to be able to create little tests inline so no issues there, but it should not be the only way. We need to think about test composition where I could define a testharness, write a test against the testharness and some DUT interface, and then have multiple different modules use that test for themselves.

Another non-blocking concern, it should be easy to include or elide tests written this way. I should be able to elaborate a module and not also be required to elaborate all of the tests. That can come later though.

[jackkoenig]

Style note, package objects should only be used for things that must be in an object but you want to be accessible within what is otherwise a package. Every top-level definition here is a regular class, trait or object, so there's no reason to use a package object.

Also, the file structure needs to match the package structure.

So basically, please move inlinetest up to the package, and put this code in src/main/scala/chisel3/experimental/inlinetest/...

[tmckay-sifive]

Gotcha, thanks. I'm not 100% I did what you asked, but I gave it a shot in 5e5bdd4

[jackkoenig]

Generally this looks great, a few points to consider.

[jackkoenig]

Nit, the name of this file shouldn't be package.scala, that is reserved for package objects, e.g. [1].

I'm not sure what to call this file since it has basically everything in the package in it, maybe just InlineTest.scala (still in directory src/main/scala/chisel3/experimental/inlinetest/) and we can break it up later if we see fit?

[jackkoenig]

Is the type of M every really used? Or put another way--what is the use case for a TestHarness that only works for a particular kind of DUT Module? I suspect there is one, but maybe it would be good to have an example in the tests to drive the point home.

[tmckay-sifive]

Yep I think it's useful to have when you want to reuse a testharness across DUTs where your testharness expects certain interfaces to be present on the DUT. Added an example.

[jackkoenig]

This is an annoying detail but one we have to consider as library writers in Chisel.

What are the odds that we are going to have to evolve this type over time, like adding fields? I'm assuming it's high--if so, we should not make this a case class because case classes are nightmares for both source and binary compatibility.

Also, should users be able to create instances of this object or not? Maybe the constructor, factory apply, and copy methods should all be private (we won't have the latter two if we make this not a case class).

[tmckay-sifive]

What are the odds that we are going to have to evolve this type over time, like adding fields? I'm assuming it's high--

Yeah, I would expect we will at some point.

if so, we should not make this a case class because case classes are nightmares for both source and binary compatibility.

Good to know. I made it a case class precisely because I wanted to avoid changes to the parameter list of the methods that take them.

I've changed it to a class for now.

[jackkoenig]

Another nit but something to think about. Neither UnitTestHarness nor TestHarnessWithResultIO extend this trait which leads me to believe TestHarness is the wrong name for this TestHarness-providing-typeclass. I am not sure of the right name though. TestHarnessGenerator? @seldridge any ideas?

[tmckay-sifive]

Yeah I think TestHarnessGenerator makes more sense. I've changed it to that pending other opinions.

[tmckay-sifive]

Incidentally, the extra logic I added to the tests now either produces invalid FIR or hits a bug in firtool.

FIRRTL version 4.2.0
circuit test_ModuleWithTests_with_monitor :
  layer Verification, bind, "verification" :
    layer Assert, bind, "verification/assert" :
    layer Assume, bind, "verification/assume" :
    layer Cover, bind, "verification/cover" :

  module ModuleWithTests :
    input clock : Clock
    input reset : UInt<1>
    output io : { flip in : UInt<32>, out : UInt<32>}
    output monProbe : Probe<{ in : UInt<32>, out : UInt<32>}>

    define monProbe = probe(io)
    connect io.out, io.in

  module ProtocolMonitor :
    input clock : Clock
    input reset : Reset
    input io : { in : UInt<32>, out : UInt<32>}

    node _T = eq(io.in, io.out)
    node _T_1 = asUInt(reset)
    node _T_2 = eq(_T_1, UInt<1>(0h0))
    layerblock Verification :
      layerblock Assert :
        intrinsic(circt_chisel_ifelsefatal<format = "Assertion failed: in === out\n", label = "chisel3_builtin">, clock, _T, _T_2)

  public module test_ModuleWithTests_with_monitor :
    input clock : Clock
    input reset : UInt<1>

    inst dut of ModuleWithTests
    connect dut.clock, clock
    connect dut.reset, reset
    connect dut.io.in, UInt<32>(0h5)
    node _T = neq(dut.io.out, UInt<1>(0h0))
    node _T_1 = eq(reset, UInt<1>(0h0))
    layerblock Verification :
      layerblock Assert :
        intrinsic(circt_chisel_ifelsefatal<format = "Assertion failed at InlineTestSpec.scala:110\n", label = "chisel3_builtin">, clock, _T, _T_1)

    inst monitor of ProtocolMonitor
    connect monitor.clock, clock
    connect monitor.reset, reset
    connect monitor.io.out, read(dut.monProbe).out
    connect monitor.io.in, read(dut.monProbe).in

  circt.stage.phases.Exceptions$FirtoolNonZeroExitCode: /sifive/tools/llvm/circt/1.103.0/bin/firtool returned a non-zero exit code. Note that this version of Chisel (7.0.0-M2+306-941cc4b7-SNAPSHOT) was published against firtool version 1.100.0.
------------------------------------------------------------------------------
ExitCode:
1
STDOUT:

STDERR:
src/test/scala/chisel3/experimental/InlineTestSpec.scala:20:23: error: 'firrtl.instance' op has a wrong number of results; expected 2 but got 4
    val dut = Instance(test.dutDefinition)
                      ^
src/test/scala/chisel3/experimental/InlineTestSpec.scala:20:23: note: see current operation: %5:4 = "firrtl.instance"() <{annotations = [], layers = [], moduleName = @ModuleWithTests, name = "dut", nameKind = #firrtl<name_kind droppable_name>, portAnnotations = [[], [], [], []], portDirections = array<i1: false, true, true, true>, portNames = ["io_in", "io_out", "monProbe_in", "monProbe_out"]}> : () -> (!firrtl.uint<32>, !firrtl.uint<32>, !firrtl.probe<uint<32>>, !firrtl.probe<uint<32>>)
src/test/scala/chisel3/experimental/InlineTestSpec.scala:69:2: note: original module declared here
@instantiable
 ^

Not sure which yet, need to look into it.

[tmckay-sifive]

Not sure which yet, need to look into it.

Turns out CIRCT bug. I've uncommented the test so it will continue to fail until fixed. Might need to rewrite the test to circumvent the bug if it's not fixed soon.