Add a test for issue #45; NFC #46
112
handlers_test.go
Normal 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"
|
||||
)
|
||||
|
||||
|
||||
// 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{
|
||||
mrngm
commented
I would suggest:
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)
|
||||
mrngm
commented
I don't think the wrapper function is useful nor does it improve legibility. I would suggest putting all test code inside
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)
|
||||
minnozz
commented
Hmm, maybe Hmm, maybe `createTemporaryRushlink` should register its own cleanup on `t`?
electricdusk
commented
Good point. :) Good point. :)
|
||||
}
|
||||
}
|
||||
|
||||
mrngm
commented
`return` is dead code.
minnozz
commented
It is probably neater to create your It is probably neater to create your `multipart/form-data` POST request using https://golang.org/pkg/mime/multipart/#Writer.CreateFormField
electricdusk
commented
Check. Agreed. Check. Agreed.
|
||||
func TestIssue43(t *testing.T) {
|
||||
srv := createTemporaryRouter(t)
|
||||
mrngm
commented
That is nice to read, but if you log this, then perhaps also do ```
$ 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)
|
||||
}
|
||||
minnozz
commented
This part of the test bypasses the code in Since a lot of tests are going to want to send an HTTP request, I think it it wise to factor out the
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.
electricdusk
commented
Fixed this, though rather than having 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()
|
||||
mrngm
commented
If you insert functions like If you insert functions like `checkStatusCode`, then `checkRedirectURL` might be an addition for legibility as well.
|
||||
srv.ServeHTTP(rr, req)
|
||||
mrngm
commented
(style: (style: `rawURL`, similar to the line below)
electricdusk
commented
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")
|
||||
}
|
39
router.go
@ -79,23 +79,8 @@ func (w *statusResponseWriter) WriteHeader(statusCode int) {
|
||||
w.Inner.WriteHeader(statusCode)
|
||||
}
|
||||
|
||||
// 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,
|
||||
}
|
||||
|
||||
// Initialize Gorilla router
|
||||
// CreateMainRouter creates the main Gorilla router for the application.
|
||||
func CreateMainRouter(rl *rushlink) *mux.Router {
|
||||
router := mux.NewRouter()
|
||||
router.Use(rl.recoveryMiddleware)
|
||||
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("/"+urlKeyExpr+"/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{
|
||||
Handler: router,
|
||||
Handler: CreateMainRouter(&rl),
|
||||
Addr: addr,
|
||||
WriteTimeout: 15 * time.Second,
|
||||
ReadTimeout: 15 * time.Second,
|
||||
|
(the comment is outdated)