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)
|
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,
|
||||||
|
(the comment is outdated)