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)
```
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:
funcinitTestIssue43(t*testing.T)rushlink{// content of wrapRushlinkTestreturnrushlink{db:database,fs:fileStore,rootURL:rootURL,}}funcTestIssue43(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
}
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.
$ 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")`?
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.
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
mrngm
minnozz
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!