feat: SpinalTestBench

[numero-744]

Continuation of #965 (thanks for your feedback @andreasWallner @Readon)

Context, Motivation & Description

Scalatest is useful to quickly write unit / regression tests, but it is not really practical for hardware description testing:

• we have to repeat the test name with the Spinal test function
• the official documentation website is almost always down
• me trying to use always starts with: "where is the thing to extend? import scalatest.funs… no, anyfuns… Err, is it FlatSuite?"

This PR mixes Spinal's doSim and doSimUntilVoid functions with scalatest features to provide the user a unified SpinalTestBench, which is a part of spinal.core.sim._ so no need to import more things!

What is a SpinalTestBench?

A SpinalTestBench contains at least a Dut[Device], which is kind of a SimCompiled[Device] but with more context to give you neat scalatest reports.

class MyComponentSpec extends SpinalSpec {
  val myDut = Dut(Identity(cfg))
}

The goal of a bench is to run tests, isn't it? Two ways to do so:

myDut should "do something" inSim { dut =>
  // some code
}

myDut.test("some test") { dut =>
  // some code
}

Just created 3 tests. Three? Yes, three:

• myDut compiles
• myDut should do something
• myDut test: some test

So if compilation fails, the failure is reported, and tests on this bench are cancelled. ✨ If it succeeds (why would it fail? 🤔) it is not re-compiled by default (but it can be configured, see below).

(Btw, it sometimes happened to me to get immediate "all tests are successful" just because compilation failed before the tests, so now it won't happen anymore 😃)

Configurable components

For a configurable component, I suggest to use TestSuite

// Using `cfg => MyComponent(cfg)` to build tests
// Using cache to not create component with same config twice over several test suites
val dutCache = DutBuilder.fromDevice(MyComponent).withCache

val testSuite = TestSuite(dutCache) { (cfg, it) =>
  it should "do stuff" inSim { dut =>
    // dam dam dee-dam
  }

  // Tests can be enabled conditionally over the current configuration
  if (cfg.size >= 8) {
    it should "lack of inspiration" inSimUntilVoid { dut => }
  }
}

val cfgs = for (n <- 5 to 7) yield MyConfig(n)
testProtocol.runWithAll(cfgs)

Oh, btw, if you just want to check that a configuration is valid or not, you can!

val goodDut = Dut(MyComponent(goodCfg))
goodDut should compile

val badDut = Dut(MyComponent(badCfg))
badDut shouldNot compile

Configuration

You can configure:

• config: SpinalSimConfig (defaults to SimConfig) by overriding defaultConfig in the spec or with bench(finalConfig=...)
• caching: Boolean (defaults to true) to cache SimCompiled in a bench, adding a test for bench compilation. Setting it to false removes the test for compilation and re-compiles on each test, so if compilation fails, all tests on this bench fail. Configure it by overriding defaultCaching in the spec or when calling bench(finalCaching=...).
• seed: Option[Int] (defaults to None) by overriding defaultSeed in the spec or using testOnly MyComponentSpec -- -Dseed=123

Other features

More examples in the file with tests.

Note: The workspace is named after the Dut, to have consistent names instead of "MyComponent_XXX".

Checklist

• Unit tests were added
• API changes are or will be documented:
  • using Scaladoc comments: /** */?
  • on RTD?
  • thanks to a new tracking issue on RTD?

[andreasWallner]

In general this seems nice to use. I also like the little touches, like that compiles the DUT lazily, which should speed of looped tests where many of the tests are filtered out.

The small thing that I'd add is to be able to specify a random seed via the scalatest command line, to reproduce a failure w/o changing the test (via testOnly ... -- -Dseed=x or similar - environment variable doesn't catch that since I wouldn't want to restart sbt for every seed change). (and yes, I'm aware of the irony that I'll contradict myself immediately ;-))

My major concern is that this feels like you tried to account for every possibility, rather than provide something for 90% of the cases that is easy to explain. If I want to mess with the used seed then I can:

• change the environment variable for it
• override the seed member
• pass a seed to the bench
• use withSeed or pass a seed to test
• (if we'd add it pass the seed on the scalatest invocation)
  This seems to complicated for such a simple thing, hard to document, explain and to provide help in e.g. the Gitter.
  Another one I'd cut back is syntax variants of running a test: I'd choose either the FlatSpec style or the test function - no reason to have both?

I think you could should be able to drop the type parameter and simplify this a bit. On some other technical notes I'd try to move Bench and (some?) of the other classes out of the copy of the Spec to avoid confusion between bench and Bench

Edit: I still have to try it here, I'd like to have a look at the naming of the testcases with the various features. Being able to filter for specific testcases is one of the most used scalatest features for me...

[numero-744]

About seed from scalatest command line, can you point me to an example of code getting this value from the Scala code? Never used that so far.

Actually I never set the seed myself, so I don't really know the use case. How do you think should we manage this? Not having the seed in the code at all would be great I think, so maybe just get the optional flag as you suggested, else let Spinal manage the seed with environment variables?

Why having both should and test? I would say should because it is a nice syntax and test because sometimes you don't want to express things with should. For instance, when I want to do debug tests on a spec with diagrams, I would do test("Figure 5: The figure 5 title") and mimic the diagram in the test. (We could even take a Wavedrom diagram but that's another topic #734 XD)

About bench vs Bench, well I would say it is usual function vs Type with conventional casing. Also Bench is not case so that users are less tempted XD Is it okay for you?

About moving stuff outside, I don't want to pollute the namespace, and as far as these are only used in SpinalSpec, I think it is better to let them there.

[numero-744]

I found that, for -- -Dseed=10, just need to convert to integer, now 😉 https://stackoverflow.com/questions/35433177/passing-additional-arguments-to-tests-with-scalatest

[andreasWallner]

Actually I never set the seed myself, so I don't really know the use case.
My use case is just during debugging: I run with random seed until I start debugging an issue, then I fix it so that it doesn't e.g. just go away because of different timing/data. When that specific bug is fixed I run with random again.

About moving stuff outside, I don't want to pollute the namespace, and as far as these are only used in SpinalSpec, I think it is better to let them there.

As far as I know, the usual way to do that in scala would be to put the classes in a companion object. That way they don't show up in intellisense (if you use such a thing) or the global namespace. They also don't get a closure of the surrounding class.

I found that, for -- -Dseed=10, just need to convert to integer, now wink https://stackoverflow.com/questions/35433177/passing-additional-arguments-to-tests-with-scalatest

Sorry, I thought you might have seen it when I posted the link on your original PR. I also had to look for it for quite some time: https://github.com/andreasWallner/spinalStuff/blob/dbde3792e46be4230995291ec727e38fffbc31eb/src/test/scala/andreasWallner/SpinalFunSuite.scala#L36

Why having both should and test?

Fair, I had thought it might avoid confusion to have 2 ways to do the exact same thing, with only minor wording diff.

[numero-744]

I'm trying to move Bench to a companion object but there are errors with the in and cancel placed outside the Spec and I'm not sure I can fix that.

My bad, I hadn't noticed that part of your code the first time and I didn't think to look at it for -D.

About test, is testUntilVoid name okay?

[andreasWallner]

I'm trying to move Bench to a companion object but there are errors with the in and cancel placed outside the Spec and I'm not sure I can fix that.

I had a quick look in the webinterface and only found a single reference to cancel, what actually is it?

My bad, I hadn't noticed that part of your code the first time and I didn't think to look at it for -D.

No worries, I did want to send you hunting though^^

About test, is testUntilVoid name okay?

Seems ok

Btw. I noticed that the code in sim and should is pretty similar, just with minor differences, but I guess you'll move that to a common function anyways?

[numero-744]

cancel is what I use to mark simulation tests as cancelled when the cached compilation test failed.

You are right, it should compile can be simplified, but not with sim as in this very function it would put two tests with the same name when caching is enabled, which is not allowed by ScalaTest. I simplified it should compile another way, forcing to build the cached bench, as the "compiles" test is already performed in this function.

With both this version and the previous one, it is not allowed to do it should compile for an already compiled and cached bench because it would imply running the same test twice. I'm forcing here it for two reasons:

• When the cache is not enabled, the test still has to be run
• When the cache is enabled, I prefer to make explicit the error of putting the same test twice rather than silently remove the second one.

[numero-744]

Rebased on dev and cleaned history

@andreasWallner I think I have nothing left to modify, waiting for your approval 😃

[numero-744]

So should all overridable defaults be def then?

[numero-744]

@andreasWallner can you review, please?

[andreasWallner]

Rebased on dev and cleaned history

@andreasWallner I think I have nothing left to modify, waiting for your approval 😃

Sorry for the delay, I was in a workshop the last few days, will have a look in the evening.

[numero-744]

@andreasWallner Great! Also, please don't merge it now: the way you modified build.sbt is better so let's get your PR merged first 😃

[andreasWallner]

Small comment on naming: Bench might be easily misunderstood, I checked with some of my digital designer colleagues and they would expect a similar meaning to what Wikipedia mentions:

The term "test bench" is used in digital design with a hardware description language to describe the test code, which instantiates the DUT and runs the test.

Where only the first part is true here, not the second. (actually less true if you do the move ;-) )
But I see why you used it, and I don't have an alternative I'm really happy with...

[Dolu1990]

Let's not have scala-test in lib, but in tester ? add this utility in the tester/main and add tester in the releases as a library ?

[numero-744]

@Dolu1990 So for the user who wants to use the feature in this PR it implies that:

• we release tester
• user has to add import spinal.tester._?

[Dolu1990]

@numero-744 Yes

[Dolu1990]

Also, instead of SpinalSpec, what's about SpinalTest ?

[Dolu1990]

PR about scala-test :
#980

If you are good with #980 we can merge #980 first i think

[numero-744]

@andreasWallner benchTransform contains things around the component (hence the initial name Bench) but if we move / delay transformation in user's code, maybe bench and Bench can become dut/Dut? Then thisDut should "do something" inSim, I think it is more consistent than previously 😃 I'm going on that and I'll seek inspiration to have a nice API for the transformation stuff 🤔

@Dolu1990 The current name is after *Spec from scalatest as it makes it possible to use the it should syntax, but maybe it is not a really good name… I think SpinalTest is too close to the notion of one single test. What do you think about SpinalTestBench, if renaming mentioned above is performed? @andreasWallner it would match the definition, isn't it?

I have set this PR as draft, I'll commit forward so that you can identify the new suggestions. I'll rebase when we agree on the result before removing draft status.

[Dolu1990]

About atomicity of the compilatoin cache, it is implemented, but maybe there is holes in it.

Do you have a way to more or less reproduce the issue ?

[numero-744]

@Dolu1990 For now I think it is a scalatest configuration issue, not a Spinal issue: the tests are only registered for later run, and the thing is about controlling in which order they are run, having the compiles test to complete before starting the tests using the Dut.

[numero-744]

FYI short example of use is available in numero-744/Aes/tb/spinal/aes/AesTestBench.scala

Btw I could use .asHex but I was missing BU"x123" here XD

[andreasWallner]

@andreasWallner benchTransform contains things around the component (hence the initial name Bench) but if we move / delay transformation in user's code, maybe bench and Bench can become dut/Dut? Then thisDut should "do something" inSim, I think it is more consistent than previously 😃 I'm going on that and I'll seek inspiration to have a nice API for the transformation stuff 🤔

Not a big fan of DUT since that would be the Component that we are testing itself? What I could offer would be Environment, but looking at pictures like the one on https://www.chipverify.com/systemverilog/systemverilog-simple-testbench this is also not a good fit since none of the typical jargon has words for the build environment around it which bench is doing. Looking at your AES example though it seems to be a great fit there (and you use it already). Taking inspiration from spinal.core.sim might also be CompiledDut or similar?

[numero-744]

Not a big fan of DUT since that would be the Component that we are testing itself?

I would say the component class name should be describing what it does, and only in the context of the testbench it becomes the "device under test" so we can say "the dut is a Dut":

val dut: Dut[MyComponent] = Dut(MyComponent(cfg))
//  ^^^^^^^^ the dut is a Dut
// but for better test reports it is better to use a good name
val myComponent = Dut(MyComponent(cfg))

myComponent should "work" inSim { dut =>
  // hello
}
// => "myComponent should work" test passed

I had taken a look at diagrams like https://www.chipverify.com/systemverilog/systemverilog-simple-testbench to rename the the SoftMyComponent as MyComponentEnv (used with myDut withEnv MyComponentEnv)

[numero-744]

Ready for final review and merge if you have time, else it can wait for v1.8.1 😁

[Readon]

About the directory, is it more clean that if we have
SpinalTestBench to main/scala/spinal/tester/sim
SpinalFormalFunSuite to main/scala/spinal/tester/formal
Also, need to get corresponding package statements in those class modified. Maybe to "package spinal.tester.sim/formal"

Because the tester is a standalone package for the user.

[numero-744]

I don't know for other items, but for SpinalTestBench having it in the same import clause as spinal.core.sim._ can be really handy for the user, to avoid adding one more import line for each single file / component to test. With correct documentation I think it is not a big deal?

If there is a consensus it is really an issue, I'll move it to the tester package, and I can add a testing file template to the snippets so that it doesn't imply more typing for the (codium) user as well.

Also, still in the case we move it to tester package, I suggest to put this PR to standby the time I sort the tests in tester; it will reveal more testing utilities, then with all the testing utilities we can discuss the package structure of tester?

[andreasWallner]

I don't know for other items, but for SpinalTestBench having it in the same import clause as spinal.core.sim._ can be really handy for the user,

To be honest I have no idea how well that plays with other JVM tools, at least for java it would be a no-no to not have the correct library name in the package name. I noticed that scala seems to care less about it.

Apart from that I thought a bit about whether the Dut class would be strictly needed, but I think its good the way it is

[numero-744]

I don't know… If it is an issue with tools, then isn't having package spinal.lib in project lib instead of project spinal an issue, too?

[andreasWallner]

I don't know… If it is an issue with tools, then isn't having package spinal.lib in project lib instead of project spinal an issue, too?

Sadly I don't know - why not stick with it until some issue pops up?

[Readon]

Maybe the library is too big, so mixing the core, simulation, and library together is not a good idea.
Additionally, the simulation subproject only contains backends for various tools, which can also be put into the core.
However, they have very clear targets, so it is a good idea not to tightly couple them to the core.
IMO, It is reasonable to put them into a standalone library.

[Readon]

I don't know for other items, but for SpinalTestBench having it in the same import clause as spinal.core.sim._ can be really handy for the user, to avoid adding one more import line for each single file / component to test. With correct documentation I think it is not a big deal?

Yes, the decision is really hard to make.
On one side, it would be easier to write the code to put SpinalTestBench into namespace spinal.core.sim.
On other side, name space sometimes is a clue for user to find the location of problem while facing an error. That would lead to a change of existing code, not user-friendly.

Also, as we discussed before, if we organize tester structure the same with things to be tested, it would be convenient, which support the method one.

[Readon]

@numero-744 Is there any problem left for this PR?

[numero-744]

@Readon before releasing, we should be more sure about where it should be. So it is about the structure of the tester/main project, which should be the main discussion in #1001

[Readon]

You have put it into the tester/main subproject, so that it is only required in user's test case. I think it's nice.
And the location of this is not related with the other testers which located in tester/test.
So I am not very confirmed why #1001 is related.

[numero-744]

To me it is about things like splitting tester/main into packages to separate formal vs non-formal or not.

The relationship with #1001 is that while moving tests, there were testing utilities that were common to different tested things, so I have put them into tester/main without more organization of this project into several packages. So I would discuss organization of tester/main in #1001 before adding more stuff into tester/main.

[Readon]

To me it is about things like splitting tester/main into packages to separate formal vs non-formal or not.

The relationship with #1001 is that while moving tests, there were testing utilities that were common to different tested things, so I have put them into tester/main without more organization of this project into several packages. So I would discuss organization of tester/main in #1001 before adding more stuff into tester/main.

Because the SpinalTestBench is in spinal.core.sim, so we can just use the path it is now as tester/src/main/scala/spinal/core/sim/SpinalTestBench.scala.
The only difference that would be is to move SpinalTestBench and SpinalFormalFuncSuite into the same spinal.tester package, which I think is not necessary now.

I think the PR is best to be decoupled, so that things would be simplified.

[numero-744]

Also the scalatest dependency (needed for this feature) has been removed from spinal.core/main so it wouldn't work if we merge it as-is.