Added documentation for how to test path variables

[palsivertsen]

No description provided.

[elithrar]

Using mux.SetURLVars is an alternative to using route variables or table-driven testing. The benefits to the latter is that it’s much clearer, and you can build your tests from actual routes. SetURLVars can be a little magic. In this case, there’s not even a route variable in the URL itself, which makes setting an “invisible” variable odd.

Don’t change the existing example, just amend a smaller example to the bottom of it as an alternative.

[palsivertsen]

I'm not sure I follow. Yes, the example do show how to use table-driven testing, but it does not show nor test route variables as far as I can see.

In the current example there is a main function:

// endpoints.go
func main() {
    r := mux.NewRouter()
    // A route with a route variable:
    r.HandleFunc("/metrics/{type}", MetricsHandler)

    log.Fatal(http.ListenAndServe("localhost:8080", r))
}

This shows how to use the MetricsHandler but the not source code. Since MetricsHandler is what we actually want to test, the source should be included as well. I'm guessing it would look something like this:

func MetricsHandler(w http.ResponseWriter, r *http.Request) {
	vars := mux.Vars(r)
	typeVar, found := vars["type"]
	if !found {
		// This should never happen as the Gorilla Mux should set it for us
		panic("The request is missing path variable 'type'")
	}
	switch typeVar {
	case "goroutines", "heap", "counters", "queries":
		w.WriteHeader(http.StatusOK)
	default:
		w.WriteHeader(http.StatusNotFound)
	}
}

If you then to run the example, you'll discover it does not compile and has a bug when checking the response so I made a few adjustments:

package examples

import (
	"fmt"
	"net/http"
	"net/http/httptest"
	"testing"
)

// endpoints_test.go
func TestMetricsHandler(t *testing.T) {
	tt := []struct {
		routeVariable string
		shouldPass    bool
	}{
		{"goroutines", true},
		{"heap", true},
		{"counters", true},
		{"queries", true},
		{"adhadaeqm3k", false},
	}

- 	for _, t := tt {
-		path := fmt.Sprintf("/metrics/%s", t.routeVariable)
+	for _, data := range tt {
+		path := fmt.Sprintf("/metrics/%s", data.routeVariable)
		req, err := http.NewRequest("GET", path, nil)
		if err != nil {
			t.Fatal(err)
		}

		rr := httptest.NewRecorder()
		handler := http.HandlerFunc(MetricsHandler)
		handler.ServeHTTP(rr, req)

		// In this case, our MetricsHandler returns a non-200 response
		// for a route variable it doesn't know about.
-               if rr.Code == http.StatusOK && !t.shouldPass {
+		if (rr.Code == http.StatusOK) != data.shouldPass {
			t.Errorf("handler should have failed on routeVariable %s: got %v want %v",
-				t.routeVariable, rr.Code, http.StatusOK)
+				data.routeVariable, rr.Code, http.StatusOK)
		}
	}
}

If you then try running the test you'll see that MetricsHandler complains that it can not find the type path variable.

What I then did was:

• Removed table-driven part of test for simplicity. Although it is an excellent way to write tests, it has nothing to with path variables.
• Add the source of what is actually being tested. MetricsHandler was reanmed to VarLogger to make it more clear what is does.
• Fixed syntax errors in test example
• Fixed bug in test example

Alternatively I could add the VarLogger handler to a mux.Router and test this using mux.Router.ServeHTTP but I feel it's out of scope for the example and unit test.

[elithrar]

Appreciate the refactored PR:

but it does not show nor test route variables as far as I can see.

It does, as a black box. We create requests with our table of route variables, and check that we get the expected response. You don’t have to know the internals of the handler here: I intentionally left it out in order to keep the example focused. I’m OK with a longer PR, but I also want to make sure we’re on the same page regarding beginner-friendless.

…

On Sat, Jan 27, 2018 at 5:46 AM Pål Marius Sivertsen < ***@***.***> wrote: I'm not sure I follow. Yes, the example do show how to use table-driven testing, but it does not show nor test route variables as far as I can see. In the current example there is a main function: // endpoints.gofunc main() { r := mux.NewRouter() // A route with a route variable: r.HandleFunc("/metrics/{type}", MetricsHandler) log.Fatal(http.ListenAndServe("localhost:8080", r)) } This shows how to use the MetricsHandler but the not source code. Since MetricsHandler is what we actually want to test, the source should be included as well. I'm guessing it would look something like this: func MetricsHandler(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) typeVar, found := vars["type"] if !found { // This should never happen as the Gorilla Mux should set it for us panic("The request is missing path variable 'type'") } switch typeVar { case "goroutines", "heap", "counters", "queries": w.WriteHeader(http.StatusOK) default: w.WriteHeader(http.StatusNotFound) } } If you then to run the example, you'll discover it does not compile and has a bug when checking the response so I made a few adjustments: package examples import ( "fmt" "net/http" "net/http/httptest" "testing" ) // endpoints_test.go func TestMetricsHandler(t *testing.T) { tt := []struct { routeVariable string shouldPass bool }{ {"goroutines", true}, {"heap", true}, {"counters", true}, {"queries", true}, {"adhadaeqm3k", false}, } - for _, t := tt {- path := fmt.Sprintf("/metrics/%s", t.routeVariable)+ for _, data := range tt {+ path := fmt.Sprintf("/metrics/%s", data.routeVariable) req, err := http.NewRequest("GET", path, nil) if err != nil { t.Fatal(err) } rr := httptest.NewRecorder() handler := http.HandlerFunc(MetricsHandler) handler.ServeHTTP(rr, req) // In this case, our MetricsHandler returns a non-200 response // for a route variable it doesn't know about.- if rr.Code == http.StatusOK && !t.shouldPass {+ if (rr.Code == http.StatusOK) != data.shouldPass { t.Errorf("handler should have failed on routeVariable %s: got %v want %v",- t.routeVariable, rr.Code, http.StatusOK)+ data.routeVariable, rr.Code, http.StatusOK) } } } If you then try running the test you'll see that MetricsHandler complains that it can not find the type path variable. What I then did was: - Removed table-driven part of test for simplicity. Although it is an excellent way to write tests, it has nothing to with path variables. - Add the source of what is actually being tested. MetricsHandler was reanmed to VarLogger to make it more clear what is does. - Fixed syntax errors in test example - Fixed bug in test example Alternatively I could add the VarLogger handler to a mux.Router and test this using mux.Router.ServeHTTP but I feel it's out of scope for the example and unit test. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#342 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcNUsQ297QybkBj2Yhvu1WiXkKbk-ks5tOyjDgaJpZM4RuBU7> .

[palsivertsen]

I think we are on the same page regarding beginner friendliness :)

But I think I'm missing something. Please correct me if I'm wrong:
When the readme mentions variables it's referring to path variables. mux.Vars is how you retrieve variables from a request. There are two ways of setting a path variable, add a new route with /path/{myVar} path syntax or use mux.SetURLVars.

I don't see how the current example illustrates how to use path variables when it does not include any of mux.Vars, /path/{myVar} or mux.SetURLVars?

[stale]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.