search

crc-org / vfkit

Apache License 2.0
119 stars 23 forks source link

json: test: Add 'stability' JSON test #103

Closed cfergeau closed 6 months ago

cfergeau commented 6 months ago

We don't want the JSON serialization of pkg/config/ data structures to change between releases. This commit makes use of the reflect package to test the JSON serialization of as many fields as possible. If one of these field change, the test will make us aware of it. We still need to explicitly list the types to be tested (ie the types which can be serialized to JSON).

openshift-ci[bot] commented 6 months ago

@BlackHole1: changing LGTM is restricted to collaborators

In response to [this](https://github.com/crc-org/vfkit/pull/103#pullrequestreview-1895829742): >Code LGTM. >The only concern I have is that if we add new devices in the future, we need to make sure to add corresponding test cases in `jsonStabilityTests`. There is a possibility that we might forget to do so. Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
cfergeau commented 6 months ago

The only concern I have is that if we add new devices in the future, we need to make sure to add corresponding test cases in jsonStabilityTests. There is a possibility that we might forget to do so.

Yes, hopefully this won't happen too often. I've tried to find a programmatic way of listing all the types defined in pkg/config, but this does not seem trivial unfortunately. I suspect something is doable with https://pkg.go.dev/go/types and maybe https://pkg.go.dev/go/importer.

BlackHole1 commented 6 months ago

I suspect something is doable with https://pkg.go.dev/go/types and maybe https://pkg.go.dev/go/importer.

It seems a bit complicated, we can consider putting it on hold for now and reevaluate if we encounter this issue in the future.

praveenkumar commented 6 months ago

/lgtm /approve

cfergeau commented 6 months ago

/approve (adding approved label after Kevin's and Praveen's reviews)

openshift-ci[bot] commented 6 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BlackHole1, cfergeau, praveenkumar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/crc-org/vfkit/blob/main/OWNERS)~~ [cfergeau] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment