[red-knot] Fix unit tests in release mode#14604
Conversation
sharkdp
requested review from
AlexWaygood,
MichaReiser and
carljm
as code owners
This is about the easiest patch that I can think of. It's simple, but has a drawback in that there is no real guarantee this won't happen again. I think this might be acceptable, given that all of this is a temporary thing. But there are other approaches we could take: - We could get rid of the debug/release distinction and just add `@Todo` type metadata everywhere. This has possible affects on runtime. The main reason I didn't follow through with this is that the size of `Type` increases. We would either have to adapt the `assert_eq_size!` test or get rid of it. Even if we add messages everywhere and get rid of the file-and-line-variant in the enum, it's not enough to get back to the current release-mode size of `Type`. - We could generally discard `@Todo` meta information when using it in tests. I think this would be a huge drawback. I like that we can have the actual messages in the mdtest. And make sure we get the expected `@Todo` type, not just any `@Todo`. It's also helpful when debugging tests.
sharkdp
force-pushed
the
david/fix-14594
branch
from
1b3d517 to
059aa98
Compare
crates/red_knot_test/src/matcher.rs
Outdated
| regex::Regex::new(r"@Todo\([^)]*\)") | ||
| .unwrap() | ||
| .replace_all(ty, "@Todo") |
There was a problem hiding this comment.
This uses a regex instead of something like if ty.starts_with("@Todo(") { "@Todo" } else { ty } or similar, because there might be more complicated types like @Todo(…) | str.
| if cfg!(debug_assertions) { | ||
| "@Todo(async iterables/iterators)" | ||
| } else { | ||
| "@Todo" | ||
| }, |
There was a problem hiding this comment.
An alternative here would be to rename assert_scope_ty to assert_scope_ty_str, and then introduce a new helper to directly assert on the actual Type, not its string representation. I would hope that this test can be migrated to an mdtest sooner or later (#13696).
|
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This looks good to me. Considering that it's somewhat easy to introduce a new failure in the future, would you mind adding a test step to the release-job running on main
ruff/.github/workflows/ci.yaml
Lines 212 to 226 in 66abef4
2 participants
Summary
This is about the easiest patch that I can think of. It has a drawback in that there is no real guarantee this won't happen again. I think this might be acceptable, given that all of this is a temporary thing. But there are other approaches we could take:
@Todotype metadata everywhere. This has possible affects on runtime. The main reason I didn't follow through with this is that the size ofTypeincreases. We would either have to adapt theassert_eq_size!test or get rid of it. Even if we add messages everywhere and get rid of the file-and-line-variant in the enum, it's not enough to get back to the current release-mode size ofType.@Todometa information when using it in tests. I think this would be a huge drawback. I like that we can have the actual messages in the mdtest. And make sure we get the expected@Todotype, not just any@Todo. It's also helpful when debugging tests.We could also think about running tests in release mode via CI, if this is something that we need to support.
closes #14594
Test Plan