Skip to content

Conversation

@mitchmindtree
Copy link
Collaborator

@mitchmindtree mitchmindtree commented Dec 3, 2025

Motivation

To-Do

@mitchmindtree mitchmindtree self-assigned this Dec 3, 2025
This is achieved by:

- Adding a `CliError` `tests_failed` variant.
- Returning this error if `total_passed < total`.
Also adds a dedicated test ensuring a failed test produces a non-zero
exit code.
@mitchmindtree mitchmindtree force-pushed the mitchmindtree/test-exit-code branch from 1a3c190 to 9ea9b4d Compare December 4, 2025 06:18
Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @mitchmindtree! Worth mentioning that there's a few places in the CLI where we don't exit with good errors, broadcast failures for one.

@mitchmindtree
Copy link
Collaborator Author

Thanks @d0cd! I'll keep an eye out for these other sites where we should be exiting with nice errors too.

I noticed that the whole process is actually panic!ing on the assertion in the new test here (see the new STDERR expectations). As a result, rather than seeing a non-zero exit code, I'm running into this:

I've attempted to workaround the VM panic issue by using catch_unwind on the VM deploy step (similar to @mohammadfawaz's workaround in #29022), but it appears the panic! is occurring in a non-UnwindSafe area of code, so the panic! propagates up regardless. I'm finishing up for the day on this end, but I've left some thoughts on the situation here and in the following comment.

I'm thinking I'll remove the new test for now, but leave an issue to re-add once #28992 is addressed.

@d0cd
Copy link
Collaborator

d0cd commented Dec 7, 2025

It doesn't fix the core issue, but if you adjust the logic so that the assertion isn't trivially false, you should be able to deploy it. One way to do this is make it depend on some input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] leo test should exit with a non-zero status code

3 participants