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

[envTest]: How to handle result.RequeueAfter ? #3090

Open
guettli opened this issue Jan 24, 2025 · 4 comments
Open

[envTest]: How to handle result.RequeueAfter ? #3090

guettli opened this issue Jan 24, 2025 · 4 comments

Comments

@guettli
Copy link
Contributor

guettli commented Jan 24, 2025

How to handle result.RequeueAfter in envTests?

I noticed that my envTests fail, when I use result.RequeuAfter instead of Requeue: true.

Do you use a global variable, and the use RequeueAfter: requeueDuration.

Then you could run the envTest with the global variable requeueDuration set to a different value.

Imagine there is a good answer to this question. Where could this answer be documented?

related Slack thread #controller-runtime

@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 28, 2025

I was thinking about this... Unlike Requeue: true, which immediately requeues, RequeueAfter relies on a duration-based requeue, which might not be properly simulated in tests. In an envtest, waiting for RequeueAfter might exceed test timeouts or just make things a bit unreliable.

Maybe a valid approach here could be to inject or override the requeue time to something shorter for the test, something like this:

func TestReconcile(t *testing.T) {
    // Override requeue duration for faster testing
    requeueDuration = 100 * time.Millisecond
    defer func() { requeueDuration = 30 * time.Second }() // Reset after test

    // Setup your test env and reconcile logic
}

Did you try something like that? Curious if this helps or if you're seeing a different issue.

Another approach, though not as clean, could be:

func (r *MyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
    if testMode {
        return ctrl.Result{Requeue: true}, nil
    }
    return ctrl.Result{RequeueAfter: requeueDuration}, nil
}

func TestReconcile(t *testing.T) {
    testMode = true
    defer func() { testMode = false }()
}

However, would it make sense for controller-runtime to default RequeueAfter to time.Millisecond * 100 when running tests, to avoid long waits? I would love it if we could try this approach.

I also noticed in the controller-runtime test code (here) that seems to use fakeClient but I could not find a solution using EnvTest.

@sbueringer
Copy link
Member

sbueringer commented Feb 28, 2025

However, would it make sense for controller-runtime to default RequeueAfter to time.Millisecond * 100 when running tests, to avoid long waits? I would love it if we could try this approach.

Frankly no. I don't think it's a good idea if controller-runtime messes with RequeueAfter in tests. Folks should be able to do that in their test code. It's not super complicated.

@guettli
Copy link
Contributor Author

guettli commented Feb 28, 2025

I agree that the code of controller-runtime needs no update for that.

But maybe it would make sense to document a "best practice". For example storing the defaultRequeueAfter on the ReconcileType of that resource. This avoids a global variable.

But where could that be documented?

@camilamacedo86
Copy link
Member

camilamacedo86 commented Feb 28, 2025

Hi @sbueringer

Thank you for your reply., So, I think the current options are:

And we have no opportunities to improve so far.
So, could we not close this one?

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

No branches or pull requests

3 participants