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.

Sign up for GitHub

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

Jump to bottom

testing: protobuf testing improvements #2786

Closed
youngnick opened this issue Aug 12, 2020 · 1 comment
Closed

testing: protobuf testing improvements #2786

youngnick opened this issue Aug 12, 2020 · 1 comment
Assignees
@youngnick
Labels
area/testing Issues or PRs related to tests or testing tools.

Comments

@youngnick
Copy link
Member

As part of testing for #2134 in #2604, @stevesloka identified that we need a better way to compare protobufs. The current internal/assert package has a number of workarounds using go-cmp/cmp in order to be able to test Envoy protobufs, particularly in the featuretests package, but as we move towards #2134 and go-control-plane, we need some more flexibility.

This is to investigate using other options like testify's assert or protocmp for comparing protobufs.
@youngnick youngnick added the area/testing Issues or PRs related to tests or testing tools. label Aug 12, 2020
@youngnick youngnick self-assigned this Aug 12, 2020
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 12, 2020
internal/assert: Better protobuf diffs by aliasing testify/assert
5757f8e 
This change replaces the current `github.com/google/go-cmp/cmp` based internal `assert.Equal` with an aliased version of testify `assert.Equal`.
If we need other assertions, we'll need to
This allows better output for non-protobuf tests.

Unfortunately, testify does not have support for unmarshaling protobufs, so I needed to move to a new `assert.EqualProto` function that uses `protocmp` to compare the two protobufs.

This also exposed another issue that the old `assert.Equal` was working around: the Envoy DiscoveryResponse proto has version and nonce fields that can change in ways that make tests flaky.
However, it turns out that all the tests in `featuretests` that needed the workaround only change the `Resources` field.
So I've updated the featuretests infrastructure to have the `Equals` method on the Response struct for testing only compare that `Resources` field.
This should probably be replaced with `EqualResources` and `NoResources` for the normal and empty case respectively, but this PR is big enough for now.

Updates projectcontour#2786 for now, until a second PR that will change the names as above.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 12, 2020
internal/assert: Better protobuf diffs by aliasing testify/assert
10dae2e 
This change replaces the current `github.com/google/go-cmp/cmp` based internal `assert.Equal` with an aliased version of testify `assert.Equal`.
If we need other assertions, we'll need to
This allows better output for non-protobuf tests.

Unfortunately, testify does not have support for unmarshaling protobufs, so I needed to move to a new `assert.EqualProto` function that uses `protocmp` to compare the two protobufs.

This also exposed another issue that the old `assert.Equal` was working around: the Envoy DiscoveryResponse proto has version and nonce fields that can change in ways that make tests flaky.
However, it turns out that all the tests in `featuretests` that needed the workaround only change the `Resources` field.
So I've updated the featuretests infrastructure to have the `Equals` method on the Response struct for testing only compare that `Resources` field.
This should probably be replaced with `EqualResources` and `NoResources` for the normal and empty case respectively, but this PR is big enough for now.

Updates projectcontour#2786 for now, until a second PR that will change the names as above.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 12, 2020
internal/assert: Better protobuf diffs by aliasing testify/assert
7b09f74 
This change replaces the current `github.com/google/go-cmp/cmp` based internal `assert.Equal` with an aliased version of testify `assert.Equal`.
If we need other assertions, we'll need to
This allows better output for non-protobuf tests.
However, it does mean that now Error message text is being checked - there were about four places I needed to fix as a result.

Unfortunately, testify does not have support for unmarshaling protobufs, so I needed to move to a new `assert.EqualProto` function that uses `protocmp` to compare the two protobufs.

This also exposed another issue that the old `assert.Equal` was working around: the Envoy DiscoveryResponse proto has version and nonce fields that can change in ways that make tests flaky.
However, it turns out that all the tests in `featuretests` that needed the workaround only change the `Resources` field.
So I've updated the featuretests infrastructure to have the `Equals` method on the Response struct for testing only compare that `Resources` field.
This should probably be replaced with `EqualResources` and `NoResources` for the normal and empty case respectively, but this PR is big enough for now.

Updates projectcontour#2786 for now, until a second PR that will change the names as above.

Signed-off-by: Nick Young <ynick@vmware.com>
@youngnick youngnick mentioned this issue Aug 12, 2020
Merged
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 12, 2020
internal/assert: Better protobuf diffs by aliasing testify/assert
c82153a 
This change replaces the current `github.com/google/go-cmp/cmp` based internal
`assert.Equal` with an aliased version of testify `assert.Equal`. If we need
other assertions, we'll need to add them one by one. This allows better output
for non-protobuf tests. However, it does mean that now Error message text is
being checked - there were about four places I needed to fix as a result.

Unfortunately, testify does not have support for unmarshaling protobufs, so
I needed to move to a new `assert.EqualProto` function that uses `protocmp`
to compare the two protobufs.

This also exposed another issue that the old `assert.Equal` was working around:
the Envoy DiscoveryResponse proto has version and nonce fields that can change in
ways that make tests flaky. However, it turns out that all the tests in `featuretests`
that needed the workaround only change the `Resources` field. So I've updated the
featuretests infrastructure to have the `Equals` method on the Response struct
for testing only compare that `Resources` field. This should probably be replaced
with `EqualResources` and `NoResources` for the normal and empty case respectively,
but this PR is big enough for now.

Updates projectcontour#2786 for now, until a second PR that will change the names as above.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 13, 2020
internal/assert: Better protobuf diffs by aliasing testify/assert
d04e34a 
This change replaces the current `github.com/google/go-cmp/cmp` based internal
`assert.Equal` with an aliased version of testify `assert.Equal`. If we need
other assertions, we'll need to add them one by one. This allows better output
for non-protobuf tests. However, it does mean that now Error message text is
being checked - there were about four places I needed to fix as a result.

Unfortunately, testify does not have support for unmarshaling protobufs, so
I needed to move to a new `assert.EqualProto` function that uses `protocmp`
to compare the two protobufs.

This also exposed another issue that the old `assert.Equal` was working around:
the Envoy DiscoveryResponse proto has version and nonce fields that can change in
ways that make tests flaky. However, it turns out that all the tests in `featuretests`
that needed the workaround only change the `Resources` field. So I've updated the
featuretests infrastructure to have the `Equals` method on the Response struct
for testing only compare that `Resources` field. This should probably be replaced
with `EqualResources` and `NoResources` for the normal and empty case respectively,
but this PR is big enough for now.

Updates projectcontour#2786 for now, until a second PR that will change the names as above.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit that referenced this issue Aug 13, 2020
internal/assert: Better protobuf diffs by aliasing testify/assert (#2787
057fdc0 
)

This change replaces the current `github.com/google/go-cmp/cmp` based internal
`assert.Equal` with an aliased version of testify `assert.Equal`. If we need
other assertions, we'll need to add them one by one. This allows better output
for non-protobuf tests. However, it does mean that now Error message text is
being checked - there were about four places I needed to fix as a result.

Unfortunately, testify does not have support for unmarshaling protobufs, so
I needed to move to a new `assert.EqualProto` function that uses `protocmp`
to compare the two protobufs.

This also exposed another issue that the old `assert.Equal` was working around:
the Envoy DiscoveryResponse proto has version and nonce fields that can change in
ways that make tests flaky. However, it turns out that all the tests in `featuretests`
that needed the workaround only change the `Resources` field. So I've updated the
featuretests infrastructure to have the `Equals` method on the Response struct
for testing only compare that `Resources` field. This should probably be replaced
with `EqualResources` and `NoResources` for the normal and empty case respectively,
but this PR is big enough for now.

The `internal/assert` library will now behave the same as the testify `assert`:
failures will produce t.Error() output, but will not be fatal.

The feature tests have been updated so that the `Equals()` method on the testing
Response struct is still fatal; this replicates the existing behavior.

Updates #2786 for now, until a second PR that will change the names as above.


Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 20, 2020
testing: Migrate as many tests as possible to internal/assert
292b4c0 
Needed to update some aliases in internal/assert to cover all the cases.

Updates projectcontour#2786.

Signed-off-by: Nick Young <ynick@vmware.com>
@youngnick youngnick mentioned this issue Aug 20, 2020
Merged
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 20, 2020
testing: Migrate as many tests as possible to internal/assert
c58a9a1 
Needed to update some aliases in internal/assert to cover all the cases.

Updates projectcontour#2786.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 24, 2020
testing: Migrate as many tests as possible to internal/assert
800b1e7 
Needed to update some aliases in internal/assert to cover all the cases.

Updates projectcontour#2786.

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 24, 2020
testing: Migrate more things to testify
b17bf92 
So, in this commit, I have goals:
- Move `assert.EqualProto` into the `internal/protobuf` package.
- Make `protobuf.ExpectEqual` and `protobuf.RequireEqual` work similarly
to testify `assert` and `require` respectively.
- Replace any remaining `cmp.Diff` calls in tests with `assert.Equal`
- Check all the tests in `internal/envoy` for protobufs and migrate any
remaining to `protobuf.ExpectEqual`.

Sorry, it's a big PR, but there's a lot of cleanup here.

Updates projectcontour#2786

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 24, 2020
testing: Migrate more things to testify
8e69c79 
So, in this commit, I have four goals:
- Remove the `internal/assert` package.
- Move `assert.EqualProto` into the `internal/protobuf` package.
- Make `protobuf.ExpectEqual` and `protobuf.RequireEqual` work similarly
to testify `assert` and `require` respectively.
- Replace any remaining `cmp.Diff` calls in tests with `assert.Equal`
- Check all the tests in `internal/envoy` for protobufs and migrate any
remaining to `protobuf.ExpectEqual`.

Sorry, it's a big PR, but there's a lot of cleanup here.

Updates projectcontour#2786

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 24, 2020
testing: Migrate more things to testify
0b1c21d 
So, in this commit, I have four goals:
- Remove the `internal/assert` package.
- Move `assert.EqualProto` into the `internal/protobuf` package.
- Make `protobuf.ExpectEqual` and `protobuf.RequireEqual` work similarly
to testify `assert` and `require` respectively.
- Replace any remaining `cmp.Diff` calls in tests with `assert.Equal`
- Check all the tests in `internal/envoy` for protobufs and migrate any
remaining to `protobuf.ExpectEqual`.

Sorry, it's a big PR, but there's a lot of cleanup here.

Updates projectcontour#2786

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 24, 2020
testing: Migrate more things to testify
db32587 
So, in this commit, I have four goals:
- Remove the `internal/assert` package.
- Move `assert.EqualProto` into the `internal/protobuf` package.
- Make `protobuf.ExpectEqual` and `protobuf.RequireEqual` work similarly
to testify `assert` and `require` respectively.
- Replace any remaining `cmp.Diff` calls in tests with `assert.Equal`
- Check all the tests in `internal/envoy` for protobufs and migrate any
remaining to `protobuf.ExpectEqual`.

Sorry, it's a big PR, but there's a lot of cleanup here.

Updates projectcontour#2786

Signed-off-by: Nick Young <ynick@vmware.com>
@youngnick youngnick mentioned this issue Aug 24, 2020
Merged
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 24, 2020
testing: Migrate more things to testify
0dab126 
So, in this commit, I have four goals:
- Remove the `internal/assert` package.
- Move `assert.EqualProto` into the `internal/protobuf` package.
- Make `protobuf.ExpectEqual` and `protobuf.RequireEqual` work similarly
to testify `assert` and `require` respectively.
- Replace any remaining `cmp.Diff` calls in tests with `assert.Equal`
- Check all the tests in `internal/envoy` for protobufs and migrate any
remaining to `protobuf.ExpectEqual`.

Sorry, it's a big PR, but there's a lot of cleanup here.

Updates projectcontour#2786

Signed-off-by: Nick Young <ynick@vmware.com>
youngnick pushed a commit to youngnick/contour that referenced this issue Aug 24, 2020
testing: Migrate more things to testify
b174e21 
So, in this commit, I have four goals:
- Remove the `internal/assert` package.
- Move `assert.EqualProto` into the `internal/protobuf` package.
- Make `protobuf.ExpectEqual` and `protobuf.RequireEqual` work similarly
to testify `assert` and `require` respectively.
- Replace any remaining `cmp.Diff` calls in tests with `assert.Equal`
- Check all the tests in `internal/envoy` for protobufs and migrate any
remaining to `protobuf.ExpectEqual`.

Sorry, it's a big PR, but there's a lot of cleanup here.

Updates projectcontour#2786

Signed-off-by: Nick Young <ynick@vmware.com>
@youngnick
Copy link
Member Author

Okay, reviewing this one, I think I'm going to call it done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@youngnick youngnick

Labels
area/testing Issues or PRs related to tests or testing tools.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@youngnick