Skip to content
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.

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

[red-knot] mdtest output improvements #13875

Open
MichaReiser opened this issue Oct 22, 2024 · 2 comments
Open

[red-knot] mdtest output improvements #13875

MichaReiser opened this issue Oct 22, 2024 · 2 comments
Labels
help wanted Contributions especially welcome red-knot Multi-file analysis & type inference testing Related to testing Ruff itself

Comments

@MichaReiser
Copy link
Member

MichaReiser commented Oct 22, 2024

Some ideas on how we could improve the mdtest output:

Remove redundant test name

The test name, e.g. imports/errors is now rendered in at least 3 places:

failures:

---- mdtest__import_errors stdout ----

/import/errors.md - Unresolved Imports - Unresolved import from statement

    /import/errors.md:16 unmatched assertion: revealed: Unknown2
    /import/errors.md:16 unexpected error: 1 [revealed-type] "Revealed type is `Unknown`"

--------------------------------------------------

thread 'mdtest__import_errors' panicked at crates/red_knot_test/src/lib.rs:61:5:
Some tests failed.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
  1. mdtest_import_errors stdout (cargo test)
  2. The error header: /import/errors.md - Unresolved Imports - Unresolved import from statement (mdtest)
  3. The cargo test error message: thread 'mdtest__import_errors' panicked at crates/red_knot_test/src/lib.rs:61:5:

It's even worse when using nextest

--- STDOUT:              red_knot_python_semantic::mdtest mdtest__import_errors ---

running 1 test

/import/errors.md - Unresolved Imports - Unresolved import from statement

    /import/errors.md:16 unmatched assertion: revealed: Unknown2
    /import/errors.md:16 unexpected error: 1 [revealed-type] "Revealed type is `Unknown`"

--------------------------------------------------

test mdtest__import_errors ... FAILED

failures:

failures:
    mdtest__import_errors

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 67 filtered out; finished in 0.16s


--- STDERR:              red_knot_python_semantic::mdtest mdtest__import_errors ---
thread 'mdtest__import_errors' panicked at crates/red_knot_test/src/lib.rs:61:5:
Some tests failed.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I suggest that we change the mdtest header to just show the test-name, excluding the markdown file:


failures:

---- mdtest__import_errors stdout ----

Unresolved Imports - Unresolved import from statement

    /import/errors.md:16 unmatched assertion: revealed: Unknown2
    /import/errors.md:16 unexpected error: 1 [revealed-type] "Revealed type is `Unknown`"

--------------------------------------------------

thread 'mdtest__import_errors' panicked at crates/red_knot_test/src/lib.rs:61:5:
Some tests failed.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

File paths not clickable

The errors.md:16 file paths aren't clickable for me because the paths' aren't relative to the workspace root. We should make the paths relative to the workspace root (and render them as relative and not absolute paths)

Reduce indentation

I find the indentation of the inner errors distracting. I don't think we need it considering that we use blank lines and a bold title to group files.


failures:

---- mdtest__import_errors stdout ----

Unresolved Imports - Unresolved import from statement

/import/errors.md:16 unmatched assertion: revealed: Unknown2
/import/errors.md:16 unexpected error: 1 [revealed-type] "Revealed type is `Unknown`"

Unresolved Imports2 - Unresolved import from statement2

/import/errors.md:16 unmatched assertion: revealed: Unknown2
/import/errors.md:16 unexpected error: 1 [revealed-type] "Revealed type is `Unknown`"

--------------------------------------------------

thread 'mdtest__import_errors' panicked at crates/red_knot_test/src/lib.rs:61:5:
Some tests failed.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@MichaReiser MichaReiser added help wanted Contributions especially welcome red-knot Multi-file analysis & type inference labels Oct 22, 2024
@AlexWaygood
Copy link
Member

Remove redundant test name

Definitely.

File paths not clickable

Huh, they're clickable for me in VSCode. But I definitely don't object to this change.

Reduce indentation

I find the indentation of the inner errors distracting. I don't think we need it considering that we use blank lines and a bold title to group files.

One thing about this is that I do think the indentation makes the output more readable when long errors "spill over" onto multiple lines. Here's one output as it is now:

Screenshot

image

And here it is without the indentation:

Screenshot

image

Diff to produce the errors used for screenshots
diff --git a/crates/red_knot_python_semantic/resources/mdtest/exception/control_flow.md b/crates/red_knot_python_semantic/resources/mdtest/exception/control_flow.md
index f7cf500d0..fec12a7b8 100644
--- a/crates/red_knot_python_semantic/resources/mdtest/exception/control_flow.md
+++ b/crates/red_knot_python_semantic/resources/mdtest/exception/control_flow.md
@@ -92,15 +92,15 @@ def could_raise_returns_str() -> str:
 x = 1
 
 try:
-    reveal_type(x)  # revealed: Literal[1]
+    reveal_type(x)
     x = could_raise_returns_str()
-    reveal_type(x)  # revealed: str
+    reveal_type(x)  # revealed: int
 except TypeError:
     reveal_type(x)  # revealed: Literal[1] | str
     x = 2
-    reveal_type(x)  # revealed: Literal[2]
+    reveal_type(x)
 
-reveal_type(x)  # revealed: str | Literal[2]
+reveal_type(x)  # revealed: str
 ```
 
 ## Multiple `except` branches

I agree in most cases the indentation feels unnecessary though. Ideally I feel like I'd like the second line of the "spillover" for very long error messages to be indented, rather than the first line. Not sure how tricky that would be.

@AlexWaygood AlexWaygood added the testing Related to testing Ruff itself label Oct 23, 2024
@MichaReiser
Copy link
Member Author

Ideally I feel like I'd like the second line of the "spillover" for very long error messages to be indented, rather than the first line. Not sure how tricky that would be.

It depends. If we would want to indent after every newline, then this Formatter adapter might be useful
Than

But that's not what we need here because there's no newline. The line wraps because your terminal doesn't have enough space to render the entire line. A trivial solution could be chunk messages into 60 char long parts and render them by part. I think the long-term solution is that we build a proper diagnostic system that supports showing rich content. It helps reducing the need for long messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions especially welcome red-knot Multi-file analysis & type inference testing Related to testing Ruff itself
Projects
None yet
Development

No branches or pull requests

2 participants