Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

About tests organization #982

Closed
numero-744 opened this issue Nov 30, 2022 · 35 comments · Fixed by #1192 · May be fixed by #1001
Closed

About tests organization #982

numero-744 opened this issue Nov 30, 2022 · 35 comments · Fixed by #1192 · May be fixed by #1001
Assignees

Comments

@numero-744
Copy link
Collaborator

Below is the first part of the path:

tester/src/test/scala/spinal/

The standard for unit tests would be:

PROJECT/src/test/scala/spinal/

So let's admit that (most) tests running simulations are integration tests, hence putting them in a separate tester/ module. It also simplifies dependencies management for the day we want to use features released in tester/ to test something from core/ or lib/.

IMHO nothing to change here 👍


tester/src/test/scala/spinal/tester/
                             ^^^^^^

Then there is the tester/ folder. As it is about integration tests, it forces to import tested things (else using the same package as the test being run would bring in scope the stuff if the same package), making tests look more like examples, to better test the user's API, which seems to be a good thing.

IMHO nothing to change here 👍


Then there are lots of tests with poor organization. Below is a suggestion:

  • drafts/ for worksheets (old things that could be removed IMHO), so that most contributors can forget them
  • path/to/item.scala where the first element of the path is basically core, lib, etc. without being repeated.
  • issues/IssueXXX.scala for bug fixes

For instance:

tester/src/test/scala/spinal/tester/scalatest/AhbLite3InterconnectTester.scala

tests:

lib/src/main/scala/spinal/lib/bus/amba3/ahblite/AhbLite3Interconnect.scala

So it would be moved to:

tester/src/test/scala/spinal/tester/lib/bus/amba3/ahblite/AhbLite3InterconnectTester.scala

Tests about different elements (elements being tested, not elements used to test other elements) would be in the deepest common element in the paths.

I think it is okay to have Apps to test Scala syntax and types among scalatest-run tests.

This way it would be easier to find and add more tests.

@numero-744 numero-744 added need agreement 💬 Waiting for approval to start implementing a feature, or merge a PR meta 🌐 About SpinalHDL's life labels Nov 30, 2022
@andreasWallner
Copy link
Collaborator

I'd say that the standard for tests would be to be placed in PROJECT/src/test/... not only unit tests. That is e.g. also the location that IntelliJ and other IDEs would go to if you use their "go to test" functionality. That is also the reason why that folder is not included in normal sbt build/publish.
But in the end that doesn't matter too much, what would help is to copy the structure like you've shown above to make it easier to check what test already exists for a given component/api/helper/etc. IMO

@Readon
Copy link
Collaborator

Readon commented Dec 1, 2022

tester/src/test/scala/spinal/tester/lib/bus/amba3/ahblite/AhbLite3InterconnectTester.scala

Then does

tester/src/test/scala/spinal/lib/bus/amba3/ahblite/AhbLite3InterconnectTester.scala
//^^        ^^^                  ^^^^ //here, tester is removed.
//project.    test directory.

this path better?
However, I am not an expert of scalatest & sbt, could not tell if the difference is acceptable for sbt.

@numero-744
Copy link
Collaborator Author

Here following the package/file parallel it has to start with package spinal.lib.bus.amba3.ahblite so as mentioned earlier it would automatically bring in scope the stuff if the same package, so it reflects less real-case user code.

@Readon
Copy link
Collaborator

Readon commented Dec 1, 2022

Here following the package/file parallel it has to start with package spinal.lib.bus.amba3.ahblite so as mentioned earlier it would automatically bring in scope the stuff if the same package, so it reflects less real-case user code.

Emm, I suppose that is how Scala do. I mean, put the unit test in the same package of DUT. Only keep it away from publishing.
That's why all test suites are just put in corresponding test directory.

Btw, neither the simulation code nor the formal verification code could be run as standalone example just by "copy & paste".
The only related thing is the creation of the DUT.

@numero-744
Copy link
Collaborator Author

Ah so here it is just about conventions. AFAIK the only common convention is the one @andreasWallner mentioned.

I think ideally the best would be to go with this one but I'm afraid of cyclic dependencies if we want to use a feature from tester in one of the tests in lib; but maybe there won't have one as it would be added only as a test dependency? We could try and see what it gives.

@andreasWallner Usually unit tests are placed in a location relative to the tested thing, and integration test are put on a completely separate project, to integrate from another point a view with clean public-only API. Also it is "go to test" (singular) so I think it is implicitly "the unit test for this", not "tests for this, unit and integration". However Scala has everything public by default so maybe it is different from what I'm used to ^^'

@andreasWallner
Copy link
Collaborator

@numero-744 I had a quick look around a few libraries (https://github.com/scalanlp/breeze, https://github.com/haifengl/smile, https://github.com/scalanlp/nak, https://github.com/intel-analytics/BigDL - chosen at random (via google search for scala libraries). They all use the src/main & src/test folder structure (it's also recommended in the sbt docs: https://www.scala-sbt.org/1.x/docs/Directories.html#Source+code, referencing the maven docs: https://maven.apache.org/guides/introduction/introduction-to-the-standard-directory-layout.html).

And I think we could have long discussions on what is a unit test vs. integration test for e.g. spinal.lib ;-) Not sure there needs to be a strict definition/dividing line. In my projects I place all my tests in the test folder of the library, simply because of convenience: if I open that library in an IDE it will show me both code and tests. If the tests are in a separate library I need to make sure that that library is compiled/run as well. (which doesn't mean it has to be the same here - the only thing that matters is that those working on the code find the respective tests...)

If there are really features in tester that are needed in other test libs as well I presume it would then make sense to just move them to core, lib or sim accordingly? Chances are that users might need those as well. That may very well need a refactoring but is then still a net gain.

One additional thing: package private content can only be tested if it is the same library&package.

@numero-744
Copy link
Collaborator Author

The initial situation was that such testing features were in lib (and I wanted to add #968 in core.sim). It is being moved #980 to tester to avoid the mandatory dependency of scalatest #968 (comment)

@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

The initial situation was that such testing features were in lib (and I wanted to add #968 in core.sim). It is being moved #980 to tester to avoid the mandatory dependency of scalatest #968 (comment)

I look over the sbt's usage, there is a test-scoped dependency, which can help to prevent the runtime depends on libraries only required in test. So scalatest as a standard test framework of SpinalHDL would not cause a conflict with user's test framework unless they use helper class defined in SpinalHDL, like SpinalFormalFunSuite.

@numero-744
Copy link
Collaborator Author

So scalatest as a standard test framework of SpinalHDL would not cause a conflict with user's test framework unless they use helper class defined in SpinalHDL, like SpinalFormalFunSuite.

If we put testing helpers like SpinalFormalSuite in test scope, I think they are just not accessible at all by the user. Hence putting them in another released library, so that users can explicitly choose to have them or not.

@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

If I describe the functions of each subproject as follows, would there be any problems?

  1. core: provide the abilities to support basic syntax and error checking.
  2. lib: provide basic component help to speed up design.
  3. sim: internal usage only library add support backend for various simulation engines.
  4. tester: test helper classes to speed up the test, such as SpinalTestBench, SpinalFormalFunSuite. as well as the GlobalClock which provide cross domain clock test helper.

@numero-744
Copy link
Collaborator Author

One thing: I think sim is for internals, and core.sim should be used by the user instead.

Also there are idsl packages, which should be used by the user.

See https://github.com/SpinalHDL/SpinalTemplateSbt/blob/master/build.sbt

@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

If we put testing helpers like SpinalFormalSuite in test scope, I think they are just not accessible at all by the user. Hence putting them in another released library, so that users can explicitly choose to have them or not.

Yes, as I previously state, SpinalFormalFunSuite only add tags for formal suites which run a seperate jobs in CI.
So let it invisible to user is acceptable.

Of course, that can be exported through a tester subproject, like SpinalTestBench.

And the best is to split the existing tester into their own subproject, if possible.
The existing tester directory is really too large to organize.

@numero-744
Copy link
Collaborator Author

Okay, I'm gonna try to PR that next week-end or before, depending on the time I have, unless someone else wants to do that?

@numero-744 numero-744 self-assigned this Dec 4, 2022
@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

One thing: I think sim is for internals, and core.sim should be used by the user instead.

Also there are idsl packages, which should be used by the user.

See https://github.com/SpinalHDL/SpinalTemplateSbt/blob/master/build.sbt

I have updated the description of sim.

About the idsl plugin and payload, I think they are more like a support library of core, which could not directly used by user.

@numero-744
Copy link
Collaborator Author

Yes but they are in the build.sbt so I guess that even if users do not directly use these function in their code, they have to specify this dependency because it is a compiler plugin? Never tried to remove that tbh, could try that!

@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

so the dependency chain can be:
core/main -> sim, idslplguin
core/test -> sim, idslplugin, tester(test scope)
lib/main -> sim, core
lib/test -> sim, core, tester(test scope)

like this?

In many cases, dependency relation should be unidirectional.
Which means tester could not import core/test and lib/test.

@numero-744
Copy link
Collaborator Author

I think core/test and lib/test just cannot be imported by anything

@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

I think core/test and lib/test just cannot be imported by anything

yes

@numero-744
Copy link
Collaborator Author

I'm falling on this: https://stackoverflow.com/questions/44764692/cyclic-dependencies-between-main-test-modules-in-sbt

Maybe a (really) dirty workaround could be to ln -rs the tester/src/main/scala/* into {core,lib}/src/test/scala/?

https://stackoverflow.com/questions/5917249/git-symbolic-links-in-windows/59761201#59761201

@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

I'm falling on this: https://stackoverflow.com/questions/44764692/cyclic-dependencies-between-main-test-modules-in-sbt

Maybe a (really) dirty workaround could be to ln -rs the tester/src/main/scala/* into {core,lib}/src/test/scala/?

https://stackoverflow.com/questions/5917249/git-symbolic-links-in-windows/59761201#59761201

I tried symlink recently, it works weird. Maybe it's caused by some mistake I have done.

@numero-744
Copy link
Collaborator Author

On my side I would say:

I tried Windows recently, it works weird. Maybe it's caused by some mistake I have done.

(Sorry, Lignux addict here 😆)

Joke apart, maybe the link I have provided at the end of my previous comment can help? I have never tried it because I stay away from Windows weirdness as much as I can.

@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

I'm falling on this: https://stackoverflow.com/questions/44764692/cyclic-dependencies-between-main-test-modules-in-sbt

Maybe a (really) dirty workaround could be to ln -rs the tester/src/main/scala/* into {core,lib}/src/test/scala/?

https://stackoverflow.com/questions/5917249/git-symbolic-links-in-windows/59761201#59761201

Firstly, this link might help.
Secondly, as above suggestion, the code in tester subproject could not use any of core or lib to avoid the cyclic dependency.

@numero-744
Copy link
Collaborator Author

Hmm for #968 I know I know need at least Component and SimCompiled (and maybe other things, I have not checked) from spinal.core._

@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

Joke apart, maybe the link I have provided at the end of my previous comment can help? I have never tried it because I stay away from Windows weirdness as much as I can.

Ah, to recall something from memory, the first reason I gave up on Chisel was that it compiled with an error on Windows.

@numero-744
Copy link
Collaborator Author

Btw what we want is exactly https://doc.rust-lang.org/cargo/reference/features.html but it doesn't seem to be a thing in Scala

@Readon
Copy link
Collaborator

Readon commented Dec 4, 2022

Btw what we want is exactly https://doc.rust-lang.org/cargo/reference/features.html but it doesn't seem to be a thing in Scala

However, we can just work around it. Here might worth a try.

BTW, there are runIvyDeps, compileIvyDeps separately in Mill :) .

@numero-744
Copy link
Collaborator Author

Thanks for the link!

The (really old) suggestion does not work in the current version of sbt so I have found the project, which has of course updated its build.sbt: https://github.com/twitter/finagle/blob/develop/build.sbt#L416 and it seems okay

@andreasWallner
Copy link
Collaborator

I would find it more natural to use the sbt/maven layout, but in the end it's not a huge thing if not.
But if we stay with having the tests in tester then I'd think it would be much easier to find stuff if we would replicate the package structure of the tested code in tester. That would also make it easier to run certain subsets of tests (e.g. for spinal.lib.bus.ahb.*)
And that wouldn't mean that we can't have additional folders as well^^

@numero-744
Copy link
Collaborator Author

@andreasWallner I'm working on the conversion to sbt layout 😉

@numero-744
Copy link
Collaborator Author

While moving stuff I notice that:

  • core/test -> sim, idslplugin, tester(test scope) ++ lib(test scope) as tests also use contents from lib (which make them integration tests IMO but let's put them in core/test for simplicity's sake)
  • feat: SpinalTestBench #968 is gonna help ^^' (waiting for review and maybe merge if you have time)

@numero-744 numero-744 removed the need agreement 💬 Waiting for approval to start implementing a feature, or merge a PR label Dec 5, 2022
@numero-744
Copy link
Collaborator Author

FYI, so far I've done 50% of tests reorganization to standard sbt structure. Will need one more week before opening a PR I think ^^'

@andreasWallner
Copy link
Collaborator

@numero-744 Just one quick thought because you mentioned #968: If you want to switch to that (which is perfectly reasonable) I'd do that in two steps, it's pretty much unreviewable if you move & change much stuff^^

@numero-744
Copy link
Collaborator Author

@andreasWallner The PR I'm preparing is just moving code around ^^'

@numero-744 numero-744 mentioned this issue Dec 18, 2022
4 tasks
@numero-744 numero-744 added code quality ♻️ and removed meta 🌐 About SpinalHDL's life labels Jan 11, 2023
@Readon
Copy link
Collaborator

Readon commented Aug 31, 2023

I'd say that the standard for tests would be to be placed in PROJECT/src/test/... not only unit tests. That is e.g. also the location that IntelliJ and other IDEs would go to if you use their "go to test" functionality. That is also the reason why that folder is not included in normal sbt build/publish. But in the end that doesn't matter too much, what would help is to copy the structure like you've shown above to make it easier to check what test already exists for a given component/api/helper/etc. IMO

I am trying to continue this work, to move some test to lib to reduce the CI time. As lib-test and tester-test tasks in CI could be running on parallel.

So do you know the requirements for "go to test" function to work? This would be considered in mind.
I myself does not use it, some hints is preferred.

For example, there are two testers in Axi4AdapterTester.scala file, one for upsizer, one for downsizer. However Axi4Upsizer and Axi4Downsizer are located in seperate file, does it matter for "go to test" function?

If you want give it a try [Readon/SpinalHDL@move-tests] is the branch.

@Readon Readon mentioned this issue Sep 3, 2023
2 tasks
@andreasWallner
Copy link
Collaborator

For example, there are two testers in Axi4AdapterTester.scala file, one for upsizer, one for downsizer. However Axi4Upsizer and Axi4Downsizer are located in seperate file, does it matter for "go to test" function?

Until now I have just had the tests in the same library, same library with the same folder structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants