-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
📝 [Proposal]: Add TestConfig to app.Test() for configurable testing #3149
Comments
@grivera64 Couldnt this be a param in |
Adding this as a param to the existing As it is a major update (v2->v3), this could be a permissible breaking change if it would make more sense to keep it as one method in the long run. If it were to be the same method, we could change: func (app *App) Test(req *http.Request, timeout ...time.Duration) (*http.Response, error) to something like: func (app *App) Test(req *http.Request, errOnTimeout bool, timeout ...time.Duration) (*http.Response, error) Or: func (app *App) Test(req *http.Request, config ...*TestConfig) (*http.Response, error) Where Was this what you were referring to @gaby ? |
I think |
@efectn For sure, this will make it cleaner to indicate default vs customized behavior. I think all we need to do is define the default behavior and this is ready to be worked on. I think a good default would be: config := &TestConfig{
Timeout: -1,
ErrOnTimeout: true,
} I will modify the issue and get working on a PR for this. |
@grivera64 Timeout should the same it's now, I believe that's 1 sec? |
Yes, default is |
You are right, my mistake. The default to match current functionality would rather be: config := &TestConfig{
Timeout: time.Second,
ErrOnTimeout: true,
} Thanks for catching me on that, @gaby ! |
@grivera64 Do you want to be assigned this issue? |
Yes, I can work on this issue. |
@grivera64 Thanks 💪 |
After thinking about this default, there may be an issue if future tests were to assume that running the following will provide those defaults from above: config := &TestConfig{} Providing an empty config would result in having the following default values: config := &TestConfig{
Timeout: 0,
ErrOnTimeout: false,
} Would it be OK to specify this technicality in the docs for users, or is this safe to assume that this is a common, expected behavior? |
@grivera64 You can check the length if using "..." tc := TestConfig{
... Default values here
}
if len(TestConfig) > 0 {
tc = TestConfig[0]
} |
@gaby Yes, this would work to use our defaults when no TestConfig is provided. What I meant to ask though was if we should do anything different if var req *http.Request
app.Test(req, TestConfig{}) My concern was that users could incorrectly assume that providing an empty Should we take this into account in the documentation for this change? |
This commit is summarized as: - Add the struct `TestConfig` as a parameter for `app.Test()` instead of `timeout` - Add documentation of `TestConfig` to docs/api/app.md and in-line - Modify middleware to use `TestConfig` instead of the previous implementation Fixes gofiber#3149
@grivera64 I see what you mean, yes it would make sense to add it in the Docs |
This commit is summarized as: - Add the struct `TestConfig` as a parameter for `app.Test()` instead of `timeout` - Add documentation of `TestConfig` to docs/api/app.md and in-line - Modify middleware to use `TestConfig` instead of the previous implementation Fixes gofiber#3149
This commit is summarized as: - Add the struct `TestConfig` as a parameter for `app.Test()` instead of `timeout` - Add documentation of `TestConfig` to docs/api/app.md and in-line - Modify middleware to use `TestConfig` instead of the previous implementation Fixes gofiber#3149
For sure, I will add it as a |
This commit is summarized as: - Add the struct `TestConfig` as a parameter for `app.Test()` instead of `timeout` - Add documentation of `TestConfig` to docs/api/app.md and in-line - Modify middleware to use `TestConfig` instead of the previous implementation Fixes gofiber#3149
@grivera64 Sounds good, looking at your last commit. You can easily align the struct using the Makefile in the repo. Just run |
Thanks for catching that @gaby . I will run the |
I also had a quick implementation question about type testConn struct {
r bytes.Buffer
w bytes.Buffer
} Are the The only current uses of testConn's buffers read these buffers directly: app.Test(). In this use, Is this intentional that both |
@ReneWerner87 Do you know the answer to the above? |
Hi, Yes, this is intentional. The testConn struct is designed to simulate a real network connection where the read ( In the context of app.Test(), the request data is written into the The EOF error you're encountering might be due to how the buffers are being managed. It's important to ensure that the data is correctly written to and read from the appropriate buffers. The separation of the read and write buffers mimics a real network connection's behavior, where reading and writing occur on different data streams. So yes, it's intentional that testConn.Read() reads from the r buffer and testConn.Write() writes to the w buffer. This design allows for accurate simulation of network communication in your tests. |
Thanks @ReneWerner87 for the insight! This makes a lot more sense now:
I am trying to add a unit test for The two ways I can do this are:
In the practical sense, I think the direct reads are fine so I will be using the 1st way from above. (The 2nd way could be a potential future refactor, but it isn't required.) Thanks again for the insight, I will work on the test case for the PR (as well as run |
* 🔥 Feature: Add thread-safe reading from a closed testConn * 🔥 Feature: Add TestConfig to app.Test() This commit is summarized as: - Add the struct `TestConfig` as a parameter for `app.Test()` instead of `timeout` - Add documentation of `TestConfig` to docs/api/app.md and in-line - Modify middleware to use `TestConfig` instead of the previous implementation Fixes #3149 * 📚 Doc: Add more details about TestConfig in docs * 🩹 Fix: Correct testConn tests - Fixes Test_Utils_TestConn_Closed_Write - Fixes missing regular write test * 🎨 Style: Respect linter in Add App Test Config * 🎨 Styles: Update app.go to respect linter * ♻️ Refactor: Rename TestConfig's ErrOnTimeout to FailOnTimeout - Rename TestConfig.ErrOnTimeout to TestConfig.FailOnTimeout - Update documentation to use changed name - Also fix stale documentation about passing Timeout as a single argument * 🩹 Fix: Fix typo in TestConfig struct comment in app.go * ♻️ Refactor: Change app.Test() fail on timeouterror to os.ErrDeadlineExceeded * ♻️ Refactor:Update middleware that use the same TestConfig to use a global variable * 🩹 Fix: Update error from FailOnTimeout to os.ErrDeadlineExceeded in tests * 🩹 Fix: Remove errors import from middlware/proxy/proxy_test.go * 📚 Doc: Add `app.Test()` config changes to docs/whats_new.md * ♻ Refactor: Change app.Test() and all uses to accept 0 as no timeout instead of -1 * 📚 Doc: Add TestConfig option details to docs/whats_new.md * 🎨 Styles: Update docs/whats_new.md to respect markdown-lint * 🎨 Styles: Update docs/whats_new.md to use consistent style for TestConfig options description --------- Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com>
Feature Proposal Description
This issue proposes to add aTestWithInterrupt
method to the App struct for internal testing.This issue proposed to add a
TestConfig
struct as an optional parameter to app.Test() for internal testing.Currently,
app.Test()
allows us to test whether an endpoint completely returns before the given timeout. If it doesn't, any provided reponse is discarded and an error only returns.TestWithInterrupt
aims to provide a way to not discard the response. In most currently available cases, there would be an EOF error (which would be returned byTestWithInterrupt
as an expected behavior).The
TestConfig
struct aims to provide a way to tellapp.Test()
to not discard the response (via the new field "ErrOnTimeout"). It would look like the following:In most currently available cases, there would be an EOF error (which would be returned as an expected behavior).
This method though is useful for buffered streaming that is allowed via fasthttp. This feature is coming to Fiber through the following issue and pull request:
Issue:
#3127
PR:
#3131
Alignment with Express API
N/A as it is a test feature, that is similar to app.Test.
HTTP RFC Standards Compliance
N/A as it is a test feature, that is similar to app.Test.
API Stability
This would be a new method, keeping the current app.Test the same. This will help avoid changes to current tests.
Feature Examples
Checklist:
The text was updated successfully, but these errors were encountered: