Improve message unit-tests #48

New issue

Open

opened 2023-07-20 07:56:08 +00:00 by aarifullin · 5 comments

aarifullin commented

2023-07-20 07:56:08 +00:00

Member

We use v2 types like this that can be converted to grpc or from grpc.

To check unmarshalling from/marshalling to protobuf or json formats we use message_test.go on generated instances.

The problem is that we may forget to set new added field within Generate* method and as the result - the bug cannot be revealed.

We need to reconsider the way we generate v2 type instances to make sure that new added fields are set and unit-tests pass well

We use `v2` types like [this](https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/branch/master/netmap/types.go#L57) that can be converted [to grpc](https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/branch/master/netmap/types.go#L191) or [from grpc](https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/branch/master/netmap/types.go#L207). To check unmarshalling from/marshalling to protobuf or json formats we use [message_test.go](https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/branch/master/netmap/message_test.go#L11) on generated instances. The problem is that we may forget to set new added field within `Generate*` method and as the result - the bug cannot be [revealed](https://git.frostfs.info/TrueCloudLab/frostfs-api-go/pulls/47/files). We need to reconsider the way we generate `v2` type instances to make sure that new added fields are set and unit-tests pass well

fyrchik commented

2023-07-20 08:16:28 +00:00

Owner

This could be useful https://pkg.go.dev/testing/quick#Value

ale64bit commented

2023-07-20 08:22:12 +00:00

Member

(there's also https://pkg.go.dev/pgregory.net/rapid, which is supposed to be better and it's not frozen)

edit: but I don't see how property-based testing helps much with this issue. Seems to be a problem of proper automation rather than bug discovery.

(there's also https://pkg.go.dev/pgregory.net/rapid, which is supposed to be better and it's not frozen) edit: but I don't see how property-based testing helps much with this issue. Seems to be a problem of proper automation rather than bug discovery.

fyrchik commented

2023-07-20 08:39:50 +00:00

Owner

We added a new field in the protobuf, we check To -> From grpc roundtrip.
Here we a also forgot to add a generator for the value, so if it could've been set automatically (at random), the bug would've been catched.

We added a new field in the protobuf, we check `To -> From` grpc roundtrip. Here we a also forgot to add a generator for the value, so if it could've been set automatically (at random), the bug would've been catched.

ale64bit commented

2023-07-20 08:50:27 +00:00

Member

We added a new field in the protobuf, we check To -> From grpc roundtrip.
Here we a also forgot to add a generator for the value, so if it could've been set automatically (at random), the bug would've been catched.

Seems a lot easier to me to e.g. lint it, use proto reflection, or automate/detect it in other ways. Property-based testing seems like a huge overkill for this and it's not even guaranteed to find it.

> We added a new field in the protobuf, we check `To -> From` grpc roundtrip. > Here we a also forgot to add a generator for the value, so if it could've been set automatically (at random), the bug would've been catched. Seems a lot easier to me to e.g. lint it, use proto reflection, or automate/detect it in other ways. Property-based testing seems like a huge overkill for this and it's not even guaranteed to find it.

👍 1

fyrchik commented

2023-08-21 10:00:18 +00:00

Owner

there's also https://pkg.go.dev/pgregory.net/rapid

It looks quite powerful at the first glance, especially with MakeFuzz
We also currently do not fuzz our StableMarshal (we should not panic on any message and Marshal() -> StableMarshal() -> Marshal() roundtrip should be a no-op).

>there's also https://pkg.go.dev/pgregory.net/rapid It looks quite powerful at the first glance, especially with `MakeFuzz` We also currently do not fuzz our `StableMarshal` (we should not panic on any message and `Marshal()` -> `StableMarshal()` -> `Marshal()` roundtrip should be a no-op).