Skip to content

Conversation

@emlowe
Copy link
Contributor

@emlowe emlowe commented Dec 8, 2025

This test didn't work properly as the time_out_asserts were not awaited (but rather asserted - I'm assuming a typo - but this was not checked by linting)

Once changed to proper await one of the checks didn't work at all, so I adjusted the code somewhat to rely on parsing LOG messages

Discovered during Python 3.14 testing which has better checks for missing awaits


Note

Stabilizes test_long_sync_untrusted_break by awaiting timeouts, simulating untrusted sync via wait_until_closed, and verifying trusted disconnect and untrusted connection closure through log assertions.

  • Tests:
    • chia/_tests/wallet/sync/test_wallet_sync.py:
      • In test_long_sync_untrusted_break:
        • Replace long sleep with peer.wait_until_closed() in mocked register_for_ph_updates.
        • Combine patch_request_handler with caplog.at_level(logging.INFO); assert start_client success and capture untrusted connection.
        • Convert assert time_out_assert(...) to await time_out_assert(...); await synced_to_trusted and only_trusted_peer checks.
        • Assert log messages for trusted-peer-triggered disconnect and untrusted connection closure.
        • Remove sync_canceled flag and related checks.

Written by Cursor Bugbot for commit ee7b9da. This will update automatically on new commits. Configure here.

@emlowe emlowe requested a review from a team as a code owner December 8, 2025 22:10
@emlowe emlowe added CI CI changes Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog labels Dec 8, 2025
@emlowe emlowe requested a review from AmineKhaldi December 8, 2025 22:12
AmineKhaldi
AmineKhaldi previously approved these changes Dec 9, 2025
Copy link
Contributor

@AmineKhaldi AmineKhaldi left a comment

Choose a reason for hiding this comment

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

Nicely done.

@emlowe emlowe requested a review from arvidn December 9, 2025 15:44
@emlowe emlowe mentioned this pull request Dec 9, 2025
21 tasks
arvidn
arvidn previously approved these changes Dec 13, 2025
@arvidn
Copy link
Contributor

arvidn commented Dec 13, 2025

nice catch. I take it you grepped for other instances of assert time_out_assert?

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Dec 15, 2025
@emlowe
Copy link
Contributor Author

emlowe commented Dec 15, 2025

I did search for other instances and I didn't find any

@emlowe emlowe dismissed stale reviews from arvidn and AmineKhaldi via 95b8656 December 15, 2025 18:30
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Dec 15, 2025
@emlowe emlowe requested review from AmineKhaldi and arvidn December 15, 2025 18:31
@coveralls-official
Copy link

Pull Request Test Coverage Report for Build 20248433891

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 23 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.02%) to 90.819%

Files with Coverage Reduction New Missed Lines %
chia/full_node/full_node_api.py 1 86.75%
chia/server/address_manager.py 1 92.7%
chia/timelord/timelord_api.py 1 93.02%
chia/wallet/util/wallet_sync_utils.py 1 85.57%
chia/server/node_discovery.py 2 79.25%
chia/timelord/timelord_launcher.py 2 70.0%
chia/introducer/introducer_api.py 5 81.48%
chia/wallet/wallet_node.py 10 86.03%
Totals Coverage Status
Change from base Build 20242846771: 0.02%
Covered Lines: 102850
Relevant Lines: 113077

💛 - Coveralls

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

Labels

CI CI changes Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants