-
Notifications
You must be signed in to change notification settings - Fork 597
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
Add FileCheck for unit testing #3410
base: main
Are you sure you want to change the base?
Conversation
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 looks amazing. The tests can really be bulletproof now.
Two thoughts:
- Is there any way to avoid needing to package FileCheck ourselves?
- This follows a pattern of FileCheck usage where the checks are all after the code. I tend to use this frequently. However, I will sometimes also write in-line tests. Is there a way that we could support this as well, e.g., something like:
val fileCheck = new FileCheck()
class Foo extends RawModule {
fileCheck("CHECK-LABEL: module Foo")
val a = IO(Input(Bool()))
fileCheck("CHECK-NEXT: input a: UInt<1>")
}
fileCheck.check(ChiselStage.emitCHIRRTL(new Foo))
This can be handled in a follow-on.
|# Check for back-pressure (ready signal is driven in the opposite direction of bits + valid) | ||
|CHECK: connect enq.ready, [[result]].ready | ||
|CHECK: connect deq, [[result]] | ||
|""".stripMargin) |
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 sooooooooo much better. 💯
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.
can we add something to the readme before this is merged with the link to primer/docs for those not familiar with file check syntax?
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.
@mwachs5 I added instructions on how to install and a very brief description of FileCheck and where to look for more information. How much of a tutorial do you think we need to include?
Just wonder if possible to share the cases among firrtl, verilog. |
FileCheck has support for checking multiple prefixes which is commonly used to share across separate invocations of some test run. This may work for sharing between firrtl and verilog, but is unlikely. |
a70ddff
to
cf448e6
Compare
d6a23b4
to
e5c9813
Compare
/** Elaborate a Module to FIRRTL and check the FIRRTL with FileCheck */ | ||
def generateFirrtlAndFileCheck(t: => RawModule, fileCheckArgs: String*)(check: String): Unit = { | ||
// Filecheck needs the thing to check in a file | ||
os.write.over(checkFile.get, check) | ||
val result = ChiselStage.emitCHIRRTL(t) | ||
val extraArgs = os.Shellable(fileCheckArgs) | ||
os.proc("FileCheck", checkFile.get, extraArgs).call(stdin = result) | ||
} |
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.
For this and the other method, just a t: => String
is probably more useful as I can see wanting to use this to check SystemVerilog emission. It isn't too onerous to have a user do fileCheck(ChiselStage.emitCHIRRTL(new Foo))
.
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.
Capturing stdout and stderr is kind of annoying though, maybe I'll provide a 3rd method that just takes a String, and have the other 2 call it?
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.
That works!
Update SETUP.md including brief instructions on FileCheck.
e5c9813
to
26c49da
Compare
Draft to see if the process works end-to-end
Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.5.x
,3.6.x
, or5.x
depending on impact, API modification or big change:6.0
)?Enable auto-merge (squash)
, clean up the commit message, and label withPlease Merge
.Create a merge commit
.