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

Support structs in assert.Contains() #1432

Closed
qualidafial opened this issue Jul 21, 2023 · 6 comments
Closed

Support structs in assert.Contains() #1432

qualidafial opened this issue Jul 21, 2023 · 6 comments
Labels
enhancement pkg-assert Change related to package testify/assert wontfix

Comments

@qualidafial
Copy link

qualidafial commented Jul 21, 2023

Often large objects cannot be compared using assert.Equal(), because they can contain extra data that cannot be predicted in a test. For example, servers may assign randomly generated UUIDs to created objects, or set CreatedAt or UpdatedAt fields at the server.

toCreate := Contact{
  Name: "Jenny",
  Numbers []Number{
    {
      Number: "8675309",
      Type: "Mobile",
    },
  },
  Addresses []Address{
    // etc
  }
}

beforeCreate := time.Now()
created, err := client.CreateContact(ctx, toCreate)
afterCreate := time.Now()
require.NoError(t, err)

assert.NotEmpty(t, created.ID)
assert.WithinRange(t, created.CreatedAt, beforeCreate, afterCreate)
assert.WithinRange(t, created.UpdatedAt, beforeCreate, afterCreate)

assert.Equal(t, toCreate.Name, created.Name)
assert.Len(t, created.Numbers, 1)
assert.Equal(t, "8675309", created.Numbers[0].Number)
assert.Equal(t, "Mobile", created.Numbers[0].Type)
// etc

When objects contain this extra data, often the test must compare each field one at a time. The assert.Equal() calls above become tedious to write when there are a lot of fields. They are also vulnerable to copy-paste errors, at least in my own experience.

It would be helpful (and easier to read) if we could check all the predictable fields in one shot:

assert.Contains(t, created, toCreate)

The way I envision this working is that when the object is a struct:

  • Assert the structs are the same type
  • For each field in the expected struct with a non-zero value:
    • Slice fields: assert the slices are the same length, and perform a Contains comparison on corresponding elements.
    • Map fields: for each key in the expected map, assert that the actual map contains that key, and perform a Contains comparison on corresponding values.
    • Struct or struct pointer fields: assert the struct types are the same, and perform a Contains comparison of the corresponding struct fields
    • All other field types (including strings): assert the fields values are equal.
@qualidafial
Copy link
Author

I would be willing to contribute the work for this, if there is interest from the testify team.

@dolmen dolmen added enhancement pkg-assert Change related to package testify/assert labels Jul 22, 2023
@dolmen
Copy link
Collaborator

dolmen commented Jul 22, 2023

I don't understand how your specification would fit your use case. UUID would be of string type and create/update timestamp would be time.Time so they would be compared with strict equality.

Edit: ok, the expected struct would have the zero value for those fields so they would be skipped.

@brackendawson
Copy link
Collaborator

brackendawson commented Jul 23, 2023

I feel like this is a risky way to solve the problem. The addition of fields to the struct in the future of the package could nullify the assertion. Given you know what field it is going to be in, you should test that field.

The proposed behaviour is very difficult to document and I don't think it represents the job of the Contains asserition. If terseness is what you are looking for, the solution that has been recommended many tiemes before is as simple: #843 (comment) just zero the unexpectable fields on your actual before using Equals.

@qualidafial
Copy link
Author

I feel like this is a risky way to solve the problem. The addition of fields to the struct in the future of the package could nullify the assertion. Given you know what field it is going to be in, you should test that field.

That depends a lot on one's testing philosophy. In general I agree that if you expect the system to behave a certain way, you should test that expectation.

However I also follow the philosophy that backward-compatible changes should not break existing tests. Tests that violate this rule are called change detectors, and make developers lives miserable.

Adding a new optional or computed field to an API is a backward-compatible change. I may want to update existing tests to add new assertions, or I may want to add new tests for that.

The assert.Contains approach allows me to opt-in to testing the specific fields I'm interested in. Zeroing fields and using assert.Equal imposes an opt-out approach, making the test a change detector.

The proposed behaviour is very difficult to document

I've written similar assertions in the past, and would contribute the documentation with the implementation.

and I don't think it represents the job of the Contains assertion.

Would you prefer if this lived in its own function, e.g. assert.StructContains?

@dolmen
Copy link
Collaborator

dolmen commented Jul 25, 2023

I don't think this is core functionality that we need to have (and maintain forever) in testify. The comparison could be published in a separate package.

As @brackendawson said, there is a risk of misuse. As maintainers we can't take the load of questions coming. I'm sorry if this feels very conservative, but it really is because testify is very popular.

@qualidafial
Copy link
Author

It sounds like the testify team isn't interested in this as a feature. Closing.

@qualidafial qualidafial closed this as not planned Won't fix, can't repro, duplicate, stale Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement pkg-assert Change related to package testify/assert wontfix
Projects
None yet
Development

No branches or pull requests

3 participants