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

Regression: panic messages about invalid mdtests are now swallowed by mdtest #15317

Closed
AlexWaygood opened this issue Jan 7, 2025 · 1 comment · Fixed by #15319
Closed

Regression: panic messages about invalid mdtests are now swallowed by mdtest #15317

AlexWaygood opened this issue Jan 7, 2025 · 1 comment · Fixed by #15319
Assignees
Labels
red-knot Multi-file analysis & type inference testing Related to testing Ruff itself

Comments

@AlexWaygood
Copy link
Member

Following #15241, some of the internal error messages from red_knot_test are now swallowed by the framework, for example this one here:

let suite = match test_parser::parse(short_title, &source) {
Ok(suite) => suite,
Err(err) => {
panic!("Error parsing `{path}`: {err:?}")
}
};

On a PR branch I was working on, I got this (not very informative!) test failure:

image

If I revert 75015b0 on that PR branch, I get this instead, which is much more helpful:

image

@AlexWaygood AlexWaygood added testing Related to testing Ruff itself red-knot Multi-file analysis & type inference labels Jan 7, 2025
@AlexWaygood AlexWaygood added the help wanted Contributions especially welcome label Jan 7, 2025
@dcreager dcreager self-assigned this Jan 7, 2025
@AlexWaygood AlexWaygood removed the help wanted Contributions especially welcome label Jan 7, 2025
@dcreager
Copy link
Member

dcreager commented Jan 7, 2025

The panic that is being swallowed is not within the closure that is wrapped in catch_unwind. I think the issue here is that the panic hook is global, and so if you're running tests concurrently, one thread might install the custom panic hook for its catch_unwind call while another thread is in the middle of e.g. this parse operation that might fail.

dcreager added a commit that referenced this issue Jan 8, 2025
…15319)

This fixes #15317. Our `catch_unwind` wrapper installs a panic hook that
captures (the rendered contents of) the panic info when a panic occurs.
Since the intent is that the caller will render the panic info in some
custom way, the hook silences the default stderr panic output.

However, the panic hook is a global resource, so if any one thread was
in the middle of a `catch_unwind` call, we would silence the default
panic output for _all_ threads.

The solution is to also keep a thread local that indicates whether the
current thread is in the middle of our `catch_unwind`, and to fall back
on the default panic hook if not.

## Test Plan

Artificially added an mdtest parse error, ran tests via `cargo test -p
red_knot_python_semantic` to run a large number of tests in parallel.
Before this patch, the panic message was swallowed as reported in
#15317. After, the panic message was shown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference testing Related to testing Ruff itself
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants