fix: JSON should be set to "success": false when any transactions failed

[Klaim]

Description

I found some cases while working on #4126 where the JSON output would set "success": true while MTransaction::execution actually failed, leading to potentially succeeding integration tests which check for that value.

This PR attempts to make the JSON output correct and reveal tests that are actually not passing.

Type of Change

• Bugfix
• Feature / enhancement
• CI / Documentation
• Maintenance

Checklist

• My code follows the general style and conventions of the codebase, ensuring consistency
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have run pre-commit run --all locally in the source folder and confirmed that there are no linter errors.
• I have added tests that prove my fix is effective or that my feature works
• New and existing tests pass locally with my changes

[codecov]

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.75%. Comparing base (115e080) to head (8922f36).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
libmamba/include/mamba/core/output.hpp	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4201      +/-   ##
==========================================
- Coverage   53.54%   52.75%   -0.79%     
==========================================
  Files         239      239              
  Lines       29275    29278       +3     
  Branches     3107     3108       +1     
==========================================
- Hits        15675    15446     -229     
- Misses      13597    13829     +232     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jjerphan]

The failure of TestMultiplePkgCaches::test_no_writable_extracted_dir_corrupted comes from those changes from #4123 (mamba does not actually support read-only caches, does it?) and which should be reverted IMO.

See: #4213

[Klaim]

@jjerphan Weird: that ci failure wasnt visible in your pr but it's the same sequence of commits here. I'm suspecting a timing issue, although the test is about detecting a "hang". I'll re-run to be sure but I think maybe that timeout is optimistically short?

[jjerphan]

We can increase the value for the timeout (we have done that several time in the past).

[jjerphan]

Thank you.

Are there way we can test it?

[Klaim]

Are there way we can test it?

I'm not sure if we actually can test it in a way that's always correct. A command that should always fail in the transaction execution?
Any suggestion?

[jjerphan]

What if we SIGINT an installation of an environment?

[Klaim]

What if we SIGINT an installation of an environment?

If I understand correctly, that would rely on timing to trigger the exception in the MTransaction::execution function, it wouldnt be very reliable?

How about attempting to get a package from an impossible url? I suspect that might fail before that function though. Maybe with an incorrect build id?

[jjerphan]

We should be able to use an explicit environment specification with a faulty package.

[Klaim]

We should be able to use an explicit environment specification with a faulty package.

I suspect we might have one somewhere in the tests, I'll look into it or create it👍🏼

[JohanMabille]

LGTM once the linter is fixed.