Skip to content
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

Open
3 tasks done
grivera64 opened this issue Sep 25, 2024 · 21 comments · May be fixed by #3161
Open
3 tasks done

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

grivera64 opened this issue Sep 25, 2024 · 21 comments · May be fixed by #3161

Comments

@grivera64
Copy link
Contributor

grivera64 commented Sep 25, 2024

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.
@gaby
Copy link
Member

gaby commented Sep 28, 2024

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

@grivera64
Copy link
Contributor Author

grivera64 commented Sep 28, 2024

@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
Copy link
Member

efectn commented Sep 30, 2024

aims

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

@grivera64
Copy link
Contributor Author

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
Copy link
Member

gaby commented Sep 30, 2024

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

@gaby
Copy link
Member

gaby commented Sep 30, 2024

Yes, default is 1 sec.

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

@grivera64
Copy link
Contributor Author

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

@grivera64 grivera64 changed the title 📝 [Proposal]: Add TestWithInterrupt to App 📝 [Proposal]: Add TestConfig to app.Test() for configurable testing Sep 30, 2024
@gaby
Copy link
Member

gaby commented Oct 1, 2024

@grivera64 Do you want to be assigned this issue?

@gaby gaby added this to the v3 milestone Oct 1, 2024
@grivera64
Copy link
Contributor Author

@grivera64 Do you want to be assigned this issue?

Yes, I can work on this issue.

@gaby
Copy link
Member

gaby commented Oct 1, 2024

@grivera64 Thanks 💪

@grivera64
Copy link
Contributor Author

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
Copy link
Member

gaby commented Oct 1, 2024

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

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

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

@grivera64
Copy link
Contributor Author

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

grivera64 added a commit to grivera64/fiber that referenced this issue Oct 2, 2024
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
@gaby
Copy link
Member

gaby commented Oct 3, 2024

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

grivera64 added a commit to grivera64/fiber that referenced this issue Oct 3, 2024
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 added a commit to grivera64/fiber that referenced this issue Oct 3, 2024
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
Copy link
Contributor Author

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

grivera64 added a commit to grivera64/fiber that referenced this issue Oct 4, 2024
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
@gaby
Copy link
Member

gaby commented Oct 4, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

grivera64 commented Oct 4, 2024

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
Copy link
Member

gaby commented Oct 4, 2024

@ReneWerner87 Do you know the answer to the above?

@ReneWerner87
Copy link
Member

ReneWerner87 commented Oct 5, 2024

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
Copy link
Contributor Author

grivera64 commented Oct 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

4 participants