📝 [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.

[gaby]

@ReneWerner87 Do you know the answer to the above?

[ReneWerner87]

Hi,

Yes, this is intentional. The testConn struct is designed to simulate a real network connection where the read (r) and write (w) buffers are separate and independent. The r buffer represents the incoming data stream (the request), and the w buffer represents the outgoing data stream (the response).

In the context of app.Test(), the request data is written into the r buffer. The server reads from this buffer using testConn.Read() to process the incoming request. Conversely, the server writes the response data using testConn.Write(), which appends to the w buffer. You can then read the response from testConn.w.

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.

[grivera64]

Thanks @ReneWerner87 for the insight! This makes a lot more sense now:

r is the request buffer (Read by the connection to know the request), while w is the response buffer (Written to by the connection to reply as a response).

(Side note: What are your thoughts on updating the names of r and wto req and res respectively? This could help with future modifications to testConn by the community, but it is also closer to the net/http interface.)

I am trying to add a unit test for testConn to make sure it is working properly since I made a few modifications on the PR I am working on. WIth this context, I should just verify that Read() and Write()are writing to the appropriate buffers.

The two ways I can do this are:

• Directly read from both buffers (conn.r and conn.w)
• or Create a method to read these

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 make betteralign as mentioned before).