fedimint / fedimint

Federated E-Cash Mint
https://fedimint.org/
MIT License
536 stars 210 forks source link

Degraded federations for devimint and rust tests #4247

Closed bradleystachurski closed 2 months ago

bradleystachurski commented 3 months ago

Closes https://github.com/fedimint/fedimint/issues/4171

Inspired by https://github.com/fedimint/fedimint/pull/4173 and https://github.com/fedimint/fedimint/pull/4198

Introduces the ability to run rust and devimint tests with a parameterized number of peers offline, defaulting to a 3/4 setup.

Running all tests with a degraded federation increases the time for CI to finish by a material amount, so we should consider if running all tests degraded is worth the additional overhead.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 8.84956% with 103 lines in your changes are missing coverage. Please review.

Project coverage is 57.96%. Comparing base (52f0c96) to head (dba55b8). Report is 174 commits behind head on master.

:exclamation: Current head dba55b8 differs from pull request most recent head a5e00f8. Consider uploading reports for the commit a5e00f8 to get more accurate results

Files Patch % Lines
devimint/src/tests.rs 0.00% 34 Missing :warning:
fedimint-load-test-tool/src/main.rs 0.00% 22 Missing :warning:
devimint/src/util.rs 0.00% 12 Missing :warning:
fedimint-load-test-tool/src/common.rs 0.00% 12 Missing :warning:
devimint/src/devfed.rs 0.00% 9 Missing :warning:
devimint/src/cli.rs 0.00% 6 Missing :warning:
devimint/src/vars.rs 0.00% 4 Missing :warning:
devimint/src/federation.rs 0.00% 3 Missing :warning:
fedimint-testing/src/fixtures.rs 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4247 +/- ## ========================================== - Coverage 58.06% 57.96% -0.10% ========================================== Files 197 197 Lines 43755 43789 +34 ========================================== - Hits 25405 25383 -22 - Misses 18350 18406 +56 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

justinmoon commented 3 months ago

dev call: would be nice to merge flakiness offensive, then rebase this one it

elsirion commented 2 months ago

Would be good if our devimint wizard @maan2003 could take a look before merging since I'm not as familiar with it :)

bradleystachurski commented 2 months ago

Thanks everyone for the reviews! I should be able to address/respond to all feedback in the next day or two. If this is approved/merged in the meantime, I'll make a followup PR.

bradleystachurski commented 2 months ago

Rebased and resolved merge conflicts, still need to address feedback 🙂

bradleystachurski commented 2 months ago

Dang, reached the 75 minute timeout in the merge queue for the 5x tests. Testing a timeout bump in a separate PR https://github.com/fedimint/fedimint/pull/4342

dpc commented 2 months ago

Dang, reached the 75 minute timeout in the merge queue for the 5x tests. Testing a timeout bump in a separate PR #4342

Wen parallel tests?

bradleystachurski commented 2 months ago

Wen parallel tests?

I don't think parallel back-compat would help for the 5x tests, but I think exploring this may help: https://github.com/fedimint/fedimint/pull/4247#discussion_r1496571057

dpc commented 2 months ago

Wen parallel tests?

I don't think parallel back-compat would help for the 5x tests, but I think exploring this may help: #4247 (comment)

Oh. OK. https://github.com/fedimint/fedimint/pull/4390

But also https://github.com/fedimint/fedimint/pull/4390

And then: Wen parallel tests, @bradleystachurski ? :D

bradleystachurski commented 2 months ago

Capturing an out-of-band chat with @dpc:

bradleystachurski commented 2 months ago

Rebased with the latency parallel improvements

justinmoon commented 2 months ago

This looks great to me. But I'm a little out-of-context so I'm going to defer "approval" to @dpc @elsirion and @maan2003.

bradleystachurski commented 2 months ago

Dangit, backend_test_bitcoind is running much slower in CI than locally and timing out at 600s. Investigating...