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

fix(engine): missing route params for CreateTestContext (#2778) #2803

Merged
merged 1 commit into from
Nov 6, 2022

Conversation

RoCry
Copy link
Contributor

@RoCry RoCry commented Aug 3, 2021

This PR trying to fix the issue we met, which exact same with #2778

If you guys have better idea to fix this, just let me know.

@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #2803 (621fc9d) into master (b57163a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2803   +/-   ##
=======================================
  Coverage   98.37%   98.38%           
=======================================
  Files          43       43           
  Lines        3148     3153    +5     
=======================================
+ Hits         3097     3102    +5     
  Misses         38       38           
  Partials       13       13           
Flag Coverage Δ
go-1.15 98.38% <100.00%> (+<0.01%) ⬆️
go-1.16 98.35% <100.00%> (+<0.01%) ⬆️
go-1.17 98.28% <100.00%> (+<0.01%) ⬆️
go-1.18 98.28% <100.00%> (+<0.01%) ⬆️
macos-latest 98.38% <100.00%> (+<0.01%) ⬆️
nomsgpack 98.35% <100.00%> (+<0.01%) ⬆️
ubuntu-latest 98.38% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gin.go 99.18% <100.00%> (ø)
test_helpers.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b57163a...621fc9d. Read the comment docs.

@shrike42
Copy link

shrike42 commented Oct 1, 2021

Had the same problem when trying to follow the examples.

The fix you outline would work but I think at that point especially with the need to create a separate engine to get the valid maxParam value one could just use a different approach using ServeHTTP and forget CreateTestContext.

Tests could be written as follows based on your example:

w := httptest.NewRecorder()
engine := New()

// At this point add the route(s) to test
engine.GET("/:action/:name", func(ctx *Context) {
ctx.String(http.StatusOK, "%s %s", ctx.Param("action"), ctx.Param("name"))
})
r = http.NewRequest(http.MethodGet, "/hello/gin", nil)

engine.ServeHTTP(w,r)

assert.Equal(t, http.StatusOK, w.Code)
assert.Equal(t, "hello gin", w.Body.String())

Admittedly this does not use the test context at all but I wonder if using CreateTestContext is even a good idea one could say that ServeHTTP is a better approach. Like the context method this does not start the engine.

@appleboy appleboy added the bug label Oct 3, 2021
@appleboy appleboy added this to the v1.8 milestone Oct 3, 2021
appleboy
appleboy previously approved these changes Oct 3, 2021
@appleboy
Copy link
Member

appleboy commented Oct 3, 2021

@RoCry Please fix the conflicts.

@nmfzone
Copy link

nmfzone commented Jul 14, 2022

Is there any plan to release this soon?

Or is there any workaround?

@RoCry RoCry force-pushed the master branch 2 times, most recently from 20fb1b8 to 76b015a Compare July 14, 2022 14:41
@appleboy appleboy changed the title fix missing route params for CreateTestContext (#2778) fix(engine): missing route params for CreateTestContext (#2778) Nov 6, 2022
@appleboy appleboy merged commit 55e27f1 into gin-gonic:master Nov 6, 2022
@appleboy appleboy modified the milestones: v1.9, v1.8.2 Nov 23, 2022
@appleboy
Copy link
Member

we will release the feature in v1.8.2

@appleboy
Copy link
Member

thxCode pushed a commit to seal-io/gin that referenced this pull request Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants