🐛 Panic instead of Exit in FakeClient

[grantr]

User code may use this fake client in tests without setting up logging, making errors here extremely difficult to track down. Panic logs more reliably with better debugging details.

I didn't change any of the other os.Exit(1) calls, since they're in CR's own tests. I'd love to know why the tests don't use panic generally. Is it to eliminate the option to recover?

[grantr]

/label 🐛

[grantr]

(No idea how this label thing is supposed to work)

[DirectXMan12]

Your title is correct. LMK if I can make the wording clearer, or just submit a PR ;-).

We can also
/kind bug

but the PR title is the important one here.

[DirectXMan12]

Hmm... I think this is probably fine. We set up all of our loggers to log to the GinkgoSetup in our tests, but you do actually have to do that manually. Honestly, I'd prefer a bit to make it easier to just set up logging for tests, since you should always do that (you either want to have Zap dump to GinkgoWriter, like we do in CR, or point the loggers at a logr.TestLogger if you're using the Go standard libary).

[DirectXMan12]

Panic logs more reliably with better debugging details.

log.Error logs a backtrace as well, so debuggability shouldn't be harmed. log.Error is favored by itself, since if you set it up right, it'll be captured by the per-test output stuff.

log.Error with os.Exit(1) is just the replacement for calls to log.Fatalf, which doesn't exist in logr (somewhat intentionally). log.Fatalf basically just calls log.Printf followed by os.Exit(1).

[grantr]

LMK if I can make the wording clearer

It's clear, I was just surprised by the appearance of :bug: in the title and assumed it was not the intended result.

log.Error logs a backtrace as well, so debuggability shouldn't be harmed.

Great!

Honestly, I'd prefer a bit to make it easier to just set up logging for tests, since you should always do that

knative/eventing uses logr to set up the manager's internal logging in the main entry point. All other logging is plain Zap, testing is stdlib, and tests don't involve the manager at all. It's fine for the CR tests to be opinionated about how testing or logging works, but the fake client is in a different category IMO. It's support code needed by every user of the real client, and thus should have the same level of pluggability as the real client.

[grantr]

which doesn't exist in logr (somewhat intentionally)

I totally get why this is missing from logr but it is a little painful to not have the log.Fatal oneliner! 😄

[grantr]

(Having said all the above, I'd like to see what changes you'd recommend to make it easier to set up logging for tests 😺)

[DirectXMan12]

honestly, the panic is probably fine here :-). I'm tempted to just merge this.

It's support code needed by every user of the real client, and thus should have the same level of pluggability as the real client.

Sure. The idea is that logr is that plugability -- if you're using stdlib, you can just do log.SetLogger(logr.TestLogger(t)) to get logging via the stdlib t.Log implementation (no backtraces, though :-/).

Having said all the above, I'd like to see what changes you'd recommend to make it easier to set up logging for tests

If you're using Ginkgo, and want to get the full zap output with traces, you can do logf.SetLogger(logf.ZapLoggerTo(GinkgoWriter, true)) in your BeforeSuite like we do, but typing that out every time is a pain. It would be nice to just be able to call log.LogForGinkgo() and be done with it. On top of that, the contents of the CR BeforeSuite method doesn't actually vary much and is pretty general, so it would be nice if you could just say BeforeSuite(&TestEnv{}.Standup).

[DirectXMan12]

Btw, looks like you've got some vet errors. If you fix them we can merge this.

[grantr]

if you're using stdlib, you can just do log.SetLogger(logr.TestLogger(t)) to get logging via the stdlib t.Log implementation

Nice, that's useful to know about!

Fixed the vet error.

[Harwayne]

I hit the same issue and had a very hard time debugging it, as I couldn't find any flags to get go test -v to give any additional information about why it was exiting. I only found the cause after @grantr pointed me to this PR.

[grantr]

Fixes #354

@anthonyho007 independently created #373 to fix this.

[grantr]

Rebased to latest master. This passes tests locally so I'm not sure why it's failing on Travis.

[grantr]

This passes tests locally so I'm not sure why it's failing on Travis.

Passed this time 😄 Friendly ping @DirectXMan12.

[DirectXMan12]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, grantr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment