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.

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

Adjust farmer service SSL tests for increased reliability #17921

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

emlowe
Copy link
Contributor

@emlowe emlowe commented Apr 26, 2024

Currently, the SSL tests specifically one test for the farmer is flaky - this seems to have been recently introduced possibly due to changes in aiohttp, but the exact reasons for flakiness are unknown

The end goal of the test is to make sure the server rejects the SSL connection - it seems that due to aiohttp internals, SSL errors are typically silent on the server and the client may not get a consistent error for why the connection was dropped. (see aio-libs/aiohttp#7325)

Adjust the flaky test as follows:
Using asyncio debug mode and setting the test to capture the log, we can look for the SSL certificate failure in the log files as it is output when asyncio is in debug mode. This is further complicated in that on some platforms, when asyncio is in debug mode, an exception is raised that is not catchable by pytest and fails the test - so a new exception handler is used to ignore the SSL exception, as we are only testing the log output.

This is currently very stable in CI. This replaces the fix in #17899

I think the value of this particular test is debatable; I currently don't think there is any way to misconfigure the code such that this test would pass - it would have to be some error in SSL, or if all SSL cert verification was turned off.

@emlowe emlowe added CI CI changes Changed Required label for PR that categorizes merge commit message as "Changed" for changelog labels Apr 30, 2024
@emlowe emlowe changed the title Trying some SSL hacks for tests Adjust SSL tests for increased reliability Apr 30, 2024
@emlowe emlowe marked this pull request as ready for review April 30, 2024 19:21
@emlowe emlowe requested a review from a team as a code owner April 30, 2024 19:21
@emlowe emlowe requested review from altendky and arvidn April 30, 2024 19:21
@emlowe emlowe changed the title Adjust SSL tests for increased reliability Adjust farmer service SSL tests for increased reliability Apr 30, 2024
@altendky altendky closed this May 3, 2024
@altendky altendky reopened this May 3, 2024
chia/_tests/core/ssl/test_ssl.py Outdated Show resolved Hide resolved
chia/_tests/core/ssl/test_ssl.py Outdated Show resolved Hide resolved
chia/_tests/core/ssl/test_ssl.py Outdated Show resolved Hide resolved
chia/_tests/core/ssl/test_ssl.py Outdated Show resolved Hide resolved
Copy link

coveralls-official bot commented May 3, 2024

Pull Request Test Coverage Report for Build 8944179727

Details

  • 48 of 48 (100.0%) changed or added relevant lines in 1 file are covered.
  • 9 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.03%) to 90.697%

Files with Coverage Reduction New Missed Lines %
chia/rpc/rpc_server.py 1 88.67%
chia/wallet/util/wallet_sync_utils.py 1 86.54%
chia/timelord/timelord_launcher.py 1 69.92%
chia/server/node_discovery.py 1 80.5%
chia/introducer/introducer.py 1 78.26%
chia/wallet/wallet_node.py 4 88.6%
Totals Coverage Status
Change from base Build 8943202463: 0.03%
Covered Lines: 98057
Relevant Lines: 108058

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog CI CI changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants