Repository: gavv/httpexpect
Pull request #367 (2023-04-01 18:55:42)

refactor request url interpolate tests
#326 
@gavv Hi! Check out please. I split tests into subtests, but I couldn't think of a case when `invalid interpol string` error is returned. And there was an issue with checking value on nil, it was duplicated in [here](https://github.com/gavv/httpexpect/blob/master/request.go#L964) and [here](https://github.com/gavv/httpexpect/blob/master/request.go#L877)

coveralls: 
[![Coverage Status](https://coveralls.io/builds/58476221/badge)](https://coveralls.io/builds/58476221)

Coverage: 95.666% (-0.01%) from 95.68% when pulling **8d23dbb31edbfa52344d73df2fed4bd265e1e406 on diPhantxm:refactor-request-url-interpolate-test** into **44aea0735601ca40075b7cd75a05cf985f577f0f on gavv:master**.

gavv: Thanks for PR!

> I split tests into subtests.

Looks fine.

One suggestion: I'd merge two "basic" subtests into one, rename it to "constructor" (because it tests how NewRequestC works), and make it table-driven, with the following table fields: path, path args, expected url.

> I couldn't think of a case when invalid interpol string error is returned

Seems it can't be reached with current implementation. The error could be triggered only if interpol string is invalid, e.g. there is unmatched brace. You already have a test for it. However, this error will be handled early by NewRequestC, and WithPath just won't be executed.

After looking at the code a bit, I found it's strange that NewRequestC tries to perform interpolation even if there are not pathargs given. It would be safer to avoid messing with interpol in this case, e.g. modify initPath as follows:

```go
if len(pathargs) != 0 {
    <all the interpolation code goes here>
}

r.path = path
```

AND, magically, if we do this, we'll also be able to cover both errors (in initPath and in withPath), by providing invalid interpol string and providing args either to constructor or to WithPath.

> And t[here](https://github.com/gavv/httpexpect/blob/master/request.go#L964) was an issue with checking value on nil, it was duplicated in [here](https://github.com/gavv/httpexpect/blob/master/request.go#L877) and here

Good catch.
gavv: Could you please also cover this branch? https://coveralls.io/builds/58476221/source?filename=request.go#L153

(nil patharg passed to constructor)
diPhantxm: Yeah, sure
diPhantxm: > After looking at the code a bit, I found it's strange that NewRequestC tries to perform interpolation even if there are not pathargs given. It would be safer to avoid messing with interpol in this case, e.g. modify initPath as follows

If I add this if, the following is success, but must be failure:
```Golang
req := NewRequestC(config, "GET", "{arg")
req.chain.assert(t, failure)
```
And I don't know why, but it also affects interpolation in constructor even if it doesn't seem to have any affect

diPhantxm: I managed to fix the problem. It was because of assigning to a new variable `path` inside if-statement

Patch:
diff --git a/request.go b/request.go
index 74e845f..94172ec 100644
--- a/request.go
+++ b/request.go
@@ -145,39 +145,42 @@ func newRequest(
 }
 
 func (r *Request) initPath(opChain *chain, path string, pathargs ...interface{}) {
-	var n int
-
-	path, err := interpol.WithFunc(path, func(k string, w io.Writer) error {
-		if n < len(pathargs) {
-			if pathargs[n] == nil {
-				opChain.fail(AssertionFailure{
-					Type:   AssertValid,
-					Actual: &AssertionValue{pathargs},
-					Errors: []error{
-						fmt.Errorf("unexpected nil argument at index %d", n),
-					},
-				})
+	if len(pathargs) != 0 {
+		var n int
+
+		var err error
+		path, err = interpol.WithFunc(path, func(k string, w io.Writer) error {
+			if n < len(pathargs) {
+				if pathargs[n] == nil {
+					opChain.fail(AssertionFailure{
+						Type:   AssertValid,
+						Actual: &AssertionValue{pathargs},
+						Errors: []error{
+							fmt.Errorf("unexpected nil argument at index %d", n),
+						},
+					})
+				} else {
+					mustWrite(w, fmt.Sprint(pathargs[n]))
+				}
 			} else {
-				mustWrite(w, fmt.Sprint(pathargs[n]))
+				mustWrite(w, "{")
+				mustWrite(w, k)
+				mustWrite(w, "}")
 			}
-		} else {
-			mustWrite(w, "{")
-			mustWrite(w, k)
-			mustWrite(w, "}")
-		}
-		n++
-		return nil
-	})
-
-	if err != nil {
-		opChain.fail(AssertionFailure{
-			Type:   AssertValid,
-			Actual: &AssertionValue{path},
-			Errors: []error{
-				errors.New("invalid interpol string"),
-				err,
-			},
+			n++
+			return nil
 		})
+
+		if err != nil {
+			opChain.fail(AssertionFailure{
+				Type:   AssertValid,
+				Actual: &AssertionValue{path},
+				Errors: []error{
+					errors.New("invalid interpol string"),
+					err,
+				},
+			})
+		}
 	}
 
 	r.path = path
@@ -874,16 +877,6 @@ func (r *Request) WithPath(key string, value interface{}) *Request {
 		return r
 	}
 
-	if value == nil {
-		opChain.fail(AssertionFailure{
-			Type: AssertUsage,
-			Errors: []error{
-				errors.New("unexpected nil argument"),
-			},
-		})
-		return r
-	}
-
 	r.withPath(opChain, key, value)
 
 	return r


Test patch:
diff --git a/e2e_report_test.go b/e2e_report_test.go
index e725986..36ba0f5 100644
--- a/e2e_report_test.go
+++ b/e2e_report_test.go
@@ -55,7 +55,206 @@ func TestE2EReport_Names(t *testing.T) {
 	assert.Contains(t, reporter.recorded, "RequestExample")
 }
 
-func TestE2EReport_Aliases(t *testing.T) {
+func TestE2EReport_Values(t *testing.T) {
+	mux := http.NewServeMux()
+
+	mux.HandleFunc("/text", func(w http.ResponseWriter, r *http.Request) {
+		_, _ = w.Write([]byte("ACTUAL TEXT"))
+	})
+	mux.HandleFunc("/number", func(w http.ResponseWriter, r *http.Request) {
+		_, _ = w.Write([]byte("88888888"))
+	})
+	mux.HandleFunc("/array", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Add("Content-Type", "application/json")
+		_, _ = w.Write([]byte("[111, 222, 444, 333]"))
+	})
+
+	server := httptest.NewServer(mux)
+	defer server.Close()
+
+	formatter := &DefaultFormatter{
+		DigitSeparator: DigitSeparatorNone,
+	}
+
+	t.Run("actual vs expected", func(t *testing.T) {
+		reporter := &recordingReporter{}
+
+		e := WithConfig(Config{
+			BaseURL:   server.URL,
+			Reporter:  reporter,
+			Formatter: formatter,
+		})
+
+		e.GET("/text").
+			Expect().
+			Body().
+			IsEqual("EXPECTED TEXT")
+		t.Logf("%s", reporter.recorded)
+
+		assert.Contains(
+			t, reporter.recorded, "expected: strings are equal",
+			"missing Errors",
+		)
+		assert.Contains(
+			t, reporter.recorded, "ACTUAL TEXT", "missing Actual",
+		)
+		assert.Contains(
+			t, reporter.recorded, "EXPECTED TEXT", "missing Expected",
+		)
+	})
+
+	t.Run("reference", func(t *testing.T) {
+		reporter := &recordingReporter{}
+
+		e := WithConfig(Config{
+			BaseURL:   server.URL,
+			Reporter:  reporter,
+			Formatter: formatter,
+		})
+
+		e.GET("/array").
+			Expect().
+			JSON().
+			Array().
+			IsOrdered()
+		t.Logf("%s", reporter.recorded)
+
+		trimmed := strings.Join(strings.Fields(reporter.recorded), "")
+
+		assert.Contains(
+			t, reporter.recorded, "expected: reference array is ordered",
+			"missing Errors",
+		)
+		assert.Contains(
+			t, trimmed, "[111,222,444,333]", "missing Reference",
+		)
+	})
+
+	t.Run("delta", func(t *testing.T) {
+		reporter := &recordingReporter{}
+
+		e := WithConfig(Config{
+			BaseURL:   server.URL,
+			Reporter:  reporter,
+			Formatter: formatter,
+		})
+
+		e.GET("/number").
+			Expect().
+			Body().
+			AsNumber().
+			InDelta(9999999, 0.55555555)
+		t.Logf("%s", reporter.recorded)
+
+		assert.Contains(
+			t, reporter.recorded, "expected: numbers lie within delta",
+			"missing Errors",
+		)
+		assert.Contains(
+			t, reporter.recorded, "8888888", "missing Actual",
+		)
+		assert.Contains(
+			t, reporter.recorded, "9999999", "missing Expected",
+		)
+		assert.Contains(
+			t, reporter.recorded, "0.55555555", "missing Delta",
+		)
+	})
+
+	t.Run("range", func(t *testing.T) {
+		reporter := &recordingReporter{}
+
+		e := WithConfig(Config{
+			BaseURL:   server.URL,
+			Reporter:  reporter,
+			Formatter: formatter,
+		})
+
+		e.GET("/number").
+			Expect().
+			Body().
+			AsNumber().
+			InRange(333333, 444444)
+		t.Logf("%s", reporter.recorded)
+
+		assert.Contains(
+			t, reporter.recorded, "expected: number is within given range",
+			"missing Errors",
+		)
+		assert.Contains(
+			t, reporter.recorded, "8888888", "missing Actual",
+		)
+		assert.Contains(
+			t, reporter.recorded, "[333333; 444444]", "missing Expected",
+		)
+	})
+
+	t.Run("list", func(t *testing.T) {
+		reporter := &recordingReporter{}
+
+		e := WithConfig(Config{
+			BaseURL:   server.URL,
+			Reporter:  reporter,
+			Formatter: formatter,
+		})
+
+		e.GET("/number").
+			Expect().
+			Body().
+			AsNumber().
+			InList(333333, 444444)
+		t.Logf("%s", reporter.recorded)
+
+		trimmed := strings.Join(strings.Fields(reporter.recorded), " ")
+
+		assert.Contains(
+			t, reporter.recorded, "expected: number is equal to one of the values",
+			"missing Errors",
+		)
+		assert.Contains(
+			t, reporter.recorded, "8888888", "missing Actual",
+		)
+		assert.Contains(
+			t, trimmed, "333333 444444", "missing Expected",
+		)
+	})
+}
+
+func TestE2EReport_Path(t *testing.T) {
+	mux := http.NewServeMux()
+
+	mux.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"foo":123}`))
+	})
+
+	server := httptest.NewServer(mux)
+	defer server.Close()
+
+	reporter := &recordingReporter{}
+
+	e := WithConfig(Config{
+		BaseURL:  server.URL,
+		Reporter: reporter,
+	})
+
+	e.GET("/test").
+		Expect().
+		JSON().
+		Object().
+		ContainsKey("bar") // will fail
+
+	t.Logf("%s", reporter.recorded)
+
+	assert.Contains(
+		t,
+		reporter.recorded,
+		`Request("GET").Expect().JSON().Object()`,
+		"cannot find Path value in report",
+	)
+}
+
+func TestE2EReport_Alias(t *testing.T) {
 	mux := http.NewServeMux()
 
 	mux.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request) {
@@ -83,7 +282,11 @@ func TestE2EReport_Aliases(t *testing.T) {
 
 	t.Logf("%s", reporter.recorded)
 
-	assert.Contains(t, reporter.recorded, "foo.Object().ContainsKey()")
+	assert.Contains(
+		t,
+		reporter.recorded,
+		"foo.Object().ContainsKey()",
+		"cannot find AliasedPath value in report")
 }
 
 func TestE2EReport_LineWidth(t *testing.T) {
diff --git a/request_test.go b/request_test.go
index f54ff18..560e3aa 100644
--- a/request_test.go
+++ b/request_test.go
@@ -719,36 +719,69 @@ func TestRequest_URLInterpolate(t *testing.T) {
 		Reporter: newMockReporter(t),
 	}
 
-	var reqs [3]*Request
+	t.Run("constructor", func(t *testing.T) {
+		cases := []struct {
+			path        string
+			pathArgs    []interface{}
+			expectedURL string
+			want        chainResult
+		}{
+			{
+				"/foo/{arg}",
+				[]interface{}{"bar"},
+				"http://example.com/foo/bar",
+				success,
+			},
+			{
+				"{arg}foo{arg}",
+				[]interface{}{"/", "/bar"},
+				"http://example.com/foo/bar",
+				success,
+			},
+			{
+				"{arg}",
+				[]interface{}{"/foo/bar"},
+				"http://example.com/foo/bar",
+				success,
+			},
+			{
+				"/{arg1}/{arg2}",
+				[]interface{}{"foo"},
+				"http://example.com/foo/%7Barg2%7D",
+				success,
+			},
+			{
+				"/{arg1}",
+				[]interface{}{nil},
+				"http://example.com/foo/%7Barg2%7D",
+				failure,
+			},
+		}
 
-	reqs[0] = NewRequestC(config, "GET", "/foo/{arg}", "bar")
-	reqs[1] = NewRequestC(config, "GET", "{arg}foo{arg}", "/", "/bar")
-	reqs[2] = NewRequestC(config, "GET", "{arg}", "/foo/bar")
+		for _, tc := range cases {
+			req := NewRequestC(config, "GET", tc.path, tc.pathArgs...)
+			req.Expect().chain.assert(t, tc.want)
+			assert.Equal(t, tc.expectedURL, client.req.URL.String())
+		}
+	})
 
-	for _, req := range reqs {
+	t.Run("WithPath", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "/{arg1}/{arg2}/{arg3}")
+		req.WithPath("ARG3", "foo")
+		req.WithPath("arg2", "bar")
 		req.Expect().chain.assert(t, success)
-		assert.Equal(t, "http://example.com/foo/bar", client.req.URL.String())
-	}
+		assert.Equal(t, "http://example.com/%7Barg1%7D/bar/foo",
+			client.req.URL.String())
+	})
 
-	r1 := NewRequestC(config, "GET", "/{arg1}/{arg2}", "foo")
-	r1.Expect().chain.assert(t, success)
-	assert.Equal(t, "http://example.com/foo/%7Barg2%7D",
-		client.req.URL.String())
-
-	r2 := NewRequestC(config, "GET", "/{arg1}/{arg2}/{arg3}")
-	r2.WithPath("ARG3", "foo")
-	r2.WithPath("arg2", "bar")
-	r2.Expect().chain.assert(t, success)
-	assert.Equal(t, "http://example.com/%7Barg1%7D/bar/foo",
-		client.req.URL.String())
-
-	r3 := NewRequestC(config, "GET", "/{arg1}.{arg2}.{arg3}")
-	r3.WithPath("arg2", "bar")
-	r3.WithPathObject(map[string]string{"ARG1": "foo", "arg3": "baz"})
-	r3.WithPathObject(nil)
-	r3.Expect().chain.assert(t, success)
-	assert.Equal(t, "http://example.com/foo.bar.baz",
-		client.req.URL.String())
+	t.Run("WithPathObject map[string]interface{}", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "/{arg1}.{arg2}.{arg3}")
+		req.WithPathObject(map[string]string{"ARG1": "foo", "arg2": "bar", "arg3": "baz"})
+		req.WithPathObject(nil)
+		req.Expect().chain.assert(t, success)
+		assert.Equal(t, "http://example.com/foo.bar.baz",
+			client.req.URL.String())
+	})
 
 	type S struct {
 		Arg1 string
@@ -756,38 +789,63 @@ func TestRequest_URLInterpolate(t *testing.T) {
 		A3   int `path:"-"`
 	}
 
-	r4 := NewRequestC(config, "GET", "/{arg1}{arg2}")
-	r4.WithPathObject(S{"foo", 1, 2})
-	r4.Expect().chain.assert(t, success)
-	assert.Equal(t, "http://example.com/foo1", client.req.URL.String())
-
-	r5 := NewRequestC(config, "GET", "/{arg1}{arg2}")
-	r5.WithPathObject(&S{"foo", 1, 2})
-	r5.Expect().chain.assert(t, success)
-	assert.Equal(t, "http://example.com/foo1", client.req.URL.String())
-
-	r6 := NewRequestC(config, "GET", "{arg}", nil)
-	r6.chain.assert(t, failure)
-
-	r7 := NewRequestC(config, "GET", "{arg}")
-	r7.chain.assert(t, success)
-	r7.WithPath("arg", nil)
-	r7.chain.assert(t, failure)
-
-	r8 := NewRequestC(config, "GET", "{arg}")
-	r8.chain.assert(t, success)
-	r8.WithPath("bad", "value")
-	r8.chain.assert(t, failure)
-
-	r9 := NewRequestC(config, "GET", "{arg")
-	r9.chain.assert(t, failure)
-	r9.WithPath("arg", "foo")
-	r9.chain.assert(t, failure)
-
-	r10 := NewRequestC(config, "GET", "{arg}")
-	r10.chain.assert(t, success)
-	r10.WithPathObject(func() {})
-	r10.chain.assert(t, failure)
+	t.Run("WithPathObject struct", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "/{arg1}{arg2}")
+		req.WithPathObject(S{"foo", 1, 2})
+		req.Expect().chain.assert(t, success)
+		assert.Equal(t, "http://example.com/foo1", client.req.URL.String())
+	})
+
+	t.Run("WithPathObject pointer to struct", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "/{arg1}{arg2}")
+		req.WithPathObject(&S{"foo", 1, 2})
+		req.Expect().chain.assert(t, success)
+		assert.Equal(t, "http://example.com/foo1", client.req.URL.String())
+	})
+
+	t.Run("WithPath and WithPathObject", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "/{arg1}{arg2}")
+		req.WithPathObject(map[string]string{"arg1": "foo"})
+		req.WithPath("arg2", 1)
+		req.Expect().chain.assert(t, success)
+		assert.Equal(t, "http://example.com/foo1", client.req.URL.String())
+	})
+
+	t.Run("WithPath nil", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "{arg}")
+		req.WithPath("arg", nil)
+		req.chain.assert(t, failure)
+	})
+
+	t.Run("WithPathObject nil", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "{arg}")
+		req.chain.assert(t, success)
+		req.WithPath("arg", "foo")
+		req.WithPathObject(nil)
+		req.Expect().chain.assert(t, success)
+		assert.Equal(t, "http://example.com/foo", client.req.URL.String())
+	})
+
+	t.Run("WithPath invalid key", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "{arg}")
+		req.chain.assert(t, success)
+		req.WithPath("bad", "value")
+		req.chain.assert(t, failure)
+	})
+
+	t.Run("WithPath invalid path", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "{arg")
+		req.chain.assert(t, success)
+		req.WithPath("arg", "foo")
+		req.chain.assert(t, failure)
+	})
+
+	t.Run("WithPathObject invalid object", func(t *testing.T) {
+		req := NewRequestC(config, "GET", "{arg}")
+		req.chain.assert(t, success)
+		req.WithPathObject(func() {})
+		req.chain.assert(t, failure)
+	})
 }
 
 func TestRequest_URLQuery(t *testing.T) {