Add a test for issue #45; NFC #46
Loading…
x
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 generateautomatically? 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
rushlinkto 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)returnreturnis 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, thencheckRedirectURLmight 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
createTemporaryRushlinkshould 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(`--XXXXXXXXIt is probably neater to create your
multipart/form-dataPOST 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.HandleFunccalls ofStartMainServerinto a separate function that you can also use during testing.createTemporaryRushlinkcan do this setup and return anhttp.Servertoo.Fixed this, though rather than having
createTemporaryRushlinkreturn ahttp.Server, I have it return amux.Routerinstead.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!