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
2 changed files with 133 additions and 18 deletions
Showing only changes of commit c57e719e15 - Show all commits

112
handlers_test.go Normal file
View File

@ -0,0 +1,112 @@
package rushlink
import (
"bytes"
"io/ioutil"
"mime/multipart"
"net/http"
"net/http/httptest"
"net/url"
"os"
"path/filepath"
"strings"
"testing"
"gitea.hashru.nl/dsprenkels/rushlink/internal/db"
"github.com/gorilla/mux"
)
Outdated
Review

(the comment is outdated)

(the comment is outdated)
// createTemporaryRouter initializes a rushlink instance, with temporary
// filestore and database.
//
// It will use testing.T.Cleanup to cleanup after itself.
func createTemporaryRouter(t *testing.T) *mux.Router {
tempDir, err := ioutil.TempDir("", "rushlink-tmp-*")
if err != nil {
t.Fatalf("creating temporary directory: %s\n", err)
}
t.Cleanup(func() {
os.RemoveAll(tempDir)
})
fileStore, err := db.OpenFileStore(filepath.Join(tempDir, "filestore"))
if err != nil {
t.Fatalf("opening temporary filestore: %s\n", err)
}
databasePath := filepath.Join(tempDir, "rushlink.db")
database, err := db.OpenDB(databasePath, fileStore)
if err != nil {
t.Fatalf("opening temporary database: %s\n", err)
}
t.Cleanup(func() {
if err := database.Close(); err != nil {
t.Errorf("closing database: %d\n", err)
}
})
// *.invalid. is guaranteed not to exist (RFC 6761).
rootURL, err := url.Parse("https://rushlink.invalid")
if err != nil {
t.Fatalf("parsing URL: %s\n", err)
}
rl := rushlink{
Outdated
Review

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) ```
db: database,
fs: fileStore,
rootURL: rootURL,
}
return CreateMainRouter(&rl)
Outdated
Review

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 }
}
// checkStatusCode checks whether the status code from a recorded response is equal
// to some response code.
func checkStatusCode(t *testing.T, rr *httptest.ResponseRecorder, code int) {
if actual := rr.Code; actual != code {
t.Logf("request body:\n%v\n", rr.Body.String())
t.Fatalf("handler returned wrong status code: got %v want %v\n",
actual, code)
}
}
// checkLocationHeader checks whether the status code from a recorded response is equal
// to some expected URL.
func checkLocationHeader(t *testing.T, rr *httptest.ResponseRecorder, expected string) {
location := rr.Header().Get("Location")
if location != expected {
t.Fatalf("handler returned bad redirect location: got %v want %v", location, expected)

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

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

Good point. :)

Good point. :)
}
}
Outdated
Review

return is dead code.

`return` is dead code.

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

Check. Agreed.

Check. Agreed.
func TestIssue43(t *testing.T) {
srv := createTemporaryRouter(t)
Outdated
Review
$ 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")`?
// Put a URL with a fragment identifier into the database.
var body bytes.Buffer
form := multipart.NewWriter(&body)
form.WriteField("shorten", "https://example.com#fragment")
form.Close()
req, err := http.NewRequest("POST", "/", bytes.NewReader(body.Bytes()))
if err != nil {
t.Fatal(err)
}

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.
req.Header.Add("Content-Type", form.FormDataContentType())
rr := httptest.NewRecorder()
Outdated
Review

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.
srv.ServeHTTP(rr, req)
Outdated
Review

(style: rawURL, similar to the line below)

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

Ack.

Ack.
checkStatusCode(t, rr, http.StatusOK)
rawURL := strings.SplitN(rr.Body.String(), "\n", 2)[0]
pasteURL, err := url.Parse(rawURL)
if err != nil {
t.Fatal(err)
}
// Check if the URL was encoded correctly.
req, err = http.NewRequest("GET", pasteURL.Path, nil)
if err != nil {
t.Fatal(err)
}
req = mux.SetURLVars(req, map[string]string{"key": pasteURL.Path[1:]})
rr = httptest.NewRecorder()
srv.ServeHTTP(rr, req)
checkStatusCode(t, rr, http.StatusTemporaryRedirect)
checkLocationHeader(t, rr, "https://example.com#fragment")
}

View File

@ -79,23 +79,8 @@ func (w *statusResponseWriter) WriteHeader(statusCode int) {
w.Inner.WriteHeader(statusCode) w.Inner.WriteHeader(statusCode)
} }
// StartMainServer starts the main http server listening on addr. // CreateMainRouter creates the main Gorilla router for the application.
func StartMainServer(addr string, db *db.Database, fs *db.FileStore, rawRootURL string) { func CreateMainRouter(rl *rushlink) *mux.Router {
var rootURL *url.URL
if rawRootURL != "" {
var err error
rootURL, err = url.Parse(rawRootURL)
if err != nil {
log.Fatalln(errors.Wrap(err, "could not parse rootURL flag"))
}
}
rl := rushlink{
db: db,
fs: fs,
rootURL: rootURL,
}
// Initialize Gorilla router
router := mux.NewRouter() router := mux.NewRouter()
router.Use(rl.recoveryMiddleware) router.Use(rl.recoveryMiddleware)
router.Use(rl.metricsMiddleware) router.Use(rl.metricsMiddleware)
@ -114,9 +99,27 @@ func StartMainServer(addr string, db *db.Database, fs *db.FileStore, rawRootURL
router.HandleFunc("/"+urlKeyWithExtExpr, rl.deletePasteHandler).Methods("DELETE") router.HandleFunc("/"+urlKeyWithExtExpr, rl.deletePasteHandler).Methods("DELETE")
router.HandleFunc("/"+urlKeyExpr+"/delete", rl.deletePasteHandler).Methods("POST") router.HandleFunc("/"+urlKeyExpr+"/delete", rl.deletePasteHandler).Methods("POST")
router.HandleFunc("/"+urlKeyWithExtExpr+"/delete", rl.deletePasteHandler).Methods("POST") router.HandleFunc("/"+urlKeyWithExtExpr+"/delete", rl.deletePasteHandler).Methods("POST")
return router
}
// StartMainServer starts the main http server listening on addr.
func StartMainServer(addr string, db *db.Database, fs *db.FileStore, rawRootURL string) {
var rootURL *url.URL
if rawRootURL != "" {
var err error
rootURL, err = url.Parse(rawRootURL)
if err != nil {
log.Fatalln(errors.Wrap(err, "could not parse rootURL flag"))
}
}
rl := rushlink{
db: db,
fs: fs,
rootURL: rootURL,
}
srv := &http.Server{ srv := &http.Server{
Handler: router, Handler: CreateMainRouter(&rl),
Addr: addr, Addr: addr,
WriteTimeout: 15 * time.Second, WriteTimeout: 15 * time.Second,
ReadTimeout: 15 * time.Second, ReadTimeout: 15 * time.Second,