Skip to content

[ty] Fix GitHub-annotations mdtest output format #23694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AlexWaygood merged 4 commits into from
Mar 3, 2026
Merged

[ty] Fix GitHub-annotations mdtest output format #23694

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
525303b
[ty] Fix GitHub annotations for mdtest failures by printing directly …
claude Mar 3, 2026
213ba68
TESTING ONLY: deliberately break mdtest assertions to verify GitHub a…
claude Mar 3, 2026
c3dc997
[ty] Use repo-relative paths for GitHub annotations so they appear in…
claude Mar 3, 2026
f508bfc
Revert "TESTING ONLY: deliberately break mdtest assertions to verify …
AlexWaygood Mar 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 63 additions & 66 deletions crates/ty_test/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,36 +98,31 @@ pub fn run(
EmbeddedFileSourceMap::new(&md_index, test_failures.backtick_offsets);

for (relative_line_number, failures) in test_failures.by_line.iter() {
let file = match output_format {
OutputFormat::Cli => relative_fixture_path.as_str(),
OutputFormat::GitHub => absolute_fixture_path.as_str(),
Copy link
Contributor

@carljm carljm Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess using the absolute path for Github format isn't actually necessary? We stop doing it here and I don't see it moved anywhere else in the PR.

Copy link
Member Author

@AlexWaygood AlexWaygood Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just unnecessary, it's incorrect. The first version of this PR didn't make this change, but I still didn't see any annotations showing up in the "files changed" tab. Claude diagnosed that the reason was that we were using absolute paths here rather than relative paths.

};
let file = relative_fixture_path.as_str();

let absolute_line_number =
match source_map.to_absolute_line_number(relative_line_number) {
Ok(line_number) => line_number,
Err(last_line_number) => {
let _ = writeln!(
assertion,
"{}",
output_format.display_error(
file,
last_line_number,
"Found a trailing assertion comment \
output_format.write_error(
&mut assertion,
file,
last_line_number,
"Found a trailing assertion comment \
(e.g., `# revealed:` or `# error:`) \
not followed by any statement."
)
not followed by any statement.",
);

continue;
}
};

for failure in failures {
let _ = writeln!(
assertion,
"{}",
output_format.display_error(file, absolute_line_number, failure)
output_format.write_error(
&mut assertion,
file,
absolute_line_number,
failure,
);
}
}
Expand All @@ -136,18 +131,11 @@ pub fn run(
if let Err(inconsistencies) = inconsistencies {
any_failures = true;
for inconsistency in inconsistencies {
match output_format {
OutputFormat::Cli => {
let info = relative_fixture_path.to_string().cyan();
let _ = writeln!(assertion, " {info} {inconsistency}");
}
OutputFormat::GitHub => {
let _ = writeln!(
assertion,
"::error file={absolute_fixture_path}::{inconsistency}"
);
}
}
output_format.write_inconsistency(
&mut assertion,
relative_fixture_path,
&inconsistency,
);
}
}

Expand Down Expand Up @@ -191,46 +179,55 @@ impl OutputFormat {
matches!(self, OutputFormat::Cli)
}

fn display_error(self, file: &str, line: OneIndexed, failure: impl Display) -> impl Display {
struct Display<'a, T> {
format: OutputFormat,
file: &'a str,
line: OneIndexed,
failure: T,
}

impl<T> std::fmt::Display for Display<'_, T>
where
T: std::fmt::Display,
{
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let Display {
format,
file,
line,
failure,
} = self;

match format {
OutputFormat::Cli => {
write!(
f,
" {file_line} {failure}",
file_line = format!("{file}:{line}").cyan()
)
}
OutputFormat::GitHub => {
write!(f, "::error file={file},line={line}::{failure}")
}
}
/// Write a test error in the appropriate format.
///
/// For CLI format, errors are appended to `assertion_buf` so they appear
/// in the assertion-failure message.
///
/// For GitHub format, errors are printed directly to stdout so that GitHub
/// Actions can detect them as workflow commands. Workflow commands must
/// appear at the beginning of a line in stdout to be parsed by GitHub.
#[expect(clippy::print_stdout)]
fn write_error(
self,
assertion_buf: &mut String,
file: &str,
line: OneIndexed,
failure: impl Display,
) {
match self {
OutputFormat::Cli => {
let _ = writeln!(
assertion_buf,
" {file_line} {failure}",
file_line = format!("{file}:{line}").cyan()
);
}
OutputFormat::GitHub => {
println!("::error file={file},line={line}::{failure}");
}
}
}

Display {
format: self,
file,
line,
failure,
/// Write a module-resolution inconsistency in the appropriate format.
///
/// See [`write_error`](Self::write_error) for details on why GitHub-format
/// messages must be printed directly to stdout.
#[expect(clippy::print_stdout)]
fn write_inconsistency(
self,
assertion_buf: &mut String,
fixture_path: &Utf8Path,
inconsistency: &impl Display,
) {
match self {
OutputFormat::Cli => {
let info = fixture_path.to_string().cyan();
let _ = writeln!(assertion_buf, " {info} {inconsistency}");
}
OutputFormat::GitHub => {
println!("::error file={fixture_path}::{inconsistency}");
}
}
}
}
Expand Down
Loading