This PR modifies run-make tests.

cc @jieyouxu

For should_panic fix, instead of checking the exit code, we directly wrap the doctest code with catch_unwind and if a panic happened, then we return success for the test, otherwise we display the appropriate error message.

I... do not see anything like that in the patch? There's two checks for exit code 101.

Woups, copied/pasted comment from original PR which was outdated after discussion with you (on the previous PR). ^^'

EDIT: Updated comment.

if langstr.should_panic {
    if out.status.code() == Some(101) {
        return Ok(());
    } else if out.status.success() {
        return Err(TestFailure::UnexpectedRunPass);
    }
}
if !out.status.success() {
    return Err(TestFailure::ExecutionFailure(out));
}

Do you think something like this would make sense, so that it's easier for the user to differentiate between aborts and panic-less success?

TestFailure::UnexpectedRunPass displays "Test didn't panic, but it's marked `should_panic`.", which means we'd need to add a new case. Do you think it's necessary to differentiate between success and non-panic failures?

Wouldn't changing UnexpectedRunPass to spell "Test executable succeeded, but it's marked should_panic." (i.e. rolling back the change in this PR) work? After all, if the change was to let UnexpectedRunPass represent non-panicking failures, it won't be necessary if we just use ExecutionFailure for that.

That wouldn't cover exit(1) anymore if we did so.

I've amended my comment a moment before you replied, apologies.

Replied too fast then. 😅

The message for ExecutionFailure is too general imo ("Test executable failed"). It doesn't help to understand should_panic didn't panic. I think once this PR is merged, you can send a new one to create a new variant in case it failed but didn't panic, like that we can have a message that is covering this case. What do you think?

The message for ExecutionFailure is too general imo ("Test executable failed"). It doesn't help to understand should_panic didn't panic.

I think we're talking past each other. I suggest that:

• If the program didn't panic and exited with code 0, we keep using UnexpectedRunPass ("test didn't panic"). This is the main use case for should_panic and has a perfectly legible description.
• If the program didn't panic and exited with a non-zero code, we use ExecutionFailure ("executable failed"). I believe that it's highly unlikely this failure mode of a should_panic test can be attributed to anything but UB or an abort, exactly like with non-should_panic tests, so I don't think it's necessary to specify that it's happened in a should_panic test specifically.

Wouldn't that work?

I still think it's too general. For end users, should_panic means the execution is supposed to fail. However it might be confusing to get "executable failed" when it's exactly what's expected (well, except it was expecting a panic and it's different). Hence why I think the current message is better and why, if we go down this road, we should add a new case to handle this case instead of relying on the too generated ExecutionFailure.

That makes sense, yeah. Let's delay it until after this PR lands then.

☔ The latest upstream changes (presumably #144692) made this pull request unmergeable. Please resolve the merge conflicts.

@bors try jobs=test-various

☀️ Try build successful (CI)
Build commit: 377ca7d (377ca7d24552618d3d6172717818ab880507e9e5, parent: 3d8c1c1fc077d04658de63261d8ce2903546db13)

Finally! \o/

@bors r=fmease rollup

📌 Commit 560d450 has been approved by fmease

It is now in the queue for this repository.

Possibly failed in rollup: #147435 (comment)

@bors r-

@bors try jobs=x86_64-gnu-aux

💔 Test for 75572d4 failed: CI. Failed jobs:

• try - x86_64-gnu-aux (web logs, extended logs)

Ah, this time a cargo test failed. Checking what's wrong.

@bors try jobs=x86_64-gnu-aux,test-various

☀️ Try build successful (CI)
Build commit: 1514cdd (1514cddf55f57c7a67702bced791e2db666e031f, parent: 4a54b26d30dac43778afb0e503524b763fce0eee)

This failure definitely illustrates why I should finish writing the lint in case the merged doctests failed compilation...