Skip to content

Commit 195bb43

Browse files
authored
[red-knot] Add GitHub PR annotations when mdtests fail in CI (#17150)
## Summary This PR adds a CI job that causes GitHub to add annotations to a PR diff when mdtest assertions fail. For example: <details> <summary>Screenshot</summary> ![image](https://github.com/user-attachments/assets/bb2a649b-46ab-429d-a576-b36545940eaf) </details> ## Motivation Debugging mdtest failures locally is currently a really nice experience: - Errors are displayed with pretty colours, which makes them much more readable - If you run the test from inside an IDE, you can CTRL-click on a path and jump directly to the line that had the failing assertion - If you use [`mdtest.py`](https://github.com/astral-sh/ruff/blob/main/crates/red_knot_python_semantic/mdtest.py), you don't even need to recompile anything after changing an assertion in an mdtest, amd the test results instantly live-update with each change to the MarkDown file Debugging mdtest failures in CI is much more unpleasant, however. Sometimes an error message is just > [static-assert-error] Argument evaluates to `False` ...which doesn't tell you very much unless you navigate to the line in question that has the failing mdtest assertion. The line in question might not even be touched by the PR, and even if it is, it can be hard to find the line if the PR touches many files. Unlike locally, you can't click on the error and jump straight to the line that contains the failing assertion. You also don't get colourised output in CI (https://github.com/astral-sh/ruff/issues/13939). GitHub PR annotations should make it really easy to debug why mdtests are failing on PRs, making PR review much easier. ## Test Plan I opened a PR to my fork [here](https://github.com/AlexWaygood/ruff/pull/11/files) with some bogus changes to an mdtest to show what it looks like when there are failures in CI and this job has been added. Scroll down to `crates/red_knot_python_semantic/resources/mdtest/type_properties/is_equivalent_to.md` on the "files changed" tab for that PR to see the annotations.
1 parent c2bb5d5 commit 195bb43

File tree

3 files changed

+82
-10
lines changed

3 files changed

+82
-10
lines changed

.github/workflows/ci.yaml

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ jobs:
3636
code: ${{ steps.check_code.outputs.changed }}
3737
# Flag that is raised when any code that affects the fuzzer is changed
3838
fuzz: ${{ steps.check_fuzzer.outputs.changed }}
39+
# Flag that is set to "true" when code related to red-knot changes.
40+
red_knot: ${{ steps.check_red_knot.outputs.changed }}
3941

4042
# Flag that is set to "true" when code related to the playground changes.
4143
playground: ${{ steps.check_playground.outputs.changed }}
@@ -166,6 +168,29 @@ jobs:
166168
echo "changed=true" >> "$GITHUB_OUTPUT"
167169
fi
168170
171+
- name: Check if the red-knot code changed
172+
id: check_red_knot
173+
env:
174+
MERGE_BASE: ${{ steps.merge_base.outputs.sha }}
175+
run: |
176+
if git diff --quiet "${MERGE_BASE}...HEAD" -- \
177+
':Cargo.toml' \
178+
':Cargo.lock' \
179+
':crates/red_knot*/**' \
180+
':crates/ruff_db/**' \
181+
':crates/ruff_annotate_snippets/**' \
182+
':crates/ruff_python_ast/**' \
183+
':crates/ruff_python_parser/**' \
184+
':crates/ruff_python_trivia/**' \
185+
':crates/ruff_source_file/**' \
186+
':crates/ruff_text_size/**' \
187+
':.github/workflows/ci.yaml' \
188+
; then
189+
echo "changed=false" >> "$GITHUB_OUTPUT"
190+
else
191+
echo "changed=true" >> "$GITHUB_OUTPUT"
192+
fi
193+
169194
cargo-fmt:
170195
name: "cargo fmt"
171196
runs-on: ubuntu-latest
@@ -221,6 +246,14 @@ jobs:
221246
uses: taiki-e/install-action@6aca1cfa12ef3a6b98ee8c70e0171bfa067604f5 # v2
222247
with:
223248
tool: cargo-insta
249+
- name: Red-knot mdtests (GitHub annotations)
250+
if: ${{ needs.determine_changes.outputs.red_knot == 'true' }}
251+
env:
252+
NO_COLOR: 1
253+
MDTEST_GITHUB_ANNOTATIONS_FORMAT: 1
254+
# Ignore errors if this step fails; we want to continue to later steps in the workflow anyway.
255+
# This step is just to get nice GitHub annotations on the PR diff in the files-changed tab.
256+
run: cargo test -p red_knot_python_semantic --test mdtest || true
224257
- name: "Run tests"
225258
shell: bash
226259
env:

crates/red_knot_python_semantic/tests/mdtest.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use camino::Utf8Path;
22
use dir_test::{dir_test, Fixture};
3+
use red_knot_test::OutputFormat;
34

45
/// See `crates/red_knot_test/README.md` for documentation on these tests.
56
#[dir_test(
@@ -18,12 +19,19 @@ fn mdtest(fixture: Fixture<&str>) {
1819

1920
let test_name = test_name("mdtest", absolute_fixture_path);
2021

22+
let output_format = if std::env::var("MDTEST_GITHUB_ANNOTATIONS_FORMAT").is_ok() {
23+
OutputFormat::GitHub
24+
} else {
25+
OutputFormat::Cli
26+
};
27+
2128
red_knot_test::run(
2229
absolute_fixture_path,
2330
relative_fixture_path,
2431
&snapshot_path,
2532
short_title,
2633
&test_name,
34+
output_format,
2735
);
2836
}
2937

crates/red_knot_test/src/lib.rs

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ pub fn run(
3434
snapshot_path: &Utf8Path,
3535
short_title: &str,
3636
test_name: &str,
37+
output_format: OutputFormat,
3738
) {
3839
let source = std::fs::read_to_string(absolute_fixture_path).unwrap();
3940
let suite = match test_parser::parse(short_title, &source) {
@@ -59,7 +60,10 @@ pub fn run(
5960

6061
if let Err(failures) = run_test(&mut db, relative_fixture_path, snapshot_path, &test) {
6162
any_failures = true;
62-
println!("\n{}\n", test.name().bold().underline());
63+
64+
if output_format.is_cli() {
65+
println!("\n{}\n", test.name().bold().underline());
66+
}
6367

6468
let md_index = LineIndex::from_source_text(&source);
6569

@@ -72,21 +76,31 @@ pub fn run(
7276
source_map.to_absolute_line_number(relative_line_number);
7377

7478
for failure in failures {
75-
let line_info =
76-
format!("{relative_fixture_path}:{absolute_line_number}").cyan();
77-
println!(" {line_info} {failure}");
79+
match output_format {
80+
OutputFormat::Cli => {
81+
let line_info =
82+
format!("{relative_fixture_path}:{absolute_line_number}")
83+
.cyan();
84+
println!(" {line_info} {failure}");
85+
}
86+
OutputFormat::GitHub => println!(
87+
"::error file={absolute_fixture_path},line={absolute_line_number}::{failure}"
88+
),
89+
}
7890
}
7991
}
8092
}
8193

8294
let escaped_test_name = test.name().replace('\'', "\\'");
8395

84-
println!(
85-
"\nTo rerun this specific test, set the environment variable: {MDTEST_TEST_FILTER}='{escaped_test_name}'",
86-
);
87-
println!(
88-
"{MDTEST_TEST_FILTER}='{escaped_test_name}' cargo test -p red_knot_python_semantic --test mdtest -- {test_name}",
89-
);
96+
if output_format.is_cli() {
97+
println!(
98+
"\nTo rerun this specific test, set the environment variable: {MDTEST_TEST_FILTER}='{escaped_test_name}'",
99+
);
100+
println!(
101+
"{MDTEST_TEST_FILTER}='{escaped_test_name}' cargo test -p red_knot_python_semantic --test mdtest -- {test_name}",
102+
);
103+
}
90104
}
91105
}
92106

@@ -95,6 +109,23 @@ pub fn run(
95109
assert!(!any_failures, "Some tests failed.");
96110
}
97111

112+
/// Defines the format in which mdtest should print an error to the terminal
113+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
114+
pub enum OutputFormat {
115+
/// The format `cargo test` should use by default.
116+
Cli,
117+
/// A format that will provide annotations from GitHub Actions
118+
/// if mdtest fails on a PR.
119+
/// See <https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-an-error-message>
120+
GitHub,
121+
}
122+
123+
impl OutputFormat {
124+
const fn is_cli(self) -> bool {
125+
matches!(self, OutputFormat::Cli)
126+
}
127+
}
128+
98129
fn run_test(
99130
db: &mut db::Db,
100131
relative_fixture_path: &Utf8Path,

0 commit comments

Comments
 (0)