Add a test for issue #45; NFC #46

Merged
electricdusk merged 1 commits from test-issue-45 into master 2020-04-27 13:00:34 +02:00
Owner

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?

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?
minnozz was assigned by electricdusk 2020-04-22 18:29:22 +02:00
mrngm was assigned by electricdusk 2020-04-22 18:29:22 +02:00
Collaborator

Hmm, I find it less than ideal that we have to run go generate ./..., otherwise this happens:

[ mrngm@kaakje ~/scm/rushlink[test-issue-45*] ]
$ go test ./...
2020/04/22 18:40:29 migrating database to version 1
2020/04/22 18:40:29 migrating database to version 2
2020/04/22 18:40:29 migrating database to version 3
--- FAIL: TestIssue43 (0.00s)
panic: template: :10:26: executing "" at <.CanDelete.String>: can't evaluate field String in type interface {} [recovered]
	panic: template: :10:26: executing "" at <.CanDelete.String>: can't evaluate field String in type interface {}

goroutine 39 [running]:
testing.tRunner.func1.1(0x8cb000, 0xc00018d120)
	/usr/lib/go/src/testing/testing.go:940 +0x2f5
testing.tRunner.func1(0xc0001a25a0)
	/usr/lib/go/src/testing/testing.go:943 +0x3f9
panic(0x8cb000, 0xc00018d120)
	/usr/lib/go/src/runtime/panic.go:969 +0x166
gitea.hashru.nl/dsprenkels/rushlink.(*rushlink).render(0xc00018cbe0, 0x9c3ea0, 0xc0001a1700, 0xc00019b100, 0x914fe2, 0x9, 0xc0001fb920)
	/home/mrngm/scm/rushlink/views.go:154 +0xa41
gitea.hashru.nl/dsprenkels/rushlink.(*rushlink).viewCreateSuccess(0xc00018cbe0, 0x9c3ea0, 0xc0001a1700, 0xc00019b100, 0xc000180ba0, 0x0)
	/home/mrngm/scm/rushlink/handlers.go:207 +0x223
gitea.hashru.nl/dsprenkels/rushlink.(*rushlink).newRedirectPasteHandler(0xc00018cbe0, 0x9c3ea0, 0xc0001a1700, 0xc00019b100, 0xc0001b6440, 0x1c)
	/home/mrngm/scm/rushlink/handlers.go:291 +0x29b
gitea.hashru.nl/dsprenkels/rushlink.(*rushlink).newPasteHandler(0xc00018cbe0, 0x9c3ea0, 0xc0001a1700, 0xc00019b100)
	/home/mrngm/scm/rushlink/handlers.go:226 +0x20b
net/http.HandlerFunc.ServeHTTP(...)
	/usr/lib/go/src/net/http/server.go:2012
gitea.hashru.nl/dsprenkels/rushlink.TestIssue43.func1(0xc00018e108, 0xc0001927d0, 0xc0001d8400)
	/home/mrngm/scm/rushlink/handlers_test.go:73 +0x2ef
gitea.hashru.nl/dsprenkels/rushlink.wrapRushlinkTest(0xc0001a25a0, 0xc000147f60)
	/home/mrngm/scm/rushlink/handlers_test.go:41 +0x3e6
gitea.hashru.nl/dsprenkels/rushlink.TestIssue43(0xc0001a25a0)
	/home/mrngm/scm/rushlink/handlers_test.go:58 +0x4e
testing.tRunner(0xc0001a25a0, 0x936138)
	/usr/lib/go/src/testing/testing.go:991 +0xdc
created by testing.(*T).Run
	/usr/lib/go/src/testing/testing.go:1042 +0x357
FAIL	gitea.hashru.nl/dsprenkels/rushlink	0.015s
?   	gitea.hashru.nl/dsprenkels/rushlink/cmd/rushlink	[no test files]
?   	gitea.hashru.nl/dsprenkels/rushlink/internal/db	[no test files]
?   	gitea.hashru.nl/dsprenkels/rushlink/pkg/gobmarsh	[no test files]
FAIL
Hmm, I find it less than ideal that we have to run `go generate ./...`, otherwise this happens: ``` [ mrngm@kaakje ~/scm/rushlink[test-issue-45*] ] $ go test ./... 2020/04/22 18:40:29 migrating database to version 1 2020/04/22 18:40:29 migrating database to version 2 2020/04/22 18:40:29 migrating database to version 3 --- FAIL: TestIssue43 (0.00s) panic: template: :10:26: executing "" at <.CanDelete.String>: can't evaluate field String in type interface {} [recovered] panic: template: :10:26: executing "" at <.CanDelete.String>: can't evaluate field String in type interface {} goroutine 39 [running]: testing.tRunner.func1.1(0x8cb000, 0xc00018d120) /usr/lib/go/src/testing/testing.go:940 +0x2f5 testing.tRunner.func1(0xc0001a25a0) /usr/lib/go/src/testing/testing.go:943 +0x3f9 panic(0x8cb000, 0xc00018d120) /usr/lib/go/src/runtime/panic.go:969 +0x166 gitea.hashru.nl/dsprenkels/rushlink.(*rushlink).render(0xc00018cbe0, 0x9c3ea0, 0xc0001a1700, 0xc00019b100, 0x914fe2, 0x9, 0xc0001fb920) /home/mrngm/scm/rushlink/views.go:154 +0xa41 gitea.hashru.nl/dsprenkels/rushlink.(*rushlink).viewCreateSuccess(0xc00018cbe0, 0x9c3ea0, 0xc0001a1700, 0xc00019b100, 0xc000180ba0, 0x0) /home/mrngm/scm/rushlink/handlers.go:207 +0x223 gitea.hashru.nl/dsprenkels/rushlink.(*rushlink).newRedirectPasteHandler(0xc00018cbe0, 0x9c3ea0, 0xc0001a1700, 0xc00019b100, 0xc0001b6440, 0x1c) /home/mrngm/scm/rushlink/handlers.go:291 +0x29b gitea.hashru.nl/dsprenkels/rushlink.(*rushlink).newPasteHandler(0xc00018cbe0, 0x9c3ea0, 0xc0001a1700, 0xc00019b100) /home/mrngm/scm/rushlink/handlers.go:226 +0x20b net/http.HandlerFunc.ServeHTTP(...) /usr/lib/go/src/net/http/server.go:2012 gitea.hashru.nl/dsprenkels/rushlink.TestIssue43.func1(0xc00018e108, 0xc0001927d0, 0xc0001d8400) /home/mrngm/scm/rushlink/handlers_test.go:73 +0x2ef gitea.hashru.nl/dsprenkels/rushlink.wrapRushlinkTest(0xc0001a25a0, 0xc000147f60) /home/mrngm/scm/rushlink/handlers_test.go:41 +0x3e6 gitea.hashru.nl/dsprenkels/rushlink.TestIssue43(0xc0001a25a0) /home/mrngm/scm/rushlink/handlers_test.go:58 +0x4e testing.tRunner(0xc0001a25a0, 0x936138) /usr/lib/go/src/testing/testing.go:991 +0xdc created by testing.(*T).Run /usr/lib/go/src/testing/testing.go:1042 +0x357 FAIL gitea.hashru.nl/dsprenkels/rushlink 0.015s ? gitea.hashru.nl/dsprenkels/rushlink/cmd/rushlink [no test files] ? gitea.hashru.nl/dsprenkels/rushlink/internal/db [no test files] ? gitea.hashru.nl/dsprenkels/rushlink/pkg/gobmarsh [no test files] FAIL ```
Author
Owner

Hmmm. Is there a way to do go generate automatically? If not, should we maybe just do it as part of the test?

Hmmm. Is there a way to do `go generate` automatically? If not, should we maybe just do it as part of the test?
mrngm requested changes 2020-04-22 18:55:56 +02:00
handlers_test.go Outdated
@ -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()
Collaborator

I would suggest:

    t.Logf("request body:\n%v\n", rr.Body.String())
    t.Fatalf("handler returned wrong status code: got %v want %v\n", actual, expected)
I would suggest: ```go t.Logf("request body:\n%v\n", rr.Body.String()) t.Fatalf("handler returned wrong status code: got %v want %v\n", actual, expected) ```
handlers_test.go Outdated
@ -0,0 +55,4 @@
}
func TestIssue43(t *testing.T) {
wrapRushlinkTest(t, func(rl rushlink) {
Collaborator

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:

func initTestIssue43(t *testing.T) rushlink {
    // content of wrapRushlinkTest

    return rushlink{
        db: database,
        fs: fileStore,
        rootURL: rootURL,
    }
}

func TestIssue43(t *testing.T) {
    rl := initTestIssue43(t)
    // rest of the inlined function
}
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: ```go func initTestIssue43(t *testing.T) rushlink { // content of wrapRushlinkTest return rushlink{ db: database, fs: fileStore, rootURL: rootURL, } } func TestIssue43(t *testing.T) { rl := initTestIssue43(t) // rest of the inlined function }
Collaborator

Hmmm. Is there a way to do go generate automatically? If not, should we maybe just do it as part of the test?

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.

> Hmmm. Is there a way to do `go generate` automatically? If not, should we maybe just do it as part of the test? 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.
mrngm reviewed 2020-04-22 19:09:07 +02:00
handlers_test.go Outdated
@ -0,0 +78,4 @@
t.Fatal(err)
return
}
t.Logf("URL shortened to: %s\n", pasteURL.Path[1:])
Collaborator
$ go test -v ./...
=== RUN   TestIssue43
2020/04/22 19:07:49 migrating database to version 1
2020/04/22 19:07:49 migrating database to version 2
2020/04/22 19:07:49 migrating database to version 3
    TestIssue43: handlers_test.go:81: URL shortened to: Ru2K

That is nice to read, but if you log this, then perhaps also do t.Logf("URL redirects to: %q", redirect") after redirect := rr.Header().Get("Location")?

``` $ go test -v ./... === RUN TestIssue43 2020/04/22 19:07:49 migrating database to version 1 2020/04/22 19:07:49 migrating database to version 2 2020/04/22 19:07:49 migrating database to version 3 TestIssue43: handlers_test.go:81: URL shortened to: Ru2K ``` That is nice to read, but if you log this, then perhaps also do `t.Logf("URL redirects to: %q", redirect")` after `redirect := rr.Header().Get("Location")`?
mrngm reviewed 2020-04-22 19:09:32 +02:00
handlers_test.go Outdated
@ -0,0 +76,4 @@
pasteURL, err := url.Parse(rawurl)
if err != nil {
t.Fatal(err)
return
Collaborator

return is dead code.

`return` is dead code.
mrngm reviewed 2020-04-22 19:14:28 +02:00
handlers_test.go Outdated
@ -0,0 +90,4 @@
handler = http.HandlerFunc(rl.viewPasteHandler)
handler.ServeHTTP(rr, req)
checkStatusCode(t, rr, http.StatusTemporaryRedirect)
redirect := rr.Header().Get("Location")
Collaborator

If you insert functions like checkStatusCode, then checkRedirectURL might be an addition for legibility as well.

If you insert functions like `checkStatusCode`, then `checkRedirectURL` might be an addition for legibility as well.
mrngm requested changes 2020-04-26 17:08:12 +02:00
handlers_test.go Outdated
@ -0,0 +14,4 @@
"github.com/gorilla/mux"
)
// initRushlinkTest initializes a rushlink instance, with temporary filestore
Collaborator

(the comment is outdated)

(the comment is outdated)
handlers_test.go 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]
Collaborator

(style: rawURL, similar to the line below)

(style: `rawURL`, similar to the line below)
Author
Owner

Ack.

Ack.
minnozz requested changes 2020-04-26 23:50:18 +02:00
handlers_test.go Outdated
@ -0,0 +73,4 @@
func TestIssue43(t *testing.T) {
rl, tempDir := createTemporaryRushlink(t)
t.Cleanup(func() { destroyTemporaryRushlink(&rl, tempDir) })
Collaborator

Hmm, maybe createTemporaryRushlink should register its own cleanup on t?

Hmm, maybe `createTemporaryRushlink` should register its own cleanup on `t`?
Author
Owner

Good point. :)

Good point. :)
handlers_test.go Outdated
@ -0,0 +76,4 @@
t.Cleanup(func() { destroyTemporaryRushlink(&rl, tempDir) })
// Put a URL with a fragment identifier into the database.
body := strings.NewReader(`--XXXXXXXX
Collaborator

It is probably neater to create your multipart/form-data POST request using https://golang.org/pkg/mime/multipart/#Writer.CreateFormField

It is probably neater to create your `multipart/form-data` POST request using https://golang.org/pkg/mime/multipart/#Writer.CreateFormField
Author
Owner

Check. Agreed.

Check. Agreed.
handlers_test.go Outdated
@ -0,0 +88,4 @@
}
req.Header.Add("Content-Type", "multipart/form-data; boundary=XXXXXXXX")
rr := httptest.NewRecorder()
handler := http.HandlerFunc(rl.newPasteHandler)
Collaborator

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 of StartMainServer into a separate function that you can also use during testing.

createTemporaryRushlink can do this setup and return an http.Server too.

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 of `StartMainServer` into a separate function that you can also use during testing. `createTemporaryRushlink` can do this setup and return an `http.Server` too.
Author
Owner

Fixed this, though rather than having createTemporaryRushlink return a http.Server, I have it return a mux.Router instead.

This allows us to ignore connection-level stuff (like timeouts etc.) but it enabled testing the router.

Fixed this, though rather than having `createTemporaryRushlink ` return a `http.Server`, I have it return a `mux.Router` instead. This allows us to ignore connection-level stuff (like timeouts etc.) but it enabled testing the router.
electricdusk closed this pull request 2020-04-27 13:00:32 +02:00
Author
Owner

I updated the patch according to the new reviews and merged the PR. :)

Thanks @mrngm and @minnozz!

I updated the patch according to the new reviews and merged the PR. :) Thanks @mrngm and @minnozz!
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: electricdusk/rushlink#46
No description provided.