Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
🔥 Feature: Add TestConfig to app.Test() for configurable testing #3161
base: main
Are you sure you want to change the base?
🔥 Feature: Add TestConfig to app.Test() for configurable testing #3161
Changes from all commits
00405f7
71c153b
073ec97
228392e
6c006a0
b07e05d
3c4017e
6112c04
4a97d7b
3639a78
4a2a77d
3776184
78e32a7
20bac97
ca3efd2
1220df7
498df60
438b630
0326c07
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use zero value for
Timeout
to disable timeout for clarityUsing
-1
to disable the timeout may be unintuitive for users. In Go, it's conventional to use a zerotime.Duration
to represent the absence of a timeout. Adjusting this will make the API more user-friendly and align with Go standards.Update the comments and logic as follows:
This change enhances readability and aligns with common Go practices regarding timeouts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grivera64 pls check this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ReneWerner87 I believe that using 0 for no timeout would be fine here. I don't believe that users will intentionally want to instant-timeout a test as possible with the current configuration, and making this change will reduce the number of people that make the mistake. As 0 is the default, you could even not include the
Timeout
field to imply no timeout in the test.I can make this change and add it to the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grivera64 if failOnTimeout is false, do we really need a -1 for the timeout? couldn't we do this implicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ReneWerner87
FailOnTimeout
does not directly relate to theTimeout
field, but the naming may mislead users to believe so.When true,
FailOnTimeout
tells the test to return an error after timing out. IfFailOnTimeout
is false, the test will return the most up-to-date response before timing out without returning an error. If there is no timeout (Timeout: -1), then the value ofFailOnTimeout
doesn't affect the test. For clarity, I keptFailOnTimeout
as false since there is no timeout that can lead to a failing test in the first place.Maybe a less misleading name for the field would be something like
IgnoreResponseOnTimeout
or something along those lines.What are your thoughts about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve the example to showcase key features.
The current example could better demonstrate the practical benefits of
TestConfig
, particularly for streaming responses and timeout handling.📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does sound like a much better example to show off, but I think it would be best to add this example use case in this separate PR:
🔥 Feature (v3): Add buffered streaming support #3131
Before doing so, I think we would need to merge these changes first so we can add unit tests using the new
fiber.TestConfig{}
feature from this PR.Please let me know your thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grivera64, that sounds like a great plan. Adding the example in PR #3131 after merging this PR makes sense. Let me know if you need any assistance!
(^.^)