[red-knot] Move UnionBuilder tests to Markdown
#15374
Conversation
This moves almost all of our existing `UnionBuilder` tests to a Markdown-based test suite. I see how this could be a more controversial change, since these tests where written specifically for `UnionBuilder`, and by creating the union types using Python type expressions, we add an additional layer on top (parsing and inference of these expressions) that move these tests away from clean unit tests more in the direction of integration tests. Also, there are probably a few implementation details of `UnionBuilder` hidden in the test assertions (e.g. order of union elements after simplifications). That said, I think we would like to see all those properties that are being tested here from *any* implementation of union types. And the Markdown tests come with the usual advantages: - Much more consice - Better readability - No re-compiliation when working on tests - Easier to add additional explanations and structure to the test suite
sharkdp
requested review from
carljm,
MichaReiser and
AlexWaygood
as code owners
|
| def _( | ||
| u1: str | LiteralString, u2: LiteralString | str, u3: Literal["a"] | str | LiteralString, u4: str | bytes | LiteralString | ||
| ) -> None: | ||
| reveal_type(u1) # revealed: str | ||
| reveal_type(u2) # revealed: str | ||
| reveal_type(u3) # revealed: str | ||
| reveal_type(u4) # revealed: str | bytes |
There was a problem hiding this comment.
These tests would be more readable if we had something like C++'s std::declval<T>() (#13789) which pretends to create a value of type T. Using knot_extensions, we could now easily implement a function like the following (using PEP 747 language):
def value_of[T](typ: TypeForm[T]) -> T: ...that would allow us to write these tests in a single line without having to introduce dummy parameters and without having the definitions a few lines away from the assertions:
| def _( | |
| u1: str | LiteralString, u2: LiteralString | str, u3: Literal["a"] | str | LiteralString, u4: str | bytes | LiteralString | |
| ) -> None: | |
| reveal_type(u1) # revealed: str | |
| reveal_type(u2) # revealed: str | |
| reveal_type(u3) # revealed: str | |
| reveal_type(u4) # revealed: str | bytes | |
| reveal_type(value_of(str | LiteralString)) # revealed: str | |
| reveal_type(value_of(LiteralString | str)) # revealed: str | |
| reveal_type(value_of(Literal["a"] | str | LiteralString)) # revealed: str | |
| reveal_type(value_of(str | bytes | LiteralString)) # revealed: str | bytes |
A disadvantage of this approach would be that it makes more of these tests dependent on knot_extensions. @AlexWaygood is still "a little sceptical" about this idea, so I kept the usual strategy of using function parameters.
There was a problem hiding this comment.
Interestingly, typing.cast (with a dummy second argument), is effectively the value_of function. I wouldn't be opposed to adding value_of as a variant that doesn't require the dummy second argument, at the same time we add typing.cast. But I don't feel strongly either way, I think the function arguments are reasonable too.
There was a problem hiding this comment.
A disadvantage of this approach would be that it makes more of these tests dependent on
knot_extensions.
Also, could consider reveal_type_of_value(...) function which has same logic of reveal_type that doesn't need to explicitly import?
There was a problem hiding this comment.
Also, could consider
reveal_type_of_value(...)function which has same logic ofreveal_typethat doesn't need to explicitly import?
More like reveal_type_of_type_expression or something like that, but yes — that's a good idea! We could take it one step further and implement the analog to assert_type on that level. Basically assert_equal_type(type1, type2).
And now that I write it out, I realize that we sort-of already have this in knot_extensions. We can write
static_assert(is_equivalent_to(str | LiteralString, str))Now that only works for fully static types, but once we have is_gradual_equivalent_to, that would be a (slightly more verbose) alternative to assert_equal_type:
static_assert(is_gradual_equivalent_to(Unknown | Unknown | str, Unknown | str))There was a problem hiding this comment.
Interestingly,
typing.cast(with a dummy second argument), is effectively thevalue_offunction.
"It's like typing.cast, just without having a value in the first place!" is exactly how I pitched it to Alex, and he still didn't want it 😄
crates/red_knot_python_semantic/resources/mdtest/union_types.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/union_types.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/union_types.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/union_types.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I haven't reviewed in detail, happy to let Alex do that for efficiency's sake. Just clarifying that I'm on board with moving these tests to mdtest, I think the pros outweigh the cons. I think almost any test where we have to use the Ty enum and construct arbitrary types, is probably better as an mdtest.
| def _( | ||
| u1: str | LiteralString, u2: LiteralString | str, u3: Literal["a"] | str | LiteralString, u4: str | bytes | LiteralString | ||
| ) -> None: | ||
| reveal_type(u1) # revealed: str | ||
| reveal_type(u2) # revealed: str | ||
| reveal_type(u3) # revealed: str | ||
| reveal_type(u4) # revealed: str | bytes |
There was a problem hiding this comment.
Interestingly, typing.cast (with a dummy second argument), is effectively the value_of function. I wouldn't be opposed to adding value_of as a variant that doesn't require the dummy second argument, at the same time we add typing.cast. But I don't feel strongly either way, I think the function arguments are reasonable too.
crates/red_knot_python_semantic/resources/mdtest/union_types.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Waygood <[email protected]> Co-authored-by: T-256 <[email protected]>
sharkdp
force-pushed
the
david/union-type-tests
branch
from
42a2c73 to
4eec725
Compare
* main: [red-knot] Move `UnionBuilder` tests to Markdown (#15374)
Summary
This moves almost all of our existing
UnionBuildertests to a Markdown-based test suite.I see how this could be a more controversial change, since these tests where written specifically for
UnionBuilder, and by creating the union types using Python type expressions, we add an additional layer on top (parsing and inference of these expressions) that moves these tests away from clean unit tests more in the direction of integration tests. Also, there are probably a few implementation details ofUnionBuilderhidden in the test assertions (e.g. order of union elements after simplifications).That said, I think we would like to see all those properties that are being tested here from any implementation of union types. And the Markdown tests come with the usual advantages:
This changeset adds a few additional tests, but keeps the logic of the existing tests except for a few minor modifications for consistency.