[red-knot] improve type shrinking coverage in red-knot property tests#15297
[red-knot] improve type shrinking coverage in red-knot property tests#15297sharkdp merged 1 commit intoastral-sh:mainfrom
Conversation
rtpg
requested review from
AlexWaygood,
MichaReiser,
carljm and
sharkdp
as code owners
|
MichaReiser
added
testing
|
I believe to have handled all the comments in the first review and have pushed up a new version |
sharkdp
left a comment
sharkdp
left a comment
There was a problem hiding this comment.
Thank you very much for working on this!
If it's not too much effort, it would be great to see some evidence that shrinking actually gets better. Is it possible to pin the random seed in quickcheck and then show a few before/after results?
| Ty::Tuple(types) => Box::new(types.into_iter()), | ||
| Ty::Intersection { pos, neg } => Box::new(pos.into_iter().chain(neg)), | ||
| Ty::Union(types) => Box::new(types.shrink().filter_map(|elts| match elts.len() { | ||
| 0 => None, |
There was a problem hiding this comment.
It might not be completely unreasonable to try and return Ty::Never here?
This also makes me think: I think we currently only try shrinking for the root of the Type tree? Like if we have tuple[A | B, C], I think we would only try to remove either A | B from the tuple, or try to remove C from the tuple, i.e. we try to shrink to C or to A | B, but we would not try to shrink the A | B union.
There was a problem hiding this comment.
Calling shrink on the vector of types actually does attempt to shrink the elements of the vector as well as trying to remove elements, so we get nice shrinking just leaning on the shrinking built into quickcheck (at least that was my read of the source)
There was a problem hiding this comment.
Regarding the empty union shrinking to Ty::Never... feels fine by me! Especially now that I fully grok that Ty is a smart constructor for Type so I don't need to worried about types being in the "wrong location", so to speak
There was a problem hiding this comment.
Calling shrink on the vector of types actually does attempt to shrink the elements of the vector as well as trying to remove elements
👍
Regarding the empty union shrinking to
Ty::Never... feels fine by me! Especially now that I fully grok thatTyis a smart constructor forTypeso I don't need to worried about types being in the "wrong location", so to speak
If we try to shrink at all "depths", I think we can skip this. The idea was to simplify nested Ty unions like A | B | (C | D) to A | B | Never and then further down to A | B, but there are many other possible shrinking-paths that would lead to that result (e.g. via the single-element union or via a element-removal shrink on the outer union).
|
@sharkdp do you know how to pin the quickcheck seed? I'm looking through the quickcheck source and readme and not seeing anything, but I am famously bad at finding things that are obvious. |
No, I just hoped there would be a way. But it does not seem to be the case: BurntSushi/quickcheck#278 In this case, I'm okay with your "now the errors seem more minimal" observation. |
* main: Use uv consistently throughout the documentation (#15302) [red-knot] Eagerly normalize `type[]` types (#15272) [`pyupgrade`] Split `UP007` to two individual rules for `Union` and `Optional` (`UP007`, `UP045`) (#15313) [red-knot] Improve symbol-lookup tracing (#14907) [red-knot] improve type shrinking coverage in red-knot property tests (#15297) [`flake8-return`] Recognize functions returning `Never` as non-returning (`RET503`) (#15298) [`flake8-bugbear`] Implement `class-as-data-structure` (`B903`) (#9601) Avoid treating newline-separated sections as sub-sections (#15311) Remove call when removing final argument from `format` (#15309) Don't enforce `object-without-hash-method` in stubs (#15310) Don't special-case class instances in binary expression inference (#15161) Upgrade zizmor to the latest version in CI (#15300)
3 participants
Summary
While looking at #14899, I looked at seeing if I could get shrinking on the examples. It turned out to be straightforward, with a couple of caveats.
I'm calling
clonea lot during shrinking. Since by the shrink step we're already looking at a test failure this feels fine? Unless I misunderstoodquickcheck's core loopWhen shrinking
Intersections, in order to just rely onquickcheck'sVecshrinking without thinking about it too much, the shrinking strategy is:This means that you can't shrink from
(A & B & ~C & ~D)directly to(A & ~C)! You would first need an intermediate failure at(A & B & ~C)or(A & ~C & ~D). This feels good enough. Shrinking the negative side first also has the benefit of trying to strip down negative elements in these intersections.Test Plan
cargo test -p red_knot_python_semantic -- --ignored types::property_tests::stablestill fails as it current does onmain, but now the errors seem more minimal.