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

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

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

cfergeau
Copy link
Collaborator

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).

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).

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Copy link
Contributor

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

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.

Copy link

openshift-ci bot commented Feb 22, 2024

@BlackHole1: changing LGTM is restricted to collaborators

In response to this:

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cfergeau
Copy link
Collaborator Author

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
Copy link
Contributor

BlackHole1 commented Feb 22, 2024

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
Copy link
Member

/lgtm
/approve

@cfergeau
Copy link
Collaborator Author

cfergeau commented Mar 5, 2024

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

Copy link

openshift-ci bot commented Mar 5, 2024

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Mar 5, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit f7a5e92 into crc-org:main Mar 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants