-
Notifications
You must be signed in to change notification settings - Fork 35
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 Regal bundle to test cmd runner #197
Add Regal bundle to test cmd runner #197
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.
👏 What a nice first contribution. Thanks a lot!
t.Parallel() | ||
|
||
stdout := bytes.Buffer{} | ||
stderr := bytes.Buffer{} |
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.
Not for this PR, just a random thought -- I wonder if something like this would be useful for testing:
type panicWriter struct{}
func (*panicWriter) Write([]byte) (int, error) {
panic("write unexpected")
}
It's like io.Discard
just with a little more drama. https://play.golang.com/p/xCQlGZH2nUq
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.
It would certainly reduce boilerplate in the test cases, at the cost of clarity in the error messages when the tests do fail.
In the current state we get "expected stderr %q, got %q", which I would appreciate to see myself if I did some changes that broke a test. This error message would likely point me in the right direction immediately.
Using this proposed construct, the test would only report "write unexpected" without any context. In this latter case, I would likely then start a debug session or add print calls to get going.
The importance of boilerplate vs error messages and preferred working style of course varies between developers, so this is only my personal thoughts on 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.
Yeah you've made a good point there. Perhaps something that takes a *testing.T
and fails accordingly would strike a middle ground... maybe some day 🙃
This looks great! I’m on mobile but do I see it right that no checks are running for this PR? I wonder why that might be. I don’t see an option to “approve” of them either. @srenatus you know? |
@anderseknert The build workflow's "on" is wonky, I think: on:
push:
branches:
- '**'
workflow_dispatch: This only matches branches on this repository and will thus not run for PRs from forks. OPA uses that on: [pull_request] Also, we don't have protection rules defined, it seems, so the checks aren't required by github either. Hence the PR got green without any tests run. |
Also add e2e test to validate that the bundle is added correctly. Signed-off-by: Kristian Svalland <kristian.svalland@gmail.com>
c061c2d
to
e689e5b
Compare
Signed-off-by: Kristian Svalland <kristian.svalland@gmail.com>
Thanks Kristian! |
* Add Regal bundle to test cmd runner Also add e2e test to validate that the bundle is added correctly. Signed-off-by: Kristian Svalland <kristian.svalland@gmail.com>
Also add e2e test to validate that the bundle is added correctly.
Fixes #196