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

move tests to speed up ci. #1192

Merged
merged 54 commits into from
Sep 4, 2023
Merged

move tests to speed up ci. #1192

merged 54 commits into from
Sep 4, 2023

Conversation

Readon
Copy link
Collaborator

@Readon Readon commented Sep 3, 2023

Closes #982 continue #1001

Context, Motivation & Description

distribute some tests in tester to speed up CI.

Impact on code generation

Checklist

  • Unit tests were added
  • API changes are or will be documented:

@Readon
Copy link
Collaborator Author

Readon commented Sep 3, 2023

now CI is only take 31 minutes for one run.

@Dolu1990
Copy link
Member

Dolu1990 commented Sep 3, 2023

<3
Let's try to make it merged fast, as it move a lot of stuff around.

@andreasWallner
Copy link
Collaborator

andreasWallner commented Sep 3, 2023

In general I'm all for this, but the unmanaged dependencies make me a bit weary. I understand that it doesn't compile without them as is, and I checked a few resources on how to structure this (i.e. this one: https://users.scala-lang.org/t/sbt-multi-project-dependencies/7948/8), pretty much all advise to have a separate subproject.
I also found https://groups.google.com/g/simple-build-tool/c/RPMxxaRtXn4/m/OX4SLbtdMkEJ which seems to endorse this, but it was for sbt 0.10.

I did a quick check and it seems that even if we have the test in the test subproject we can still replicate the package structure and put additional stuff into e.g. the spinal.core package which makes the IDE features work.
We would also be able to get the CI speedup that you are currently seeing, but in that case by running separate jobs with testOnly spinal.core.* / testOnly spinal.lib.* (and we'd get a similar effect of making it easier to find tests for a certain class).
I did the check by keeping all your source changes, but moving e.g. core/test/scala/spinal/core to tester/test/scala/spinal/core.

To be clear: there is no real technical reason I can point to. The main motivation is that I've been on and off debugging why SpinalHDL is so often rebuilt when running sbt from the command line and from googling I often found issues around unmanagedClasspath. It also seems weird to work around the package manager in that way if there is an alternative.

@Readon
Copy link
Collaborator Author

Readon commented Sep 4, 2023

In general I'm all for this, but the unmanaged dependencies make me a bit weary. I understand that it doesn't compile without them as is, and I checked a few resources on how to structure this (i.e. this one: https://users.scala-lang.org/t/sbt-multi-project-dependencies/7948/8), pretty much all advise to have a separate subproject. I also found https://groups.google.com/g/simple-build-tool/c/RPMxxaRtXn4/m/OX4SLbtdMkEJ which seems to endorse this, but it was for sbt 0.10.

I did a quick check and it seems that even if we have the test in the subproject we can still replicate the package structure and put additional stuff into e.g. the package which makes the IDE features work. We would also be able to get the CI speedup that you are currently seeing, but in that case by running separate jobs with / (and we'd get a similar effect of making it easier to find tests for a certain class). I did the check by keeping all your source changes, but moving e.g. to .test``spinal.core``testOnly spinal.core.*``testOnly spinal.lib.*``core/test/scala/spinal/core``tester/test/scala/spinal/core

To be clear: there is no real technical reason I can point to. The main motivation is that I've been on and off debugging why SpinalHDL is so often rebuilt when running sbt from the command line and from googling I often found issues around unmanagedClasspath. It also seems weird to work around the package manager in that way if there is an alternative.

So do you mean if we move core/src/test/scala/spinal/core directory into tester/src/test/scala/spinal/core would be better than put them into the core lib's test?

@Readon
Copy link
Collaborator Author

Readon commented Sep 4, 2023

In general I'm all for this, but the unmanaged dependencies make me a bit weary. I understand that it doesn't compile without them as is, and I checked a few resources on how to structure this (i.e. this one: https://users.scala-lang.org/t/sbt-multi-project-dependencies/7948/8), pretty much all advise to have a separate subproject. I also found https://groups.google.com/g/simple-build-tool/c/RPMxxaRtXn4/m/OX4SLbtdMkEJ which seems to endorse this, but it was for sbt 0.10.
I did a quick check and it seems that even if we have the test in the subproject we can still replicate the package structure and put additional stuff into e.g. the package which makes the IDE features work. We would also be able to get the CI speedup that you are currently seeing, but in that case by running separate jobs with / (and we'd get a similar effect of making it easier to find tests for a certain class). I did the check by keeping all your source changes, but moving e.g. to . testspinal.coretestOnly spinal.core.*testOnly spinal.lib.*core/test/scala/spinal/coretester/test/scala/spinal/core ``
To be clear: there is no real technical reason I can point to. The main motivation is that I've been on and off debugging why SpinalHDL is so often rebuilt when running sbt from the command line and from googling I often found issues around unmanagedClasspath. It also seems weird to work around the package manager in that way if there is an alternative.

So do you mean if we move core/src/test/scala/spinal/core directory into tester/src/test/scala/spinal/core would be better than put them into the core lib's test?

How about Readon/SpinalHDL@move-tests this one? Does it work in your IDEA environment?

@Readon
Copy link
Collaborator Author

Readon commented Sep 4, 2023

I have tested on my server that "go to test" function could only jump to files that with the right filename, such as xxxTest.scala for yyy.scala which include xxx class's definition.
This is only tests on Axi4WriteOnlyUpsizer and Axi4WriteOnlyDownsizer.

However, for Axi4Upsizer and Axi4Downsizer it works.

@Readon Readon marked this pull request as draft September 4, 2023 08:51
@Readon Readon marked this pull request as ready for review September 4, 2023 14:50
@Dolu1990
Copy link
Member

Dolu1990 commented Sep 4, 2023

hmmm kind of like it (stuff still in tester module, but in proper separated packages)
I think for me that's good for merge.

@Readon Readon merged commit fae3461 into SpinalHDL:dev Sep 4, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

About tests organization
3 participants