Factor out more mdtest helpers#24754
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 89.36%. The percentage of expected errors that received a diagnostic held steady at 85.49%. The number of fully passing files held steady at 88/134. |
Memory usage reportMemory usage unchanged ✅ |
|
|
9a92402 to
9287424
Compare
There was a problem hiding this comment.
Can you say how much of the code in this file changed? Is it mostly a move, and I don't have to review the code beyond the outer signature, or is there more to it?
There was a problem hiding this comment.
Yes, sorry, I should have split the commits up a bit more. I just rebased and split out the changes from the moves themselves. It was mostly a move but should be easier to see in the diff now.
| @@ -240,14 +353,15 @@ pub(crate) fn apply_snapshot_filters(rendered: &str) -> std::borrow::Cow<'_, str | |||
| } | |||
|
|
|||
| pub fn validate_inline_snapshot( | |||
There was a problem hiding this comment.
Does this function still need to be public? What about other symbols that we made public as part of your last PRs?
There was a problem hiding this comment.
Yes, this is currently used in ty_test, but maybe some of your other comments will change this. I went through all of the other symbols and restricted more visibilities.
| continue; | ||
| } | ||
|
|
||
| let result = run_test(&test, &mut assertion, output_format); |
There was a problem hiding this comment.
I think the abstraction here is correct. It is unfortunate that attempt_test needs to be public only because ty_test's pull types
There was a problem hiding this comment.
I agree that this feels weird, but I didn't really see a good way around it. ty uses attempt_test twice, as you point out, and there doesn't seem to be a good way to roll both the check_file and the pull_types calls into a single attempt_test invocation. I'm definitely happy to revisit it if I'm missing something, though.
| .has_directive_set(MdtestDirective::PullTypesSkip) | ||
| } | ||
|
|
||
| pub fn check_panic(&self, panic_info: Option<PanicError>) { |
There was a problem hiding this comment.
It feels weird that this is in the parser crate. Is this something we could expose on AttemptTest instead?
There was a problem hiding this comment.
Hmm, I actually thought this was pretty nice here. This is the parser module within the mdtest crate, not a separate parser crate, in case that makes any difference. So it's exposed as MarkdownTest::check_panic, which gives us access to MarkdownTest::should_expect_panic and MarkdownTest::name. We'd have to pass some of that information separately if we wanted to expose this on AttemptTest.
There was a problem hiding this comment.
Oh, this is on test.
Maybe it would be worth moving test out of the parser module. It feels more like a data structure, similar to the AST. Anyway, I don't feel to strongly about that.
I think I'd still prefer making this a standalone function. E.g., it feels a bit off to me that a test file knows how to snapshot diagnostics, or assert the panic info. But I don't feel strongly about it. I guess my biggest concern really is that this logic is in the parser module
There was a problem hiding this comment.
Ah okay, makes sense. I'll just move these checks to standalone functions for now.
| test_name: &str, | ||
| crate_name: &str, | ||
| suite: &parser::MarkdownTestSuite<'_, C>, | ||
| mut run_test: impl FnMut( |
There was a problem hiding this comment.
I'm wondering if we should make this a trait, where C is an associated type. Maybe it's not worth it. I'm just thinking about how we could share more of run_test between crates. It probably requires something where the trait has a set_up() method that returns a TestRun (yay, more associated types), where TestRun has a check_file function. set_up would contain the logic to set up the Db etc.
For now, I think either a function or a trait is fine.
There was a problem hiding this comment.
My feeling was that it was not worth it to factor out much more of the two run_test versions because they end up being pretty different. The last chunk of code I'd really like to share is this:
ruff/crates/ruff_test/src/lib.rs
Lines 128 to 160 in 0ad4abe
but it was only ~30 lines and with a couple of small differences. I can still look into this, though. It would probably be better than the snapshot_diagnostics and check_panic methods anyway.
| .has_directive_set(MdtestDirective::PullTypesSkip) | ||
| } | ||
|
|
||
| pub fn check_panic(&self, panic_info: Option<PanicError>) { |
There was a problem hiding this comment.
Oh, this is on test.
Maybe it would be worth moving test out of the parser module. It feels more like a data structure, similar to the AST. Anyway, I don't feel to strongly about that.
I think I'd still prefer making this a standalone function. E.g., it feels a bit off to me that a test file knows how to snapshot diagnostics, or assert the panic info. But I don't feel strongly about it. I guess my biggest concern really is that this logic is in the parser module
## Summary This is a follow-up to astral-sh#24616 to share a bit more infrastructure between `ty_test` and the `ruff_test` crate introduced in astral-sh#24617. In particular, the primary `run` function has moved into `mdtest`, along with the `attempt_test`, `check_panic` and `snapshot_diagnostics` helpers. You should be able to review commit-by-commit, but the first commit is probably the most important to look at since it moves the `tracing` setup and inconsistency checks from `ty_test::run` into `ty_test::run_test`. I think that narrows the span of the `tracing` logs a bit, but the important section should still be covered. The other commits should be pretty straightforward and reviewable as one diff. ## Test Plan Existing tests, as well as rebasing astral-sh#24617 onto this branch.
Summary
This is a follow-up to #24616 to share a bit more infrastructure between
ty_testand theruff_testcrate introduced in #24617. In particular, the primaryrunfunction has moved intomdtest, along with theattempt_test,check_panicandsnapshot_diagnosticshelpers.You should be able to review commit-by-commit, but the first commit is probably the most important to look at since it moves the
tracingsetup and inconsistency checks fromty_test::runintoty_test::run_test. I think that narrows the span of thetracinglogs a bit, but the important section should still be covered.The other commits should be pretty straightforward and reviewable as one diff.
Test Plan
Existing tests, as well as rebasing #24617 onto this branch.