Various linter updates #2469
Various linter updates #2469
Conversation
leonklingele
force-pushed
the
lint-updates
branch
from
73ec8e9
to
b8bbdd4
Compare
leonklingele
force-pushed
the
lint-updates
branch
from
50af4d2
to
34fd034
Compare
| func (app *App) Test(req *http.Request, msTimeout ...int) (*http.Response, error) { | ||
| // Set timeout | ||
| timeout := 1000 | ||
| timeout := int((10 * time.Second) / time.Millisecond) |
There was a problem hiding this comment.
this would be a breaking change
There was a problem hiding this comment.
True. I don't really know how to avoid it though. As most of the tests run concurrently now, individual tests might take longer than the previous 1 second defined here due to the Go scheduler rescheduling execution between tests.
What do you think of adding and using a new app.TestWithDynamicTimeout(req *http.Request, msTimeout ...int) which defaults this to a timeout of 10s as of now, and might be further increased in the future?
There was a problem hiding this comment.
@ReneWerner87 what do you recommend to do about this?
|
@leonklingele can you answer the other questions from the review |
|
leon is not available for 2 weeks, lets wait with this until he is available |
|
hey @leonklingele any updates on this? |
|
@leonklingele ping |
|
Totally forgot about this! Will get to it some time next week.
|
ok thx |
leonklingele
force-pushed
the
lint-updates
branch
3 times, most recently
from
0026bd0
to
daaee0a
Compare
|
@ReneWerner87 Updated & rebased. PTAL :) |
| QueryParam string `query:"query_param"` | ||
| HeaderParam string `reqHeader:"header_param"` | ||
| BodyParam string `json:"body_param" xml:"body_param" form:"body_param"` | ||
| QueryParam string ` query:"query_param"` |
There was a problem hiding this comment.
Actually, it's done intentionally so one can better differentiate the various tags being used.
There was a problem hiding this comment.
But the line width will increase. I'd prefer old layout
| password := c.Locals("password").(string) | ||
| username, ok := c.Locals("username").(string) | ||
| if !ok { | ||
| t.Fatal("missing username") |
There was a problem hiding this comment.
Should be part of another MR.
There was a problem hiding this comment.
Should be part of another MR.
It can be done in this PR since it was added here
There was a problem hiding this comment.
Sorry for the delay. How would you use assertequal here? We want to retrieve the user + pass from locals.
There was a problem hiding this comment.
Sorry for the delay. How would you use assertequal here? We want to retrieve the user + pass from locals.
utils.AssertEqual(t, true, ok)
leonklingele
force-pushed
the
lint-updates
branch
from
4c4d347
to
3336bf1
Compare
This makes it much easier for us to spot data races, which otherwise would simply be ignored when not being detected in a retry-run.
|
@leonklingele hi can you look again, last time you were unfortunately not quite finished with the merges and now some places have been added maybe we can also split the parts into individual pull requests so that we can merge them in a timely manner think some customizations from you from these pull requests would be very good for our code |
|
Closed due to conflicts and because v2 will not receive any further customizations |
See the individual commit messages for details.
A data race in fasthttp was identified using the stricter config. See valyala/fasthttp#1565.