Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print underlying object in ptr comparison #1287

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
54 changes: 49 additions & 5 deletions assert/assertions.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,28 @@ type ErrorAssertionFunc func(TestingT, error, ...interface{}) bool
// Comparison is a custom function that returns true on success and false on failure
type Comparison func() (success bool)

// List of basic types. This is specifically a list of types whose underlying value is not printed
// when using the %#v format specifier to print a pointer to the value.
var basicTypes = []reflect.Kind{
reflect.Int,
reflect.Int8,
reflect.Int16,
reflect.Int32,
reflect.Int64,
reflect.Uint,
reflect.Uint8,
reflect.Uint16,
reflect.Uint32,
reflect.Uint64,
reflect.Uintptr,
reflect.Float32,
reflect.Float64,
reflect.Complex64,
reflect.Complex128,
reflect.String,
reflect.Bool,
}

/*
Helper functions
*/
Expand Down Expand Up @@ -512,22 +534,44 @@ func samePointers(first, second interface{}) bool {
// to a type conversion in the Go grammar.
func formatUnequalValues(expected, actual interface{}) (e string, a string) {
if reflect.TypeOf(expected) != reflect.TypeOf(actual) {
return fmt.Sprintf("%T(%s)", expected, truncatingFormat(expected)),
fmt.Sprintf("%T(%s)", actual, truncatingFormat(actual))
return fmt.Sprintf("%T(%s)", expected, truncatingFormat(expected, false)),
fmt.Sprintf("%T(%s)", actual, truncatingFormat(actual, false))
}
switch expected.(type) {
case time.Duration:
return fmt.Sprintf("%v", expected), fmt.Sprintf("%v", actual)
}
return truncatingFormat(expected), truncatingFormat(actual)

return truncatingFormat(expected, true), truncatingFormat(actual, true)
}

// truncatingFormat formats the data and truncates it if it's too long.
//
// This helps keep formatted error messages lines from exceeding the
// bufio.MaxScanTokenSize max line length that the go testing framework imposes.
func truncatingFormat(data interface{}) string {
value := fmt.Sprintf("%#v", data)
//
// If the `printValueOfBasicTypes` flag is set to true, the underlying value of
// pointers of basic types (see `basicTypes` for list of basic types) will be
// returned in addition to the pointer address. For non-basic types, only the pointer
// address will be returned.
// If the `printValueOfBasicTypes` flag is set to false, only the pointer address
// will be returned, regardless of whether `data` is a basic type or not.
func truncatingFormat(data interface{}, printValueOfBasicTypes bool) string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be neater if we just moved the logic for checking whether to print the value into this function rather than passing it in as an argument at every call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for checking whether to print the value is already inside the truncatingFormat function (when it loops through the basicTypes list and checks if the type of data matches any of the types in the basicTypes list).

The use of the printValueOfBasicTypes is an override, where if it is set to false, only the pointer address (and not the underlying value) is returned, regardless of whether it is a basic type or not. The use case of this is in the formatUnequalValues function where two pointers of different types are being compared. In this case, we only want the pointer addresses to be printed and not the underlying values (even if the underlying values are basic types). This emphasizes the fact that the pointers are different types (which is why the comparison failed) and that the underlying value of the pointers do not matter for the comparison.

Also updated the comments for the truncatingFormat function to better explain the use of the printValueOfBasicTypes

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @boyan-soubachov here, don't use a boolean argument to hide a new function inside an exising one.

  • Put line 561 to 573 in a different function (maybe call it truncatingFormatValues).
  • Put line 575 to 578 in another new function (maybe called truncateString).
  • Both truncatingFormat and truncatingFormatValues can end with:
    return truncateString(value)

var value string
if printValueOfBasicTypes && reflect.ValueOf(data).Kind() == reflect.Ptr {
v := reflect.ValueOf(data).Elem()
for _, t := range basicTypes {
if v.Kind() == t {
value = fmt.Sprintf("%#v %v", data, v.Interface())
break
}
}
Comment on lines +563 to +568
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against your basicTypes but this could be written as:

Suggested change
for _, t := range basicTypes {
if v.Kind() == t {
value = fmt.Sprintf("%#v %v", data, v.Interface())
break
}
}
if isBasicType(v.Kind()) {
value = fmt.Sprintf("%#v %v", data, v.Interface())
}

With:

func isBasicType(k reflect.Kind) bool {
	return k == reflect.String || (k >= reflect.Bool && k <= reflect.Complex128)
}

Rather than iterating.

}

if value == "" {
value = fmt.Sprintf("%#v", data)
}

max := bufio.MaxScanTokenSize - 100 // Give us some space the type info too if needed.
if len(value) > max {
value = value[0:max] + "<... truncated>"
Expand Down
27 changes: 25 additions & 2 deletions assert/assertions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2896,18 +2896,41 @@ func Test_validateEqualArgs(t *testing.T) {
func Test_truncatingFormat(t *testing.T) {

original := strings.Repeat("a", bufio.MaxScanTokenSize-102)
result := truncatingFormat(original)
result := truncatingFormat(original, false)
Equal(t, fmt.Sprintf("%#v", original), result, "string should not be truncated")

original = original + "x"
result = truncatingFormat(original)
result = truncatingFormat(original, false)
NotEqual(t, fmt.Sprintf("%#v", original), result, "string should have been truncated.")

if !strings.HasSuffix(result, "<... truncated>") {
t.Error("truncated string should have <... truncated> suffix")
}
}

func Test_truncatingFormat_PrintValueOfBasicTypes(t *testing.T) {
i := 5
iPtr := &i
result := truncatingFormat(iPtr, true)
if !strings.HasSuffix(result, "5") {
t.Error("format should have printed the underlying value of int pointer")
}

b := false
bPtr := &b
result = truncatingFormat(bPtr, true)
if !strings.HasSuffix(result, "false") {
t.Error("format should have printed the underlying value of bool pointer")
}

s := "testingString"
sPtr := &s
result = truncatingFormat(sPtr, true)
if !strings.HasSuffix(result, "testingString") {
t.Error("format should have printed the underlying value of string pointer")
}
}

// parseLabeledOutput does the inverse of labeledOutput - it takes a formatted
// output string and turns it back into a slice of labeledContent.
func parseLabeledOutput(output string) []labeledContent {
Expand Down
Loading