-
Notifications
You must be signed in to change notification settings - Fork 623
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.
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
base: main
Are you sure you want to change the base?
Conversation
d5476f5
to
99d4456
Compare
Signed-off-by: unlsycn <[email protected]>
Hi, I’d appreciate it if anyone could take a look at this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks!
One question if -I
(or long version: --include-dir
) can be used to keep a test working.
thrown.getMessage must include("gcd.io.result.expect(5)") | ||
thrown.getMessage must include(" ^") |
There was a problem hiding this comment.
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}")
?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
ExceptionHelpers.getErrorLineInFile(Seq(), sl) |
simulate
and keep it in simulation time.
com-lihaoyi/mill#3347 enables the sandbox for testing by default, and this prevents us from assuming that we are running tests in the workspace root. Instead, we should use the environment variables provided by mill to locate the resources that files in tests should access.
Notably, a match assertion of sourceline and caret has been removed from SimulatorSpec, as we don't have an option to specify the source root of this test, it wouldn't undermine the correctness of the test.
Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x
,5.x
, or6.x
depending on impact, API modification or big change:7.0
)?Enable auto-merge (squash)
and clean up the commit message.Create a merge commit
.