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
Detect and clean up unneeded after_roundtrip fixtures #116384
Conversation
Skipping CI for Draft Pull Request. |
@@ -364,6 +364,12 @@ func writeFile(t *testing.T, dir string, gvk schema.GroupVersionKind, suffix, ex | |||
} | |||
} | |||
|
|||
func deleteFile(t *testing.T, dir string, gvk schema.GroupVersionKind, suffix, extension string) { | |||
if err := os.Remove(filepath.Join(dir, makeName(gvk)+suffix+"."+extension)); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File not found errors should probably be tolerated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh, we only call this if the file exists and we want to remove ... not sure I care
expectedJSONAfterRoundTrip, expectedYAMLAfterRoundTrip, expectedProtoAfterRoundTrip, _ := read(previousVersionDir, gvk, ".after_roundtrip", usedFiles) | ||
if len(expectedJSONAfterRoundTrip) == 0 { | ||
expectedJSONAfterRoundTrip = jsonBeforeRoundTrip | ||
} else if bytes.Equal(jsonBeforeRoundTrip, expectedJSONAfterRoundTrip) { | ||
t.Errorf("JSON after_roundtrip file is identical and should be removed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did we get such a file in the first place?
What if one or both buffers is not in canonical form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did we get such a file in the first place?
probably iterating on a PR in a way that make update
created the after_roundtrip (like starting a new optional field as a non-pointer, make update
created an after_roundtrip file, then later fixing the new field to be a pointer and make update
regenerates after_roundtrip
(since it exists) but the content is now identical to the original fixture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if one or both buffers is not in canonical form?
not sure what you mean, the files are generated by our encoding so are canonical
#116364 flake |
/retest |
/lgtm since it's only devs exercising this once in a while probably don't need to worry about the corner cases |
LGTM label has been added. Git tree hash: beaf51b0fc4097168e04f9a9d746efaa48486a61
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, liggitt 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 |
/triage accepted |
Any change to admissionregistration.k8s.io.v1alpha1.ValidatingAdmissionPolicy API would regenerate the deleted files. We need to do this PR again when the API changes are finalized for this release. |
if there's a change, it's fine to have an after_roundtrip file created. this PR was cleaning up after_roundtrip files that were identical to their original files |
/kind cleanup
Detects and removes unnecessary "after_roundtrip" fixtures which are identical to their original files.
cleans up four that crept in accidentally, these were identical to their original fixtures:
/sig api-machinery
/assign @lavalamp