[red-knot] add opt-in external diagnostic snapshotting to mdtest by BurntSushi · Pull Request #15945 · astral-sh/ruff

BurntSushi · 2025-02-04T19:59:18Z

This PR makes it possible to opt into diagnostic snappshotting
in mdtests. To opt in, add snapshot-diagnostic to the info string
on at least one of the code blocks in a single test. Like this:

## Unresolvable module import

```py snapshot-diagnostic
import zqzqzqzqzqzqzq
```

This integrates with insta via external file snapshotting, so
one can just use cargo insta to manage the snapshots like you
would anywhere else.

While highly desirable, this doesn't support inline snapshotting.
I don't see an easy way to make this work with insta, so I think
we'd have to roll our own snapshotting system. I actually do think
that's probably worth doing, but this at least gets us some integration
with mdtests and full diagnostic output without too much effort.

Reviewers are encouraged to go commit-by-commit!

Closes #15867

BurntSushi · 2025-02-04T20:07:20Z

Yessss. My favorite kind of problem. /s

github-actions · 2025-02-04T20:08:43Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

carljm

Thank you, this is awesome!

I didn't carefully review the code changes yet, my comments are more on the feature design first.

crates/red_knot_test/src/parser.rs

crates/red_knot_test/README.md

carljm · 2025-02-04T21:26:34Z

crates/red_knot_test/README.md

+output generated. When any one code block has snapshotting enabled, then it is
+automatically enabled for all code blocks.


I'm wondering about the rationale or need for this. It seems confusing. Is it difficult to implement snapshotting for some files in a test and not others? If so, then I wonder if we should find a different way to signal the need for snapshotting, that is scoped to the markdown heading and not to the code block. If not, then it seems less confusing to me to only snapshot the code blocks explicitly marked for snapshotting.

I think it's pretty easy to implement. See my other comment above for why I went this direction. But I agree that if I keep this direction, then it probably makes more sense to use something scoped to a section and not to a code block. Do you have any opinions on what that might look like? I was unsure of that direction since I don't think there are any section-scoped options right now.

I don't think there's existing markdown syntax that lends itself naturally to this. One way to leverage existing mdtest syntax would be to make it an option in the toml config that can be optionally provided (as its own code block), but this is pretty verbose, and I'm not sure I like having mdtest-specific (as opposed to red-knot) configuration in those toml blocks.

So beyond that, I guess it could be something like adding [snapshot] to the end of the header line? Or on the next non-empty line after the header?

We could use an HTML comment at the top of the file? I think they're hidden by GitHub's MarkDown rendering?

 # This is my mdtest Tests for the `foo` Python feature ```py foo() ```

Rendered, this looks like:

This is my mdtest

Tests for the foo Python feature

foo()

What would be neat long term if output snapshots would be a link to the snapshot file. Should we use

- [Snapshots]:

or similar to enable this in the future?

Either way. I'm fine with whatever marker you pick :)

crates/red_knot_python_semantic/resources/mdtest/import/basic.md

crates/red_knot_test/src/parser.rs

crates/red_knot_test/README.md

crates/red_knot_test/src/lib.rs

.../mdtest/snapshots/red_knot_test__basic.md_-_Structures_-_Unresolvable_submodule_imports.snap

crates/red_knot_test/src/matcher.rs

AlexWaygood

Overall this is awesome!

I have similar concerns to @carljm:

To me it's pretty counter-intuitive that adding snapshot-diagnostics to one code fence would enable snapshotting for all embedded files in the mdtest suite. I would prefer a mechanism that makes it more obvious that it toggles the setting for the whole mdtest (if that's the way we want to enable snapshotting), such as an HTML comment at the top of the file (https://github.com/astral-sh/ruff/pull/15945/files#r1942686379)
I'm also not sure I'm keen on having mdtest automatically ignore unmatched diagnostics if there are no inline assertions and diagnostic snapshotting is enabled. It feels too implicit and error-prone to me, and I don't find adding an inline assertion to be particularly arduous if you only assert the error code (omitting the error message). One of the reasons I'm most excited about snapshotting is I think we can assert the full error message much less in our inline assertions; the full error message feels like something that's much better covered by our snapshots, enabling us to keep our inline assertions concise, readable, and focussed on whether we capture Python's semantics correctly.

...test/snapshots/red_knot_test__basic.md_-_Structures_-_Unresolvable_submodule_imports.md.snap

AlexWaygood

LGTM other than #15945 (comment). Thank you!

MichaReiser

This is great! Thanks for taking the time to integrate diagnostic snapshoting into mdtests

MichaReiser · 2025-02-05T16:15:19Z

...antic/resources/mdtest/snapshots/basic.md_-_Structures_-_Unresolvable_submodule_imports.snap

Is it possible to change the snapshot extension to md.snap so that we get markdown rendering on Github? (you might also have to insert an artificial newline at the start of the file to have the title separated from ---)

Andrew tried that, but unfortunately the MarkDown rendering made it completely unreadable, as GitHub thought we were trying to make an HMTL table. See discussion in #15945 (comment) :-)

Yeah I think for now I'd prefer not to spend more time fiddling with this. The existing .md.snap files don't render well either: https://github.com/astral-sh/ruff/blob/a3f802acb4aacb7a6ff3d8daaae4dd9cf3620f8d/crates/ruff_linter/src/rules/pylint/rules/snapshots/ruff_linter__rules__pylint__rules__unreachable__tests__if.py.md.snap#L4

Yeah, we might need a few newlines after the table but we can follow up on that separately

crates/red_knot_test/src/lib.rs

MichaReiser · 2025-02-05T16:17:27Z

crates/red_knot_test/src/lib.rs

+
+    let mut snapshot = String::new();
+    writeln!(snapshot, "---").unwrap();
+    writeln!(snapshot, "mdtest name: {}", test.name()).unwrap();


Should/could we insert a link to the mdtest file?

OK, so I've at least put the full path to the mdtest file here.

I'm not sure about a link. Since this already isn't rendering well as Markdown on GitHub. But if we get that working then I think just turning it into a relative link that GitHub understands should be straight-forward.

carljm

This approach looks great to me as a starting point to experiment with and see how we like it! Thanks for considering my comments.

…oning This change was done to reduce snapshot churn. Previously, if one added a new section to an Markdown test suite, then the snapshots of all sections with unnamed files below it would necessarily change because of the unnamed file count being global to the test suite. Instead, we track counts based on section. While adding new unnamed files within a section will still change unnamed files below it, I believe this will be less "churn" because the snapshot will need to change anyway. Some churn is still possible, e.g., if code blocks are re-ordered. But I think this is an acceptable trade-off.

I found it useful to have the `&dyn Diagnostic` trait impl specifically. I added `Arc<dyn Diagnostic>` for completeness. (I do kind of wonder if we should be preferring `Arc<dyn ...>` over something like `Box<dyn ...>` more generally, especially for things with immutable APIs. It would make cloning cheap.)

This makes it possible for callers to set where snapshots should be stored. In general, I think we expect this to always be set, since otherwise snapshots will end up in `red_knot_test`, which is where the tests are actually run. But that's overall counter-intuitive. This permits us to store snapshots from mdtests alongside the mdtests themselves.

I split this out into a separate commit and put it here so that reviewers can get a conceptual model of what the code is doing before seeing the code. (Hopefully that helps.)

This ties together everything from the previous commits. Some interesting bits here are how the snapshot is generated (where we include relevant info to make it easier to review the snapshots) and also a tweak to how inline assertions are processed. This commit also includes some example snapshots just to get a sense of what they look like. Follow-up work should add more of these I think.

BurntSushi requested review from AlexWaygood, MichaReiser, carljm and sharkdp as code owners February 4, 2025 19:59

carljm reviewed Feb 4, 2025

View reviewed changes

MichaReiser added the ty Multi-file analysis & type inference label Feb 5, 2025

sharkdp mentioned this pull request Feb 5, 2025

[red-knot] Merge Markdown code blocks inside a single section #15950

Merged

AlexWaygood reviewed Feb 5, 2025

View reviewed changes

crates/red_knot_test/src/parser.rs Outdated Show resolved Hide resolved

crates/red_knot_test/src/parser.rs Outdated Show resolved Hide resolved

AlexWaygood reviewed Feb 5, 2025

View reviewed changes

BurntSushi force-pushed the ag/diagnostic-tests branch from 841886f to 48a66ba Compare February 5, 2025 14:19

AlexWaygood reviewed Feb 5, 2025

View reviewed changes

...test/snapshots/red_knot_test__basic.md_-_Structures_-_Unresolvable_submodule_imports.md.snap Show resolved Hide resolved

BurntSushi force-pushed the ag/diagnostic-tests branch from 48a66ba to e760bc2 Compare February 5, 2025 14:35

BurntSushi requested review from AlexWaygood and carljm February 5, 2025 14:44

AlexWaygood approved these changes Feb 5, 2025

View reviewed changes

BurntSushi force-pushed the ag/diagnostic-tests branch from e760bc2 to 98745f8 Compare February 5, 2025 14:55

MichaReiser approved these changes Feb 5, 2025

View reviewed changes

carljm approved these changes Feb 5, 2025

View reviewed changes

BurntSushi added 5 commits February 5, 2025 12:35

red_knot_test: update README with section on diagnostic snapshotting

ff623c6

I split this out into a separate commit and put it here so that reviewers can get a conceptual model of what the code is doing before seeing the code. (Hopefully that helps.)

BurntSushi force-pushed the ag/diagnostic-tests branch from 98745f8 to a68ad9c Compare February 5, 2025 17:59

BurntSushi enabled auto-merge (rebase) February 5, 2025 18:00

BurntSushi merged commit a84b27e into main Feb 5, 2025
20 checks passed

BurntSushi deleted the ag/diagnostic-tests branch February 5, 2025 18:02

AlexWaygood mentioned this pull request Feb 6, 2025

[red-knot] Fixup a couple of nits in the red_knot_test README #15996

Merged

BurntSushi mentioned this pull request Feb 19, 2025

add support for inline diagnostic snapshotting for mdtest astral-sh/ty#195

Open

		output generated. When any one code block has snapshotting enabled, then it is
		automatically enabled for all code blocks.

Conversation

BurntSushi commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BurntSushi commented Feb 4, 2025

Uh oh!

github-actions bot commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

This is my mdtest

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BurntSushi commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

`ruff-ecosystem` results

AlexWaygood Feb 5, 2025 •

edited

Loading