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.

Sign up for GitHub

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

Jump to bottom

Add WithRetryHandler for a custom RetryPolicy #427

Open
acouvreur opened this issue Sep 13, 2023 · 6 comments · May be fixed by #435
Open

Add WithRetryHandler for a custom RetryPolicy #427

acouvreur opened this issue Sep 13, 2023 · 6 comments · May be fixed by #435
Assignees
@rafiramadhana
Labels
feature New feature or request good first issue Good for newcomers help wanted Contributions are welcome

Comments

@acouvreur
Copy link

Context:
My test is as simple as:

e.GET("/nginx").
		WithMaxRetries(10).
		WithRetryDelay(time.Second, time.Second*2).
		WithRetryPolicy(httpexpect.RetryAllErrors).
		Expect().
		Status(http.StatusNotFound).
		Body().Contains(`nginx/`)

I'm expecting a dynamic result from one call to the other. And I'd like to handle the retry policy myself here. Whuch does not seems doable right now.

So something like:

e.GET("/nginx").
		WithMaxRetries(10).
		WithRetryDelay(time.Second, time.Second*2).
		WithRetryPolicy(httpexpect.RetryAllErrors).
		WithRetryHandler(func(resp *http.Response, err error) bool {
			// Check body if available, etc.
			return ...
		})
		Expect().
		Status(http.StatusNotFound).
		Body().Contains(`nginx/`)

What do you think ?
@acouvreur
Copy link
Author

So, I have no tests and all, but this is working for my use case:

acouvreur@4197623

@gavv
Copy link
Owner

gavv commented Oct 2, 2023

Nice idea, pull requests are welcome!

"handler" is probably a bit misleading here, because it suggests that this function handles retry, but it instead decides whether to perform a retry.

Maybe something simpler like WithRetryFunc would fit better.

@gavv gavv added feature New feature or request help wanted Contributions are welcome good first issue Good for newcomers labels Oct 2, 2023
@gavv gavv pinned this issue Oct 3, 2023
@nkzren nkzren mentioned this issue Oct 4, 2023
Closed
@nkzren
Copy link

nkzren commented Oct 4, 2023

So, I have no tests and all, but this is working for my use case:

acouvreur@4197623

@acouvreur I've included a simple test and some docs on top of your commit and @gavv 's suggestion to rename the function (with your co-author on the commit ofc, I hope you don't mind)

@acouvreur
Copy link
Author

So, I have no tests and all, but this is working for my use case:

acouvreur@4197623

@acouvreur I've included a simple test and some docs on top of your commit and @gavv 's suggestion to rename the function (with your co-author on the commit ofc, I hope you don't mind)

Looks great! Thanks!

@gavv gavv unpinned this issue Oct 5, 2023
@rafiramadhana
Copy link
Contributor

@gavv can I work on this?

the previous MR #429 is just closed

@gavv
Copy link
Owner

gavv commented Oct 25, 2023

@rafiramadhana Sure, thanks. I agree with the comment about the test, but didn't have a chance to look at the PR closely yet.

@rafiramadhana rafiramadhana linked a pull request Oct 27, 2023 that will close this issue
Open
@gavv gavv assigned rafiramadhana and unassigned nkzren Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rafiramadhana rafiramadhana

Labels
feature New feature or request good first issue Good for newcomers help wanted Contributions are welcome
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add Request.WithRetryFunc rafiramadhana/httpexpect
feat: add support for custom retry functions nkzren/httpexpect
4 participants
@gavv @acouvreur @nkzren @rafiramadhana