Rapid test ensure bitstream consumed #2013
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2013 +/- ##
=======================================
Coverage 60.76% 60.77%
=======================================
Files 350 350
Lines 45686 45689 +3
=======================================
+ Hits 27762 27766 +4
Misses 16405 16405
+ Partials 1519 1518 -1 ☔ View full report in Codecov by Sentry. |
| @@ -401,6 +402,7 @@ func (tvg *tvGen) WithNullAndUnknown(gen *rapid.Generator[tv]) *rapid.Generator[ | |||
| nullGen := rapid.Just(tftypes.NewValue(tv0.typ, nil)) | |||
| options = append(options, nullGen) | |||
| } | |||
| _ = rapid.Bool().Draw(t, "consumeBitstream2") | |||
There was a problem hiding this comment.
Well, hmm, I bet this is not idiomatic but I don't know if this introduces any semantic problems also.
Is the problem simply in the degenerate case where we have:
tvg.generateUnknowns == false
!tv0.schema.Required == false
options == []*rapid.Generator[tftypes.Value]{gen}
gen == rapid.OneOf(options...)
Should it stop wrapping gen with rapid.OneOf(gen) in that case maybe that fixes the problem?
There was a problem hiding this comment.
This one feels especially wrong. We do draw entropy from t on L393+: tv0 := gen.Draw(t, "tv"). The only way we can get this error message (as I understand it), is if gen.Draw(t, "tv") doesn't draw entropy, implying the bug is somewhere else.
There was a problem hiding this comment.
I have not verified both are needed, just that the panic no longer happens with them.
| @@ -209,6 +209,7 @@ func (tvg *tvGen) GenBlockWithDepth(depth int) *rapid.Generator[tb] { | |||
| return tftypes.NewValue(objType, fields) | |||
| }) | |||
| } else { | |||
| _ = rapid.Bool().Draw(t, "consumeBitstream1") | |||
There was a problem hiding this comment.
This is really weird also. I think perhaps issue could be with this code when minFields=3 and there's no entropy going into the result?
nFields := rapid.IntRange(minFields, 3).Draw(t, "nFields")
There was a problem hiding this comment.
I believe the issue comes when drawing the values, not the schema.
I suspect if we run into this branch, we produce a rapid.Just(tftypes.NewValue(objType, map[string]tftypes.Value{})) which consumes none of the entropy.
I don't see why this has to consume the bitstream, really - am I misunderstanding the framework here?
There was a problem hiding this comment.
The whole thing is a bit tricky since I haven't found a reliable way of reproducing the issue - it's sensitive to code changes, so the same seed does not produce the error after changing the code.
There was a problem hiding this comment.
rapid.Just should be fine so long as something else drew entropy. I think the problem arises where rapid.Custom(f) draws no entropy in f.
There was a problem hiding this comment.
We always draw entropy, when calling rapid.IntRange(minFields, 3).Draw(t, "nFields"). @VenelinMartinov If we leave a hack like this in, can you please mark it with a TODO and create an issue to track removing the hack.
That would be a good issue for help-wanted and good-first-issue.
There was a problem hiding this comment.
That would be a good issue for help-wanted and good-first-issue.
Well FWIW I do not think so, unfortunately I do not think this would work very well.. As an external contributor I'd expect to have zero interest in verification work as it is too indirectly connected to end-user problems and outcomes. Perhaps we can make a distinction between easy onboarding tasks and welcoming external contributors.
The encompassing if check was flipped and this didn't work correctly. #2013 should take care of the rapid panics for unconsumed bitrstreams, so the encompassing if check should not be needed any more.
|
@VenelinMartinov Do we want to merge this? |
|
I've meant to revisit but haven't managed to yet. If you are seeing the panics here then I'm happy to merge this until we find time to root-cause |
No problem.
I'm not sure what you mean here. |
Are you blocked on this PR getting merged or were you just revisiting old PRs? |
Just revisiting old PRs. |
|
@VenelinMartinov Are you still interested in pushing this PR forward? |
rapid wants us to always consume from the random bitstream but we don't do that when we generate empty collection values. This just adds some generators in those cases so that rapid doesn't panic.