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

contrib/gin-gonic: add option to ignore request #1061

Merged
merged 7 commits into from
Dec 1, 2021

Conversation

mackjmr
Copy link
Member

@mackjmr mackjmr commented Nov 19, 2021

This PR add's an option to ignore requests for contrib/gin-gonic.

Fixes: #1060

@mackjmr mackjmr requested a review from gbbr November 19, 2021 13:31
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a few nits.

contrib/gin-gonic/gin/gintrace.go Outdated Show resolved Hide resolved
Comment on lines 378 to 387
mt := mocktracer.Start()
defer mt.Stop()

r := httptest.NewRequest("GET", "http://localhost"+test.url, nil)
w := httptest.NewRecorder()

router.ServeHTTP(w, r)

spans := mt.FinishedSpans()
assert.Equal(t, test.spanCount, len(spans))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mt := mocktracer.Start()
defer mt.Stop()
r := httptest.NewRequest("GET", "http://localhost"+test.url, nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, r)
spans := mt.FinishedSpans()
assert.Equal(t, test.spanCount, len(spans))
mt := mocktracer.Start()
defer mt.Reset()
r := httptest.NewRequest("GET", "http://localhost"+test.url, nil)
router.ServeHTTP(httptest.NewRecorder(), r)
assert.Equal(t, test.spanCount, len(mt.FinishedSpans()))

No need to create variables that are not used. Also, we can use Reset instead of Stop, on the mocktracer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, addressed in latest commit.

c.Writer.Write([]byte("Skip"))
})

for _, test := range tests {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer it if we didn't create the tests variable. There is no need for it, it's not used in more than one place. You can just inline all the tests. If you like (up to you), it could also be a map:

Suggested change
for _, test := range tests {
for path, count := range map[string]int{
"/OK": 1,
// ...
} {

Copy link

@prithvihv prithvihv Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can add a field called expectedToSkip bool (to represent a table driven tests).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you can meet both the suggested requirements by doing something like this:

for path, shouldSkip := range map[string]bool{
    "/OK": false,
    // ...
} {
   // ...
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, made the change 👍

contrib/gin-gonic/gin/option.go Outdated Show resolved Hide resolved
mackjmr and others added 3 commits November 22, 2021 17:15
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful! Thank you!

@gbbr gbbr added this to the 1.35.0 milestone Nov 23, 2021
@knusbaum knusbaum merged commit c48db64 into v1 Dec 1, 2021
@knusbaum knusbaum deleted the gin-gonic-ignore-request branch December 1, 2021 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proposal: contrib/gin-gonic: add WithIgnoreRequest
4 participants