Repository: gavv/httpexpect
Pull request #220 (2023-01-02 01:31:31)

Add DefaultFormatter.DisableScientific
Issue: https://github.com/gavv/httpexpect/issues/190

Tasks:

- [x] Add DisableScientific flag in DefaultFormatter
- [x] Add/update _sample_ unit test (for early review)
- [ ] ~~Add/update remaining unit test (if _sample_ is ok)~~
- [X] Refine comments
- [ ] ~~Complete unit test with other assertion types (e.g. `formatting of AssertionList with number` as mentioned in https://github.com/gavv/httpexpect/pull/220#issuecomment-1399282552)~~

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

Coverage: 95.436% (+0.007%) from 95.43% when pulling **2b35d6a7ea72e38cbcad2783e5fc4a02bc77df36 on neverbeenthisweeb:190-default-formatter-disable-scientific** into **69c1ae446081de7b4d305213e459dcea58cad613 on gavv:master**.

gavv: I'd suggest to extract disable scientific ckecks into a separate test. This way it's easy to track what is covered for this flag.

Also, I'd double each check: one check with flag, one check without flag, both checks with the same number. This way it's easy to track how the flag changes behavior. 
gavv: Thanks for PR! See a few comments above.
neverbeenthisweeb: > I'd suggest to extract disable scientific ckecks into a separate test. This way it's easy to track what is covered for this flag.
> 
> Also, I'd double each check: one check with flag, one check without flag, both checks with the same number. This way it's easy to track how the flag changes behavior.

got it, will separate the unit tests
neverbeenthisweeb: > > I'd suggest to extract disable scientific ckecks into a separate test. This way it's easy to track what is covered for this flag.
> > Also, I'd double each check: one check with flag, one check without flag, both checks with the same number. This way it's easy to track how the flag changes behavior.
> 
> got it, will separate the unit tests

updated:

- https://github.com/gavv/httpexpect/pull/220/commits/1bfe1aaf40662aa33a5d9c2504b2d4635ea6aad6
- https://github.com/gavv/httpexpect/pull/220/commits/cc14c1efc4dd9b6729b2ab2d5afe9532f5575ca3

wdyt @gavv ? I check the template string instead of `formatData` here because:

- The `AssertionFailure` varies between cases of expected, actual, reference, and delta. Checking the `formatData` is less _generic_ because each "template value" is stored in different field (`.Expected`, `.Actual`, `.Reference`, and `.Delta`). So, I check the template string instead 

if u are OK, i will continue to test the actual, reference, and delta cases of `DisabledScientific`

thanks
gavv: > The `AssertionFailure` varies between cases of expected, actual, reference, and delta. Checking the `formatData` is less _generic_ because each "template value" is stored in different field (`.Expected`, `.Actual`, `.Reference`, and `.Delta`). So, I check the template string instead

But we can easily add wantExpected, wantActual, etc? In every test case, we need what field to expect because every test case manually set Expected, Actual, etc.

E.g. instead of:

```
neverbeenthisweeb: > > The `AssertionFailure` varies between cases of expected, actual, reference, and delta. Checking the `formatData` is less _generic_ because each "template value" is stored in different field (`.Expected`, `.Actual`, `.Reference`, and `.Delta`). So, I check the template string instead
> 
> But we can easily add wantExpected, wantActual, etc? In every test case, we already know what field to expect because every test case manually set Expected, Actual, etc.
> 
> E.g. instead of:
> 
> ```
> 			assertionFailure: AssertionFailure{
> 				Type: AssertInRange,
> 				Expected: &AssertionValue{
> 					Value: AssertionRange{
> 						Min: float32(-1234567.89),
> 						Max: float32(1234567.89),
> 					},
> 				},
> 			},
> 			wantTpl: "[-1234567.875; 1234567.875]",
> ```
> 
> we could write:
> 
> ```
> 			assertionFailure: AssertionFailure{
> 				Type: AssertInRange,
> 				Expected: &AssertionValue{
> 					Value: AssertionRange{
> 						Min: float32(-1234567.89),
> 						Max: float32(1234567.89),
> 					},
> 				},
> 			},
> 			wantExpected: "[-1234567.875; 1234567.875]",
> ```

got it

so, if we want to assert with `wantExpected`, `wantActual`, and `wantReference`, there will be 3 table driven tests (one for each _wantXXX_?
neverbeenthisweeb: hi @gavv , just FYI, sry i am a bit slow resp here. Perhaps, will progress on the weekend. Cheers 🥂 
gavv: Cool, no worries
neverbeenthisweeb: @gavv 

updates:
- Refactor unit test (merge all assertion into one test
- Add extractFloat to extract `float64` from `interface{}`
gavv: The updated test looks good.

Also it would be nice to cover these cases:

* formatting of AssertionList with number
* formatting of Expected with number that is not AssertionList or AssertionRange
* formatting of Actual with number
* formatting of Reference with number

What else? I guess only https://github.com/gavv/httpexpect/pull/220#discussion_r1068125302 ?
neverbeenthisweeb: > The updated test looks good.
> 
> Also it would be nice to cover these cases:
> 
> * formatting of AssertionList with number
> * formatting of Expected with number that is not AssertionList or AssertionRange
> * formatting of Actual with number
> * formatting of Reference with number
> 
> What else? I guess only [#220 (comment)](https://github.com/gavv/httpexpect/pull/220#discussion_r1068125302) ?

yes agree

the update is more into an example, will complete the rest afterwards
neverbeenthisweeb: ah yes, sry i forgot the https://github.com/gavv/httpexpect/pull/220#discussion_r1068125302

i think that idea looks fine

we can have that implemented in this PR
neverbeenthisweeb: @gavv 

Updates:

- Replace `DisableScientific` with `FloatFormat` to support decimal, scientific, and auto float formatting (i'm still unsure of the naming, LMK if you have suggestions)
- Update unit test. We tests:
  - auto with small exponent
  - auto with large exponent
  - decimal with large exponent
  - scientific with small exponent

TODO:

- [ ] Complete unit test with other assertion types (e.g. `formatting of AssertionList with number` as mentioned in https://github.com/gavv/httpexpect/pull/220#issuecomment-1399282552)
neverbeenthisweeb: btw @gavv, for float in decimals why did we prefer:

```
strconv.FormatFloat(*fl, 'f', -1, 64)
```

to?:

```
fmt.Sprintf("%f", value)
```

sry i kinda forget about this one
gavv: Because strconv variant shows full precision, unlike fmt: https://go.dev/play/p/qFly0dwuIYn
gavv: Using `%f`, you can specify arbitrary precision, e.g. `%.6f`. However there is now way to ask "use minimum precision needed to represent fractional part full without loss". And strconv.FormatFloat has this option - we can use precision `-1` for that.
neverbeenthisweeb: > Using `%f`, you can specify arbitrary precision, e.g. `%.6f`. However there is now way to ask "use minimum precision needed to represent fractional part fully without loss". And strconv.FormatFloat has this option - we can use precision `-1` for that.

ok, so is it expected that we use:

- `%g` for auto
- `strconv.FormatFloat` for decimal
- `%e` for scientific?

because it seems a bit misleading, since `%g` fallbacks to `%f` for decimal

but i think it is OK, considering your comments here

https://github.com/gavv/httpexpect/pull/220#discussion_r1085286292

where it says _similar to_
gavv: Good point. Yes, probably not critical, but I wonder if there is a sane way to implement auto using strconv instead of %f for small exponents.
gavv: > Complete unit test with other assertion types (e.g. formatting of AssertionList with number as mentioned in Add DefaultFormatter.DisableScientific

If you wish, we can create a separate issue for this. Let me know if you want to create one.
gavv: > The default precision for %e, %f and %#g is 6; for %g it is the smallest number of digits necessary to identify the value uniquely.

Oh, actually %g already have desired behavior.

So we can just mention in comment for Decimal:

> Similar to %f with precision set to the smallest number of digits necessary to identify the value uniquely
neverbeenthisweeb: > If you wish, we can create a separate issue for this. Let me know if you want to create one.

@gavv ok, let's separate the issue for this one

i will write a note in the #190 (flexible if u think it should be better with a new issue)
neverbeenthisweeb: > So we can keep it as is, just mention in comment for Decimal:

ok, will update the comment
neverbeenthisweeb: this MR is now ready @gavv 

Patch:
diff --git a/formatter.go b/formatter.go
index c02ed9a..2d5c7a5 100644
--- a/formatter.go
+++ b/formatter.go
@@ -21,6 +21,25 @@ type Formatter interface {
 	FormatFailure(*AssertionContext, *AssertionFailure) string
 }
 
+// FloatFormat defines the format for printing float
+type FloatFormat int
+
+const (
+	// Print floats in scientific notation for large exponents,
+	// otherwise print in decimal notation.
+	// Similar to %g format.
+	FloatFormatAuto FloatFormat = iota
+
+	// Always print floats in decimal notation.
+	// Similar to %f format.
+	FloatFormatDecimal
+
+	// Always print floats in scientific notation.
+	// Similar to %f format, with precision set to the smallest number of digits
+	// necessary to identify the value uniquely.
+	FloatFormatScientific
+)
+
 // DefaultFormatter is the default Formatter implementation.
 //
 // DefaultFormatter gathers values from AssertionContext and AssertionFailure,
@@ -45,6 +64,10 @@ type DefaultFormatter struct {
 	// Exclude diff from failure report.
 	DisableDiffs bool
 
+	// Float printing format.
+	// Default is FloatFormatAuto.
+	FloatFormat FloatFormat
+
 	// Wrap text to keep lines below given width.
 	// Use zero for default width, and negative value to disable wrapping.
 	LineWidth int
@@ -411,7 +434,16 @@ func (f *DefaultFormatter) formatTyped(value interface{}) string {
 
 func (f *DefaultFormatter) formatValue(value interface{}) string {
 	if isNumber(value) {
-		return fmt.Sprintf("%v", value)
+		switch f.FloatFormat {
+		case FloatFormatAuto:
+			return fmt.Sprintf("%v", value)
+		case FloatFormatDecimal:
+			return fmt.Sprintf("%f", value)
+		case FloatFormatScientific:
+			return fmt.Sprintf("%e", value)
+		default:
+			panic("invalid float format")
+		}
 	}
 
 	if !isNil(value) && !isHTTP(value) {
@@ -536,6 +568,18 @@ func extractList(value interface{}) *AssertionList {
 	}
 }
 
+func extractFloat(value interface{}) *float64 {
+	switch f := value.(type) {
+	case float32:
+		ff := float64(f)
+		return &ff
+	case float64:
+		return &f
+	default:
+		return nil
+	}
+}
+
 func isNil(value interface{}) bool {
 	defer func() {
 		_ = recover()


Test patch:
diff --git a/formatter_test.go b/formatter_test.go
index 8955649..d80d673 100644
--- a/formatter_test.go
+++ b/formatter_test.go
@@ -216,6 +216,7 @@ func TestFormat_FailureExpected(t *testing.T) {
 		name             string
 		assertionType    AssertionType
 		assertionValue   interface{}
+		formatter        DefaultFormatter
 		wantHaveExpected bool
 		wantExpectedKind string
 		wantExpected     []string
@@ -409,7 +410,6 @@ func TestFormat_FailureExpected(t *testing.T) {
 		},
 	}
 
-	df := &DefaultFormatter{}
 	ctx := &AssertionContext{}
 
 	for _, tc := range tests {
@@ -420,7 +420,7 @@ func TestFormat_FailureExpected(t *testing.T) {
 					Value: tc.assertionValue,
 				},
 			}
-			fd := df.buildFormatData(ctx, fl)
+			fd := tc.formatter.buildFormatData(ctx, fl)
 			assert.Equal(t, tc.wantHaveExpected, fd.HaveExpected)
 			assert.Equal(t, tc.wantExpectedKind, fd.ExpectedKind)
 			assert.Equal(t, tc.wantExpected, fd.Expected)
@@ -551,3 +551,262 @@ func TestFormat_FailureDelta(t *testing.T) {
 		})
 	}
 }
+
+func TestFormatter_FloatFormat(t *testing.T) {
+	type testCase struct {
+		name             string
+		assertionFailure AssertionFailure
+		formatter        DefaultFormatter
+		wantExpected     []string
+		wantDelta        string
+	}
+
+	testCases := []testCase{
+		{
+			name: "AssertInRange float32 auto small exponent",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatAuto,
+			},
+			assertionFailure: AssertionFailure{
+				Type: AssertInRange,
+				Expected: &AssertionValue{
+					Value: AssertionRange{
+						Min: float32(-1.23456789),
+						Max: float32(1.23456789),
+					},
+				},
+			},
+			wantExpected: []string{"[-1.2345679; 1.2345679]"},
+		},
+		{
+			name: "AssertInRange float32 auto large exponent",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatAuto,
+			},
+			assertionFailure: AssertionFailure{
+				Type: AssertInRange,
+				Expected: &AssertionValue{
+					Value: AssertionRange{
+						Min: float32(-12345678.9),
+						Max: float32(12345678.9),
+					},
+				},
+			},
+			wantExpected: []string{"[-1.2345679e+07; 1.2345679e+07]"},
+		},
+		{
+			name: "AssertInRange float32 decimal",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatDecimal,
+			},
+			assertionFailure: AssertionFailure{
+				Type: AssertInRange,
+				Expected: &AssertionValue{
+					Value: AssertionRange{
+						Min: float32(-12345678.9),
+						Max: float32(12345678.9),
+					},
+				},
+			},
+			wantExpected: []string{"[-12345679.000000; 12345679.000000]"},
+		},
+		{
+			name: "AssertInRange float32 scientific",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatScientific,
+			},
+			assertionFailure: AssertionFailure{
+				Type: AssertInRange,
+				Expected: &AssertionValue{
+					Value: AssertionRange{
+						Min: float32(-1.23456789),
+						Max: float32(1.23456789),
+					},
+				},
+			},
+			wantExpected: []string{"[-1.234568e+00; 1.234568e+00]"},
+		},
+		{
+			name: "AssertInRange float64 auto small exponent",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatAuto,
+			},
+			assertionFailure: AssertionFailure{
+				Type: AssertInRange,
+				Expected: &AssertionValue{
+					Value: AssertionRange{
+						Min: float64(-1.23456789),
+						Max: float64(1.23456789),
+					},
+				},
+			},
+			wantExpected: []string{"[-1.23456789; 1.23456789]"},
+		},
+		{
+			name: "AssertInRange float64 auto large exponent",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatAuto,
+			},
+			assertionFailure: AssertionFailure{
+				Type: AssertInRange,
+				Expected: &AssertionValue{
+					Value: AssertionRange{
+						Min: float64(-12345678.9),
+						Max: float64(12345678.9),
+					},
+				},
+			},
+			wantExpected: []string{"[-1.23456789e+07; 1.23456789e+07]"},
+		},
+		{
+			name: "AssertInRange float64 decimal",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatDecimal,
+			},
+			assertionFailure: AssertionFailure{
+				Type: AssertInRange,
+				Expected: &AssertionValue{
+					Value: AssertionRange{
+						Min: float64(-12345678.9),
+						Max: float64(12345678.9),
+					},
+				},
+			},
+			wantExpected: []string{"[-12345678.900000; 12345678.900000]"}},
+		{
+			name: "AssertInRange float64 scientific",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatScientific,
+			},
+			assertionFailure: AssertionFailure{
+				Type: AssertInRange,
+				Expected: &AssertionValue{
+					Value: AssertionRange{
+						Min: float64(-1.23456789),
+						Max: float64(1.23456789),
+					},
+				},
+			},
+			wantExpected: []string{"[-1.234568e+00; 1.234568e+00]"},
+		},
+		{
+			name: "Delta float32 auto small exponent",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatAuto,
+			},
+			assertionFailure: AssertionFailure{
+				Delta: &AssertionValue{
+					Value: float32(1.23456789),
+				},
+			},
+			wantDelta: "1.2345679",
+		},
+		{
+			name: "Delta float32 auto large exponent",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatAuto,
+			},
+			assertionFailure: AssertionFailure{
+				Delta: &AssertionValue{
+					Value: float32(12345678.9),
+				},
+			},
+			wantDelta: "1.2345679e+07",
+		},
+		{
+			name: "Delta float32 decimal",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatDecimal,
+			},
+			assertionFailure: AssertionFailure{
+				Delta: &AssertionValue{
+					Value: float32(12345678.9),
+				},
+			},
+			wantDelta: "12345679.000000",
+		},
+		{
+			name: "Delta float32 scientific",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatScientific,
+			},
+			assertionFailure: AssertionFailure{
+				Delta: &AssertionValue{
+					Value: float32(1.23456789),
+				},
+			},
+			wantDelta: "1.234568e+00",
+		},
+		{
+			name: "Delta float64 auto small exponent",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatAuto,
+			},
+			assertionFailure: AssertionFailure{
+				Delta: &AssertionValue{
+					Value: float64(1.23456789),
+				},
+			},
+			wantDelta: "1.23456789",
+		},
+		{
+			name: "Delta float64 auto large exponent",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatAuto,
+			},
+			assertionFailure: AssertionFailure{
+				Delta: &AssertionValue{
+					Value: float64(12345678.9),
+				},
+			},
+			wantDelta: "1.23456789e+07",
+		},
+		{
+			name: "Delta float64 decimal",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatDecimal,
+			},
+			assertionFailure: AssertionFailure{
+				Delta: &AssertionValue{
+					Value: float64(12345678.9),
+				},
+			},
+			wantDelta: "12345678.900000",
+		},
+		{
+			name: "Delta float64 scientific",
+			formatter: DefaultFormatter{
+				FloatFormat: FloatFormatScientific,
+			},
+			assertionFailure: AssertionFailure{
+				Delta: &AssertionValue{
+					Value: float64(1.23456789),
+				},
+			},
+			wantDelta: "1.234568e+00",
+		},
+	}
+
+	assertFn := func(t *testing.T, tc testCase, fd *FormatData) {
+		if len(tc.wantExpected) > 0 {
+			assert.Equal(t, tc.wantExpected, fd.Expected)
+			return
+		}
+
+		if tc.wantDelta != "" {
+			assert.Equal(t, tc.wantDelta, fd.Delta)
+			return
+		}
+
+		panic("expectation is not set")
+	}
+
+	ctx := &AssertionContext{}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fd := tc.formatter.buildFormatData(ctx, &tc.assertionFailure)
+			assertFn(t, tc, fd)
+		})
+	}
+}