-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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] Report line numbers in mdtest relative to the markdown file, not the test snippet #13798
Comments
I prefer the way it is. It's closer to the cli behavior and the diagnostic contains the file name |
This would also lead to fragile tests where changing a test description can change the asserted line numbers. Keeping the line numbers relative to the file ensures that only local changes affect the diagnostic line numbers |
@carljm and I were finding it extremely difficult to debug failing tests in #13800 just now with the current behaviour :/
I'm not sure I understand or agree. The CLI never runs on Markdown files; it runs on Python files. If you run the CLI on a file |
I think it's important to clarify here that we are not talking about line numbers in Red Knot diagnostics; we are only talking about line numbers in mdtest error messages when a test fails. I definitely don't think mdtest should ever modify line numbers as they appear in a Red Knot diagnostic.
This is not an issue, we never assert in tests based on line numbers (instead we just put the assertion on or above the relevant line). And this seems to be based on a misunderstanding that we are talking about line numbers in red-knot diagnostics, rather than line numbers in mdtest failure messages. The only use for a line number in an mdtest failure message is to help you quickly find where in your Markdown test file the error occurred. So the only relevant question here is what makes that fast and easy. I think it's 100% clear that jumping to line X in your editor in the Markdown file is easier than first finding the right test header, then finding the right embedded file, then manually counting lines from the top of the code block. |
I can see the pain point that but improving the rendering of diagnostics might help to make this more obvious. I kind of agree that it's different from the CLI and possibly closer to how diagnostics in embedded files should work. But the fact that it makes tests more fragile (changing a single description can break many assertions) is a no go for me unless we make those line numbers relative (which is also annoying because you then need to count the lines manually). That's why I would keep it as is. Adjusting the line numbers would also complicate the diagnostic rendering (not how it is done today but when we have a proper diagnostic system) |
I'm not sure what's unclear about the error messages in the linked test. Some markdown editors support showing line numbers for code blocks. That might help https://stackoverflow.com/a/65579102 |
but it doesn't
I assume you're talking about Red Knot diagnostic rendering? The change proposed here has nothing to do with Red Knot diagnostics at all. |
To clarify the point I made above, let's say that in future we have (and match on) red-knot diagnostics that include a line number, and we are asserting against a full diagnostic message, including the line number (which would usually be a bad idea because it's fragile to changes in the test, but maybe sometime we want to do it.) So today in that case, if your test fails because of a line number mismatch, mdtest might tell you something like this:
Oops, we asserted a red-knot diagnostic including the text "line 8" but we actually got one including the text "line 9". And mdtest helpfully tells us this problem occurs on line 9 of the embedded So to debug this, we have to open The proposed change here is that mdtest would instead output something like this:
Line numbers in red-knot diagnostics are untouched; you still see that from the perspective of the embedded file that red-knot sees, the discrepancy in red-knot diagnostic is line 8 vs 9. But now you also can find the problematic assertion in your test by going directly to line 36 of Given my experience working with mdtest today, I think this change (or something providing equivalent information) is a requirement for usability of mdtest. I don't know which editors show line numbers inside markdown code blocks; I don't think I've used one, and I don't think that's a reasonable editor requirement to make mdtest usable. |
That makes sense. Thanks for explaining and sorry for the confusion on my side. We may want to use a format that many editors recognize in the shell. E.g path/test.md:line Error so that you can directly jump to the relevant line |
Yes, good idea! |
Currently if an assertion in
mdtest
fails, it gives you a message as follows:The line number here isn't particularly helpful if it's the third or fourth snippet in a Markdown test suite, because it gives you the line number relative to the specific test snippet rather than the Markdown file. Nobody wants to manually count thirteen lines in a Python code block to figure out where the assertion failed!
It would be great if these line numbers could be reported relative to the enclosing Markdown file rather than just the specific code block that contained the failing assertion.
The text was updated successfully, but these errors were encountered: