Skip to content

Enable sandbox for testing and run tests in parallel #4790

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enable sandbox for testing and run tests in parallel #4790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ jobs:
$CHISEL_FIRTOOL_PATH/firtool -version >> $GITHUB_STEP_SUMMARY
echo \`\`\` >> $GITHUB_STEP_SUMMARY
- name: Test
run: ./mill -j0 firrtl.cross[].test -oF + svsim.cross[].test -oF + chisel[].test -oF
run: ./mill firrtl.cross[].test -oF + svsim.cross[].test -oF + chisel[].test -oF

mill:
name: compile project with mill
Expand Down Expand Up @@ -178,7 +178,7 @@ jobs:
dir=$(dirname $(which firtool))
echo "CHISEL_FIRTOOL_PATH=$dir" >> "$GITHUB_ENV"
- name: Integration Tests
run: ./mill -j0 integration-tests.cross[].test -oF
run: ./mill integration-tests.cross[].test -oF

# Currently just a sanity check that the benchmarking flow works
benchmark:
Expand Down
3 changes: 0 additions & 3 deletions build.mill
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,6 @@ trait Chisel extends CrossSbtModule with HasScala2MacroAnno with HasScala2Plugin
object test extends CrossSbtTests with TestModule.ScalaTest with ScalafmtModule {
def ivyDeps = Agg(v.scalatest, v.scalacheck)

// TODO: enable sandbox and run tests in parallel
override def testSandboxWorkingDir = false

// Suppress Scala 3 behavior requiring explicit types on implicit definitions
// Note this must come before the -Wconf is warningSuppression
override def scalacOptions = Task { super.scalacOptions() :+ "-Wconf:cat=other-implicit-type:s" }
Expand Down
8 changes: 4 additions & 4 deletions overlay.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
final: prev:
{
mill = (prev.mill.overrideAttrs (oldAttrs: rec {
version = "0.12.5";
src = prev.fetchurl {
url = "https://github.com/com-lihaoyi/mill/releases/download/${version}/${version}-assembly";
hash = "sha256-DHslQS/uzwbZVdATQY3pqQgM51W+26x2AckQnDPVoFc=";
version = "0.12.7";
src = final.fetchurl {
url = "https://repo1.maven.org/maven2/com/lihaoyi/mill-dist/${version}/mill-dist-${version}-assembly.jar";
hash = "sha256-bbx1NtEYtYbCqp8nAl/d6F5jiJFN0IkUsdvLdBcMg+E=";
};
})).override {
jre = final.openjdk21;
Expand Down
2 changes: 1 addition & 1 deletion src/test/scala-2/chiselTests/BlackBoxImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class BlackBoxMinusPath extends HasBlackBoxPath {
val in2 = Input(UInt(16.W))
val out = Output(UInt(16.W))
})
addPath(new File("src/test/resources/chisel3/BlackBoxTest.v").getCanonicalPath)
addPath(new File(s"${sys.env.get("MILL_TEST_RESOURCE_DIR").get}/chisel3/BlackBoxTest.v").getCanonicalPath)
}

class UsesBlackBoxMinusViaResource extends Module {
Expand Down
2 changes: 1 addition & 1 deletion src/test/scala-2/chiselTests/ExtModuleImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class ExtModuleMinusPath extends ExtModule with HasExtModulePath {
val out = Output(UInt(16.W))
})
addPath(
new File("src/test/resources/chisel3/BlackBoxTest.v").getCanonicalPath
new File(s"${sys.env.get("MILL_TEST_RESOURCE_DIR").get}/chisel3/BlackBoxTest.v").getCanonicalPath
)
}

Expand Down
4 changes: 1 addition & 3 deletions src/test/scala-2/chiselTests/simulator/SimulatorSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,6 @@ class SimulatorSpec extends AnyFunSpec with Matchers {
(thrown.getMessage must include).regex(
""" @\[src/test/scala-2/chiselTests/simulator/SimulatorSpec\.scala:\d+:\d+\]"""
)
thrown.getMessage must include("gcd.io.result.expect(5)")
thrown.getMessage must include(" ^")
Comment on lines -96 to -97
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is annoying as this is the exact thing this test is trying to check.

Does this continue to work if simulate is passed firtoolOpts = Array(s"-include-dir=${sys.env.get("MILL_TEST_RESOURCE_DIR").get}") ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this check depends on elaborate time options like --source-root rather than the firtool option, so the major problem is that we cannot easily modify it without refactoring Simulator.simulate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any advice to change it for passing chisel option? If necessary I'd like to make this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simulator.simulate(...) has a firtoolOpts argument you can use. It also takes a chiselOpts argument if you need to set --source-root.

Or: what is the source locator that is produced? If that is an absolute path already, then it may be as simple as doing --include-dir=/. However, if this uses a synthetic, sandbox directory, then this would be harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh..I see, the arguments are added after this PR, let me update the branch to fix it.

Or: what is the source locator that is produced? If that is an absolute path already, then it may be as simple as doing --include-dir=/. However, if this uses a synthetic, sandbox directory, then this would be harder.

the source locator works fine and the caret is produced by Chisel rather than firtool thus I think --include-dir does not make sense. The problem is --source-root is an elaborate time option but the caret is produced in simulation time, which does not capture it (see

chisel/src/main/scala/chisel3/simulator/PeekPokeAPI.scala

Line 169 in 8498293

ExceptionHelpers.getErrorLineInFile(Seq(), sl)
), thus we may need to parse the option in simulate and keep it in simulation time.

}

it("runs a design that includes an external module") {
Expand All @@ -118,7 +116,7 @@ class SimulatorSpec extends AnyFunSpec with Matchers {

class Qux extends ExtModule with HasExtModulePath {
val a = IO(Output(Bool()))
addPath("src/test/resources/chisel3/simulator/Qux.sv")
addPath(s"${sys.env.get("MILL_TEST_RESOURCE_DIR").get}/chisel3/simulator/Qux.sv")
}

class Foo extends RawModule {
Expand Down
27 changes: 20 additions & 7 deletions src/test/scala-2/circtTests/stage/ChiselStageSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ class ChiselStageSpec extends AnyFunSpec with Matchers with chiselTests.LogUtils
}

it("should emit Annotations inline in emitted CHIRRTL") {
val targetDir = os.pwd / "ChiselStageSpec" / "should-inline-Annotations-in-emitted-CHIRRTL"
val targetDir = baseDir / "ChiselStageSpec" / "should-inline-Annotations-in-emitted-CHIRRTL"

val args: Array[String] = Array(
"--target",
Expand Down Expand Up @@ -582,7 +582,12 @@ class ChiselStageSpec extends AnyFunSpec with Matchers with chiselTests.LogUtils
intercept[java.lang.Exception] {
(new ChiselStage)
.execute(
Array("--target", "chirrtl"),
Array(
"--target",
"chirrtl",
"--source-root",
s"${sys.env.get("MILL_WORKSPACE_ROOT").get}"
),
Seq(ChiselGeneratorAnnotation(() => new ChiselStageSpec.RecoverableError))
)
}
Expand All @@ -598,11 +603,14 @@ class ChiselStageSpec extends AnyFunSpec with Matchers with chiselTests.LogUtils
}

it("should NOT include source line and caret with an incorrect --source-root") {
val incorrectRoot = new File(s"incorrect_root")
incorrectRoot.mkdirs()

val (stdout, stderr, _) = grabStdOutErr {
intercept[java.lang.Exception] {
(new ChiselStage)
.execute(
Array("--target", "chirrtl", "--source-root", ".github"),
Array("--target", "chirrtl", "--source-root", incorrectRoot.toString),
Seq(ChiselGeneratorAnnotation(() => new ChiselStageSpec.RecoverableError))
)
}
Expand All @@ -628,7 +636,7 @@ class ChiselStageSpec extends AnyFunSpec with Matchers with chiselTests.LogUtils
"--source-root",
".",
"--source-root",
"src/test/resources/chisel3/sourceroot1"
s"${sys.env.get("MILL_TEST_RESOURCE_DIR").get}/chisel3/sourceroot1"
),
Seq(ChiselGeneratorAnnotation(() => new ChiselStageSpec.RecoverableErrorFakeSourceInfo))
)
Expand All @@ -651,9 +659,9 @@ class ChiselStageSpec extends AnyFunSpec with Matchers with chiselTests.LogUtils
"--target",
"chirrtl",
"--source-root",
"src/test/resources/chisel3/sourceroot2",
s"${sys.env.get("MILL_TEST_RESOURCE_DIR").get}/chisel3/sourceroot2",
"--source-root",
"src/test/resources/chisel3/sourceroot1"
s"${sys.env.get("MILL_TEST_RESOURCE_DIR").get}/chisel3/sourceroot1"
),
Seq(ChiselGeneratorAnnotation(() => new ChiselStageSpec.RecoverableErrorFakeSourceInfo))
)
Expand All @@ -671,7 +679,12 @@ class ChiselStageSpec extends AnyFunSpec with Matchers with chiselTests.LogUtils
val e = intercept[java.lang.Exception] {
(new ChiselStage)
.execute(
Array("--target", "systemverilog", "--source-root", "src/test/resources/chisel3/sourceroot1"),
Array(
"--target",
"systemverilog",
"--source-root",
s"${sys.env.get("MILL_TEST_RESOURCE_DIR").get}/chisel3/sourceroot1"
),
Seq(ChiselGeneratorAnnotation(() => new ChiselStageSpec.ErrorCaughtByFirtool))
)
}
Expand Down
Loading