📝 [Proposal]: Add TestConfig to app.Test() for configurable testing

[grivera64]

Feature Proposal Description

~This issue proposes to add a TestWithInterrupt 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 by TestWithInterrupt as an expected behavior).~ The TestConfig struct aims to provide a way to tell app.Test() to not discard the response (via the new field "ErrOnTimeout"). It would look like the following:

type TestConfig struct {
    Timeout      time.Duration
    ErrOnTimeout bool
}

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

app := New()
app.Get("/", func(c Ctx) error {
    time.Sleep(1 * time.Second)
    return c.SendString("Should never be called")
})

respBody, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), &TestConfig{
    Timeout: 100*time.Millisecond,
    ErrOnTimeout: false,  // If false, do not discard response
})

Checklist:

• [X] I agree to follow Fiber's Code of Conduct.
• [X] I have searched for existing issues that describe my proposal before opening this one.
• [X] I understand that a proposal that does not meet these guidelines may be closed without explanation.

[gaby]

@grivera64 Couldnt this be a param in app.Test() ?

[grivera64]

@grivera64 Couldnt this be a param in app.Test() ?

Adding this as a param to the existing app.Test() method would require refactoring previous uses of it. To avoid this, I had proposed to make a separate method for it.

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 TestConfig is a structure that has the errOnTimeout and timeout parameters as public fields.

Was this what you were referring to @gaby ?

[efectn]

aims

I think func (app *App) Test(req *http.Request, config ...*TestConfig) (*http.Response, error) is OK

[grivera64]

aims

I think func (app *App) Test(req *http.Request, config ...*TestConfig) (*http.Response, error) is OK

@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.

[gaby]

@grivera64 Timeout should the same it's now, I believe that's 1 sec?

[gaby]

Yes, default is 1 sec.

https://github.com/gofiber/fiber/blob/main/app.go#L871

[grivera64]

@grivera64 Timeout should the same it's now, I believe that's 1 sec?

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 !

[gaby]

@grivera64 Do you want to be assigned this issue?

[grivera64]

@grivera64 Do you want to be assigned this issue?

Yes, I can work on this issue.

[gaby]

@grivera64 Thanks 💪

[grivera64]

The default to match current functionality would rather be:

config := &TestConfig{
    Timeout: time.Second,
    ErrOnTimeout: true,
}

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?

[gaby]

@grivera64 You can check the length if using "..."

tc := TestConfig{
   ... Default values here
}

if len(TestConfig) > 0 {
    tc = TestConfig[0]
}

[grivera64]

@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 app.Test() is called with an explicitly empty TestConfig:

var req *http.Request
app.Test(req, TestConfig{})

My concern was that users could incorrectly assume that providing an empty TestConfig{} themselves would be the equivalent of the default behavior.

Should we take this into account in the documentation for this change?

[gaby]

@grivera64 I see what you mean, yes it would make sense to add it in the Docs

[grivera64]

@grivera64 I see what you mean, yes it would make sense to add it in the Docs

For sure, I will add it as a :::caution ::: message in the updated documentation and make it into a PR.

[gaby]

@grivera64 Sounds good, looking at your last commit. You can easily align the struct using the Makefile in the repo. Just run make betteralign. See here https://github.com/gofiber/fiber/blob/main/Makefile#L57

[grivera64]

@grivera64 Sounds good, looking at your last commit. You can easily align the struct using the Makefile in the repo. Just run make betteralign. See here https://github.com/gofiber/fiber/blob/main/Makefile#L57

Thanks for catching that @gaby . I will run the make commands to make sure that formatting is ready to go before making the PR.

[grivera64]

I also had a quick implementation question about testConn:

type testConn struct {
  r bytes.Buffer 
  w bytes.Buffer
}

Are the r and w buffers linked? I had assumed so, but it seems that there is an issue with using testConn.Read() and testConn.Write() for testing that gives an EOF error.

The only current uses of testConn's buffers read these buffers directly: app.Test(). In this use, testConn's r buffer is used for writing, while w buffer is used for reading.

Is this intentional that both testConn.Read() and testConn.w.Read() are used this way? Thanks in advance for the insight.