Skip to content
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

Cannot test work that inserts jobs using river.ClientFromContext #512

Closed
rgalanakis opened this issue Aug 7, 2024 · 7 comments · Fixed by #514 or #526
Closed

Cannot test work that inserts jobs using river.ClientFromContext #512

rgalanakis opened this issue Aug 7, 2024 · 7 comments · Fixed by #514 or #526
Labels
enhancement New feature or request

Comments

@rgalanakis
Copy link

If I use river.ClientFromContext() within a job, and test it using the Work method (as docs suggest), I cannot insert the client into the context. Could be solved by adding a river.ContextWithClient helper, exposing ctxKey, etc.

@bgentry
Copy link
Contributor

bgentry commented Aug 7, 2024

I actually just encountered this yesterday! Rather than expose a setter for the context value or key, I’m trying to build a test helper that executes a single job. I think this can be a huge improvement for testability.

Are you testing with a job row that you’ve actually inserted, or just a simulated job struct you’ve constructed yourself?

bgentry added a commit that referenced this issue Aug 7, 2024
We were previously using `context.Background()` in some situations to
avoid cancellation of things that really shouldn't be cancelled by the
user context cancellation. However, we can now do that with
`context.WithoutCancel` instead in order to preserve context values
without any cancellation or deadline.

Fixes #512.
bgentry added a commit that referenced this issue Aug 7, 2024
We were previously using `context.Background()` in some situations to
avoid cancellation of things that really shouldn't be cancelled by the
user context cancellation. However, we can now do that with
`context.WithoutCancel` instead in order to preserve context values
without any cancellation or deadline.

Fixes #512.
@bgentry
Copy link
Contributor

bgentry commented Aug 7, 2024

Should be resolved by #514 in the next release, thanks for the report!

@rgalanakis
Copy link
Author

I think your comment refers to #511, not this issue.

Yeah, a test helper sounds great.

I am testing with a simulated worker struct, like:

w := scheduled_sync_worker.New(scheduled_sync_worker.Input{})
Expect(w.Work(ctx, &river.Job[scheduled_sync_worker.Args]{JobRow: &rivertype.JobRow{ID: 5}, Args: scheduled_sync_worker.Args{}})).To(Succeed())

@bgentry
Copy link
Contributor

bgentry commented Aug 7, 2024

D’oh, yes, sorry 😆 I have some details to work out with this test helper but I think that’s gotta be the real answer here.

For a workaround, maybe define a helper function that falls back to extracting the client from an alternate context key you only use in tests. Not ideal though.

@rgalanakis
Copy link
Author

Yup, that is what I ended up doing, not a huge problem but would be nice to avoid critical paths that are different when running unit tests, as you already know :)

@bgentry bgentry added the enhancement New feature or request label Aug 8, 2024
@bgentry bgentry closed this as completed in 21d1c23 Aug 8, 2024
@staugaard
Copy link

I'm pretty sure this was accidentally closed.

@bgentry bgentry reopened this Aug 23, 2024
@bgentry
Copy link
Contributor

bgentry commented Aug 23, 2024

@staugaard you're right, it’s actually #526 which provides a fix for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants