Skip to content

rivertest.Worker fixes: handle nil config, rename Kind/EventKind, log panic trace#775

Merged
bgentry merged 4 commits intomasterfrom
bg-fix-testworker-nil-config
Feb 19, 2025
Merged

rivertest.Worker fixes: handle nil config, rename Kind/EventKind, log panic trace#775
bgentry merged 4 commits intomasterfrom
bg-fix-testworker-nil-config

Conversation

@bgentry
Copy link
Copy Markdown
Contributor

@bgentry bgentry commented Feb 19, 2025

I wrote some tests for the demo app using the test worker (#766) and found a few issues:

  1. It panics with a nil *river.Config. I fixed this by making Config.WithDefaults handle a nil pointer so no change was necessary in the rivertest.NewWorker caller.
  2. The Kind field in rivertest.WorkResult felt unnatural as I was writing assertions for it like require.Equal(t, river.EventKindJobCompleted, result.Kind). I renamed this to EventKind which I still don't love but it feels at least a little more appropriate to me.
  3. When I accidentally made my worker panic during the course of developing tests, I realized the current output (just the panic value) was not very helpful for debugging. I adjusted this to also log the full trace, which is also how I realized Remove irrelevant frames from panic traces #774 was necessary.

Side note on (3) above: I realized I couldn't actually use rivertest.Worker to write a test for a worker that intentionally panics, because its panic handler calls tb.Fatal. This is probably not a very likely issue for real users, and it's probably a good default behavior, but I won't be surprised if we feel compelled to add an option to the test worker type so that it avoid calling fatal and instead surface an error or call tb.Error. Wanted to run it by you in case you had other thoughts on this. For example, we could just switch it to tb.Error because Fatal is quite presumptive?

@bgentry bgentry requested a review from brandur February 19, 2025 03:19
@brandur
Copy link
Copy Markdown
Contributor

brandur commented Feb 19, 2025

Wanted to run it by you in case you had other thoughts on this. For example, we could just switch it to tb.Error because Fatal is quite presumptive?

Yeah, honestly I didn't see this part on my review, but IMO the panic shouldn't be swallowed. Might even be best not to have any error/panic handlers and just let users do what they want with those values.

Comment thread rivertest/worker_test.go Outdated
}
}

t.Run("HandlesANilRiverConfig", func(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, but found the casing a bit awkward to read. Really doesn't need the "A" anyway:

Suggested change
t.Run("HandlesANilRiverConfig", func(t *testing.T) {
t.Run("HandlesNilRiverConfig", func(t *testing.T) {

@bgentry bgentry force-pushed the bg-fix-testworker-nil-config branch from 7947a62 to 2be891e Compare February 19, 2025 16:18
@bgentry
Copy link
Copy Markdown
Contributor Author

bgentry commented Feb 19, 2025

Yeah, honestly I didn't see this part on my review, but IMO the panic shouldn't be swallowed. Might even be best not to have any error/panic handlers and just let users do what they want with those values.

An error handler is necessary in order to capture and return the error via the test helper—otherwise it'd be lost and only recorded in the DB.

I've restructured this so there's a PanicError type that gets surfaced as the return error from Work / WorkJob. We're no longer using tb.Fatal when working a test job except for a couple of internal error cases that shouldn't ever happen. Lmk if you have any issues with how I've done this and we can fix in a follow-up PR before shipping this (hopefully later today) 🙏 :shipit:

If a panic occurs, return a `PanicError` struct containing the trace and
panic cause. Do not use `tb.Fatalf` from within execution except for
unexpected internal issues—instead propagate errors to the user.

Also change the return type to `*WorkResult` since it may be optional
when an internal error occurs.
@bgentry bgentry force-pushed the bg-fix-testworker-nil-config branch from 2be891e to 21a24d2 Compare February 19, 2025 16:19
@bgentry bgentry enabled auto-merge (squash) February 19, 2025 16:23
@bgentry bgentry merged commit 8e66dd5 into master Feb 19, 2025
@bgentry bgentry deleted the bg-fix-testworker-nil-config branch February 19, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants