-
Notifications
You must be signed in to change notification settings - Fork 50
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
assert: ensure message is always displayed & fix under bazel #276
Conversation
All tests run via
I'll see if I can fix the lint issue. |
Hmm. The
but that test passes with [email protected] on my machine (running macOS). Maybe a flaky test? Might bump CI with an empty commit. |
Yay! The tests pass. But I just realized that my editor was hiding the fact that tabs and spaces got mixed together in that big error message string. Just pushed a commit to fix that up. |
And move logic to a function so that variables can be closer to where they are used
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.
Thank you for the PR! I made a couple small changes, but overall this looks great.
Primarily I moved the variables to a bazel.go
file, and extracted some of the logic to a new function. This way the variables and the code can be located a bit closer together.
I also extracted the multi-line error message to a raw string to make it a bit easier to read in code. The raw string removes the need to escape the double quotes.
If these changes look alright to you I think this is ready to be merged.
name = "your_package_test", | ||
srcs = ["your_test.go"], | ||
deps = ["@tools_gotest_v3//assert"], | ||
data = glob(["*_test.go"]) |
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.
On this line I added the trailing )
which I believe was missing, and removed the "test source files added as test data here!" comment. Does that seem ok? I guess the glob should capture all the test files.
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.
Thanks! Yes, I think this is great 👍
@dnephin Those changes look great! Don't see anything else I'd change. Thanks! |
Note that I still need to run the tests, but I wanted to get this PR open ASAP.
This PR fixes #274.
Now, when running under
bazel test
we'll see the following sort of error message when the target test source file can't be found:If the user adds the test source file to their test data in the respective
go_test
target, they'll now see something like: