Add a test for issue #45; NFC #46
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "test-issue-45"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR adds a test for issue #45. It is the first test that I wrote with
httptest
. Can any of you maybe quickly review it?Hmm, I find it less than ideal that we have to run
go generate ./...
, otherwise this happens:Hmmm. Is there a way to do
go generate
automatically? If not, should we maybe just do it as part of the test?@ -0,0 +50,4 @@
t.Errorf("handler returned wrong status code: got %v want %v\n",
actual, expected)
t.Logf("request body:\n%v\n", rr.Body.String())
t.FailNow()
I would suggest:
@ -0,0 +55,4 @@
}
func TestIssue43(t *testing.T) {
wrapRushlinkTest(t, func(rl rushlink) {
I don't think the wrapper function is useful nor does it improve legibility. I would suggest putting all test code inside
TestIssue43
, or do something like:If it's necessary for
rushlink
to run, then we might better include it in the repository. It would then be up to the committer to provide the latest generated file.@ -0,0 +78,4 @@
t.Fatal(err)
return
}
t.Logf("URL shortened to: %s\n", pasteURL.Path[1:])
That is nice to read, but if you log this, then perhaps also do
t.Logf("URL redirects to: %q", redirect")
afterredirect := rr.Header().Get("Location")
?@ -0,0 +76,4 @@
pasteURL, err := url.Parse(rawurl)
if err != nil {
t.Fatal(err)
return
return
is dead code.@ -0,0 +90,4 @@
handler = http.HandlerFunc(rl.viewPasteHandler)
handler.ServeHTTP(rr, req)
checkStatusCode(t, rr, http.StatusTemporaryRedirect)
redirect := rr.Header().Get("Location")
If you insert functions like
checkStatusCode
, thencheckRedirectURL
might be an addition for legibility as well.@ -0,0 +14,4 @@
"github.com/gorilla/mux"
)
// initRushlinkTest initializes a rushlink instance, with temporary filestore
(the comment is outdated)
@ -0,0 +91,4 @@
handler := http.HandlerFunc(rl.newPasteHandler)
handler.ServeHTTP(rr, req)
checkStatusCode(t, rr, http.StatusOK)
rawurl := strings.SplitN(rr.Body.String(), "\n", 2)[0]
(style:
rawURL
, similar to the line below)Ack.
@ -0,0 +73,4 @@
func TestIssue43(t *testing.T) {
rl, tempDir := createTemporaryRushlink(t)
t.Cleanup(func() { destroyTemporaryRushlink(&rl, tempDir) })
Hmm, maybe
createTemporaryRushlink
should register its own cleanup ont
?Good point. :)
@ -0,0 +76,4 @@
t.Cleanup(func() { destroyTemporaryRushlink(&rl, tempDir) })
// Put a URL with a fragment identifier into the database.
body := strings.NewReader(`--XXXXXXXX
It is probably neater to create your
multipart/form-data
POST request using https://golang.org/pkg/mime/multipart/#Writer.CreateFormFieldCheck. Agreed.
@ -0,0 +88,4 @@
}
req.Header.Add("Content-Type", "multipart/form-data; boundary=XXXXXXXX")
rr := httptest.NewRecorder()
handler := http.HandlerFunc(rl.newPasteHandler)
This part of the test bypasses the code in
router.go
, which is a risk for different behaviour between the test and the real application.Since a lot of tests are going to want to send an HTTP request, I think it it wise to factor out the
router.HandleFunc
calls ofStartMainServer
into a separate function that you can also use during testing.createTemporaryRushlink
can do this setup and return anhttp.Server
too.Fixed this, though rather than having
createTemporaryRushlink
return ahttp.Server
, I have it return amux.Router
instead.This allows us to ignore connection-level stuff (like timeouts etc.) but it enabled testing the router.
I updated the patch according to the new reviews and merged the PR. :)
Thanks @mrngm and @minnozz!