JUnit output for ty lints#22125
Merged
charliermarsh merged 12 commits intoastral-sh:mainfrom Feb 11, 2026
Merged
Conversation
Contributor
Author
|
Slowly working on this one Links back to astral-sh/ty#1476 |
Typing conformance resultsNo changes detected ✅ |
|
|
cetanu
force-pushed
the
cetanu/junit_for_ty
branch
2 times, most recently
from
49c648f to
d32b2ed
Compare
cetanu
commented
Dec 22, 2025
crates/ruff/tests/cli/snapshots/cli__format__output_format_junit.snap
Outdated
Show resolved
Hide resolved
Member
|
Thanks for working on this. For an initial version, I suggest focusing on just rendering the |
cetanu
force-pushed
the
cetanu/junit_for_ty
branch
2 times, most recently
from
5ca12d6 to
b1b7686
Compare
Contributor
Author
|
I removed the code for handling sub diagnostics and snippets with the full renderer and instead just print out the concise message I changed how the output is formatted, making the XML a bit easier on human eyes for both ruff and ty junit formats, but it was done by specifying indents and newlines which may be brittle if the format is changed in future. I can squash these commits if needed. I left them as-is in case some of the history is useful in future |
MichaReiser
reviewed
Jan 4, 2026
cetanu
force-pushed
the
cetanu/junit_for_ty
branch
from
0a8e2bd to
433b5a0
Compare
Member
|
Is this PR ready for review ? |
cetanu
requested review from
Gankra,
carljm,
dcreager and
sharkdp
as code owners
Contributor
Author
|
Yes! |
MichaReiser
requested changes
Jan 15, 2026
Member
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thank you.
This requires some more work to remove the remaining mentions of ruff and we should add a CLI test demonstrating that the output format works in ty (which I'd expect to panic right now). See
ruff/crates/ty/tests/cli/main.rs
Lines 656 to 740 in 433b5a0
Comment on lines
6
to
9
| <testsuites name="ruff" tests="1" failures="1" errors="0"> | ||
| <testsuite name="/call.py" tests="1" disabled="0" errors="0" failures="1" package="org.ruff"> | ||
| <testcase name="org.ruff.F821" classname="/call" line="1" column="1"> | ||
| <failure message="Undefined name `f`">line 1, col 1, Undefined name `f`</failure> |
Member
There was a problem hiding this comment.
We need to parametrize the output format with the tool name so that it mentions ty here (when using in ty), similar to how it's done for the GithubEmitter
ruff/crates/ruff_db/src/diagnostic/render/github.rs
Lines 3 to 6 in 1644786
| .extra | ||
| .insert(XmlString::new("package"), XmlString::new("org.ruff")); | ||
|
|
||
| let classname = Path::new(filename).with_extension(""); |
Member
There was a problem hiding this comment.
We need to make changes to group_diagnostics_by_filename to support both ruff and ty files (it calls expect_ruff_file).
| let classname = Path::new(filename).with_extension(""); | ||
| case.set_classname(classname.to_str().unwrap_or(filename)); | ||
| let msg = diagnostic.concise_message().to_str(); | ||
| case.status.set_message(msg.clone()); |
Member
There was a problem hiding this comment.
Nit: I've a slight preference to keep status mutable as it was before and setting its fields before passing it to TestCase::new
| col = location.column, | ||
| )) | ||
| }) | ||
| .unwrap_or_default(); |
Member
There was a problem hiding this comment.
I don't think we should use unwrap_or_default here. We're very intentional about omitting line/column information in some diagnostics if they apply to the entire file and we should preserve this here
charliermarsh
force-pushed
the
cetanu/junit_for_ty
branch
from
433b5a0 to
aa3289e
Compare
charliermarsh
requested review from
AlexWaygood and
BurntSushi
as code owners
This uses existing code from ruff and "just works". The test cases output are okay, but they are missing bits of data that would be useful, and would normally be present in the default output format. I'm not sure what approach I will take from here, but I think, if it's possible, it would be good to have the entire text message that would normally appear, in the failure message of the junit test cases. Any decent junit report parser should display these well, in places such as the results of a CI job.
Unsure if it's desired to have them formatted like this or whether they should be some kind of additional structure
Not sure if there are other sub diagnostic types that could end up here. Will need to look around and see if that's the case
Also added the severity as a prefix, to emulate what the default formatter looks like for these messages. Still haven't quite worked out how to get it to print out a snippet of the code with arrows/underlines. Maybe leave that for later. The amount of information in the junit report seems sufficient for a user to track down the issue, at the moment.
Rather than trying to do a full renderer, including code snippets.
charliermarsh
force-pushed
the
cetanu/junit_for_ty
branch
from
fd0071e to
e6b7de6
Compare
charliermarsh
approved these changes
Feb 11, 2026
Memory usage reportMemory usage unchanged ✅ |
ntBre
added a commit
that referenced
this pull request
Feb 11, 2026
Summary -- This is a follow-up to #22125, which added a `program` field to the shared `DisplayDiagnosticConfig` type. We can reuse this in the GitHub rendering too to avoid some of the special casing we had just for Ruff. The one wrinkle here is that we previously used capital `Ruff` for GitHub diagnostics, but we use `ruff` in the JUnit diagnostics. I think it's probably fine to change this in the GitHub diagnostics, but it felt worth calling out. Test Plan -- Existing tests with updated snapshots showing the case change
ntBre
added a commit
that referenced
this pull request
Feb 11, 2026
) Summary -- This is a follow-up to #22125, which added a `program` field to the shared `DisplayDiagnosticConfig` type. We can reuse this in the GitHub rendering too to avoid some of the special casing we had just for Ruff. The one wrinkle here is that we previously used capital `Ruff` for GitHub diagnostics, but we use `ruff` in the JUnit diagnostics. I think it's probably fine to change this in the GitHub diagnostics, but it felt worth calling out. Test Plan -- Existing tests with updated snapshots showing the case change
T-256
reviewed
Feb 12, 2026
| TestEnvironment { | ||
| db: TestDb::new(), | ||
| config: DisplayDiagnosticConfig::default(), | ||
| config: DisplayDiagnosticConfig::new("ty"), |
Contributor
There was a problem hiding this comment.
better for avoid snapshot modifications (ruff lint codes after ty name) in this PR:
Suggested change
| config: DisplayDiagnosticConfig::new("ty"), | |
| config: DisplayDiagnosticConfig::new("ruff"), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adding JUnit XML output for
tyTest Plan
Reusing existing tests, still working this out, will amend this description later when I figure out how to specifically test
tylints, it seems they already are but will verify