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

Question

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

grivera64 opened this issue 3 months ago · 21 comments

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:

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

Answer 1 · 2024-09-28T16:44:03.000Z

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

Answer 2 · 2024-09-28T17:30:24.000Z

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

Answer 3 · 2024-09-30T19:46:45.000Z

aims

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

Answer 4 · 2024-09-30T23:44:31.000Z

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.

Answer 5 · 2024-09-30T23:46:13.000Z

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

Answer 6 · 2024-09-30T23:47:15.000Z

Yes, default is 1 sec.

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

Answer 7 · 2024-09-30T23:48:25.000Z

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

Answer 8 · 2024-10-01T02:19:33.000Z

@grivera64 Do you want to be assigned this issue?

Answer 9 · 2024-10-01T02:37:03.000Z

@grivera64 Do you want to be assigned this issue?

Yes, I can work on this issue.

Answer 10 · 2024-10-01T02:38:00.000Z

@grivera64 Thanks 💪

Answer 11 · 2024-10-01T23:10:27.000Z

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?

Answer 12 · 2024-10-01T23:44:39.000Z

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

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

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

Answer 13 · 2024-10-02T02:38:06.000Z

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

Answer 14 · 2024-10-03T03:27:35.000Z

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

Answer 15 · 2024-10-04T01:42:57.000Z

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

Answer 16 · 2024-10-04T01:56:57.000Z

@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

Answer 17 · 2024-10-04T02:22:31.000Z

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

Answer 18 · 2024-10-04T02:36:52.000Z

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.

Answer 19 · 2024-10-04T12:37:26.000Z

@ReneWerner87 Do you know the answer to the above?

Answer 20 · 2024-10-05T06:54:17.000Z

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.

Answer 21 · 2024-10-08T20:08:56.000Z

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