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.

Sign up for GitHub

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

Jump to bottom

chore: make the test-ci-all work harder #4524

Merged
merged 4 commits into from
Mar 12, 2024
Merged

chore: make the test-ci-all work harder #4524

merged 4 commits into from
Mar 12, 2024

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Mar 10, 2024

No description provided.

@dpc dpc requested review from a team as code owners March 10, 2024 21:56
bradleystachurski
bradleystachurski previously approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@bradleystachurski bradleystachurski left a comment

Choose a reason for hiding this comment

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

Verified just test-ci-all works locally, but I'm seeing errors on back-compat.

22867   │ 00:00:25 Error: polling lightningd after 10 retries
22868   │ 00:00:25
22869   │ 00:00:25 Caused by:
22870   │ 00:00:25     0: connect to lightningd
22871   │ 00:00:25     1: No such file or directory (os error 2)
22872   │ ## FAILED: devimint_cli_test (FM: current, CLI: current, GW: v0.2.1)

I'll continue tweaking params on my end. Approving since I think it's safe to merge and followup.

@m1sterc001guy If you have a chance I'm curious if this resolves the issues you had locally.

@dpc
Copy link
Contributor Author

dpc commented Mar 11, 2024

Verified just test-ci-all works locally, but I'm seeing errors on back-compat.

For this one we should just increase the time. 10s is not a lot. 30s seems more reasonable.

@bradleystachurski
Copy link
Member

I bumped to 30 and now seeing networking errors with the bitcoind client

00:00:21 2024-03-11T23:32:23.403138Z  WARN devimint: Failed to create wallet ... retrying JSON-RPC error: transport error: Couldn't connect to host: Connection refused (os error 111)
00:00:24 2024-03-11T23:32:26.068629Z  INFO devimint::external: Beginning to mine 101 blocks to address bcrt1qj06wd2nptduccl jvp52k84ynlkv9s2ehmdn30a
00:01:09 2024-03-11T23:33:11.942712Z  INFO devimint: sending SIGKILL to bitcoind and waiting for it to exit
00:01:09 Error: Failed to generate blocks
00:01:09
00:01:09 Caused by:
00:01:09     JSON-RPC error: transport error: Didn't receive response data in time, timed out.
## FAILED: devimint_cli_test (FM: current, CLI: v0.2.1, GW: current)

@dpc
Copy link
Contributor Author

dpc commented Mar 12, 2024 •
edited

@bradleystachurski All, right. I've lowered the default load again, and added ability to customize it locally:

> cat .shrc.local
export FM_TEST_CI_ALL_MAX_LOAD=40000
export FM_TEST_CI_ALL_DELAY=.5

drop a file in there like that, tune to your liking, reload the dev shell and it should do the job.

bradleystachurski
bradleystachurski approved these changes Mar 12, 2024
View reviewed changes
Copy link
Member

@bradleystachurski bradleystachurski left a comment

Choose a reason for hiding this comment

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

.shrc.local is a solid solution. Making tweaks locally allowed just test-compatibility to pass again 👍

justinmoon
justinmoon approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@justinmoon justinmoon left a comment

Choose a reason for hiding this comment

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

Didn’t test

@justinmoon justinmoon added this pull request to the merge queue Mar 12, 2024
@dpc dpc mentioned this pull request Mar 12, 2024
Merged
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2024
@justinmoon justinmoon added this pull request to the merge queue Mar 12, 2024
Merged via the queue into fedimint:master with commit 0e5c76c Mar 12, 2024
20 checks passed
elsirion
elsirion reviewed Mar 15, 2024
View reviewed changes
devimint/src/tests.rs
@@ -288,7 +288,7 @@ pub async fn latency_tests(dev_fed: DevFed, r#type: LatencyTest) -> Result<()> {
let fm_pay_stats = stats_for(fm_internal_pay);

println!("### LATENCY FM PAY: {fm_pay_stats}");
assert!(fm_pay_stats.median < Duration::from_secs(12));
assert!(fm_pay_stats.median < Duration::from_secs(15));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe latency tests should not run in parallel on the same machine? This hack may hide latency introduced from round trips that don't require much compute?

Copy link
Member

Choose a reason for hiding this comment

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

Good callout 👍

Made an issue to make sure we keep track #4579.

@dpc dpc deleted the 24-03-10-ci-go-faster branch March 15, 2024 15:09
@bradleystachurski bradleystachurski mentioned this pull request Mar 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradleystachurski bradleystachurski bradleystachurski approved these changes

@elsirion elsirion elsirion left review comments

@justinmoon justinmoon justinmoon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dpc @bradleystachurski @justinmoon @elsirion