-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add fuzz tests for templates #5776
Comments
@mfojtik fyi |
@liggitt can you give me a clue about how this should work? We intentionally don't convert anything in the Objects (or I think we should not be doing that) as you might have different versions of the objects inside the Template. I tried to write this: func(j *template.Template, c fuzz.Continue) {
c.Fuzz(&j.ObjectMeta)
c.Fuzz(&j.Parameters)
fuzzObjects := func(objects *[]runtime.Object) {
samples := []runtime.Object{
&build.BuildConfig{},
&image.ImageStream{},
&api.Pod{},
}
for i, _ := range samples {
c.Fuzz(&samples[i])
*objects = append(*objects, samples[i])
}
return
}
fuzzObjects(&j.Objects)
}, But of course the test failed badly... |
Not sure... @bparees might know the guts of the template object better to know what's expected |
i think that test is correct as is... we fuzz the parameters and metadata, which do need conversion, and we use a fixed object for the ObjectList, which does not need conversion because the template logic doesn't process those as api objects, they're just opaque json. |
The problem was that the template stuff was round-tripping JSON in a lossy way. Fixed in #5774, but a fuzzing test might have caught it earlier. |
@mfojtik why did that test fail? it looks reasonable to me. |
@bparees because conversion touches all objects I pass and try to convert the version of them to match the template (we are not doing that for templates), then the serialization test explodes because of that... |
@bparees do you have anyone to pick this up? I might get to this next week (maybe). |
not really (that is, this isn't any more important than any of our other vast amount of tech debt), but i can add it to our tech debt backlog, no reason for you to own it. |
I can't say I understand the coding/decoding machinery (are there any docs?) but it appears that if there's e.g.
Number three is probably closest to what actually happens when running openshift because there's probably no reason to convert concrete objects to |
i think we should be doing roughly what @mfojtik laid out above. I still don't totally understand why his example test didn't work, but if we need to assert the api version of the objects as part of fuzzing them, we can do that. |
@jim-minter several people have tried and failed at this but i have faith in you. |
Automatic merge from submit-queue (batch tested with PRs 16667, 16796, 16960, 16965, 16894). enhance template fuzz testing fixes #5776
This is the current fuzzer for templates:
We need to test conversions with templates, especially since they make use of unstructured API objects, which is uncommon.
The text was updated successfully, but these errors were encountered: