Skip to content

test: improve functional tests compatibility with BSD/macOS #19368

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
Merged
laanwj merged 2 commits into from
Jul 1, 2020
Merged

test: improve functional tests compatibility with BSD/macOS #19368

laanwj merged 2 commits into from
Jul 1, 2020

Conversation

@S3RK
Copy link
Contributor

@S3RK S3RK commented Jun 24, 2020 •
edited
Loading

Rationale: a few minor changes to make experience of running tests on macOS a bit better
1.pidof is not available on BSD/macOS, while pgrep is present on BSD, Linux and macOS
2. Add retry as a workaround for a weird behavior when writing to a socket (https://bugs.python.org/issue33450). Stacktrace attached

Man pages:
https://www.freebsd.org/cgi/man.cgi?query=pgrep&apropos=0&sektion=1&manpath=FreeBSD+6.0-RELEASE&arch=default&format=html
https://man7.org/linux/man-pages/man1/pgrep.1.html

Related to #19281

Stacktrace example:

...
33/161 - feature_abortnode.py failed, Duration: 63 s

stdout:
2020-06-11T10:46:43.947000Z TestFramework (INFO): Initializing test directory /var/folders/2q/d5w9zh614r7g5c8r74ln3g400000gq/T/test_runner_₿_🏃_20200611_174102/feature_abortnode_128
2020-06-11T10:46:45.199000Z TestFramework (INFO): Waiting for crash
2020-06-11T10:47:15.921000Z TestFramework (INFO): Node crashed - now verifying restart fails
2020-06-11T10:47:47.068000Z TestFramework (INFO): Stopping nodes
[node 1] Cleaning up leftover process


stderr:
Traceback (most recent call last):
  File "/Users/xxx/Projects/bitcoin/test/functional/feature_abortnode.py", line 50, in <module>
    AbortNodeTest().main()
  File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
    exit_code = self.shutdown()
  File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 266, in shutdown
    self.stop_nodes()
  File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_framework.py", line 515, in stop_nodes
    node.stop_node(wait=wait)
  File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/test_node.py", line 318, in stop_node
    self.stop(wait=wait)
  File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/authproxy.py", line 142, in __call__
    response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
  File "/Users/xxx/Projects/bitcoin/test/functional/test_framework/authproxy.py", line 107, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 1107, in request
    self._send_request(method, url, body, headers)
  File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 1152, in _send_request
    self.endheaders(body)
  File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 1103, in endheaders
    self._send_output(message_body)
  File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 936, in _send_output
    self.send(message_body)
  File "/Users/xxx/.pyenv/versions/3.5.6/lib/python3.5/http/client.py", line 908, in send
    self.sock.sendall(data)
OSError: [Errno 41] Protocol wrong type for socket

@S3RK
test: use pgrep for better compatibility
8cf9d15 
pidof is not available on BSD system, while pgrep is present on BSD, Linux and macOS
@fanquake fanquake added the Tests label Jun 24, 2020
@S3RK
test: retry when write to a socket fails on macOS
3a7e794 
If the socket is tearing down macOS will return EPROTOTYPE instead of EPIPE.
Because python doesn't handle this internally we have to do a workaround and retry the request.
See https://bugs.python.org/issue33450
@maflcko
Copy link
Member

maflcko commented Jun 25, 2020

Oh nice. Does that mean we can enable the tests again?

bitcoin/ci/test/00_setup_env_mac_host.sh

Line 14 in 67881de

export TEST_RUNNER_EXTRA="wallet_disable" # Only run wallet_disable as a smoke test, see https://github.com/bitcoin/bitcoin/pull/17240#issuecomment-546022121 why the other tests are disabled

@S3RK
Copy link
Contributor Author

S3RK commented Jun 26, 2020

@MarcoFalke oh, I didn't even know we had already experienced this problem, I thought it's just me 😄 I've looked at the comment and related travis logs and indeed the error is the same as in my stacktrace.

From what I understood the behaviour we see on macOS is similar to BrokenPipeError and ConnectionResetError on linux/BSD, it's also triggered by reset connection. So It's reasonable to expect that the same workaround (reconnect and retry) would work. Though it is still a mystery to me why the connection would be reset in the first place, but I'm afraid that's beyond my understanding.

I think we can try to enable the tests again. Would you suggest to have some prior discussion on IRC maybe or just re-enable the tests in this PR and let reviewers decide?

@maflcko
Copy link
Member

maflcko commented Jun 26, 2020

just re-enable the tests in this PR and let reviewers decide?

That seems fine

laanwj
laanwj reviewed Jun 29, 2020
View reviewed changes
@laanwj
Copy link
Member

laanwj commented Jun 29, 2020

ACK 3a7e794
Verified that the pgrep works on FreeBSD whereas pidof does not. It's somewhat of a pity that we need to rely on a shell commend being present here at all, but for this single optional check it's also not worth introducing an dependency on, say, the psutil module.

@laanwj
Copy link
Member

laanwj commented Jun 29, 2020

Though it is still a mystery to me why the connection would be reset in the first place, but I'm afraid that's beyond my understanding.

There are various edge cases where a connection reset can happen. For example at shutdown it's not guaranteed that stop RPC command will return a value. It usually does, but on a busy system it might not, and the RPC server is shut down before it manages to send the reply.

@laanwj laanwj merged commit f6072e6 into bitcoin:master Jul 1, 2020
@S3RK S3RK deleted the test_improved_process_detection branch July 1, 2020 15:48
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 28, 2021
@S3RK @PiRK
test: improve functional tests compatibility with BSD/macOS
27ed27a 
Summary:
> test: use pgrep for better compatibility
>
> pidof is not available on BSD system, while pgrep is present on BSD, Linux and macOS

> test: retry when write to a socket fails on macOS
>
> If the socket is tearing down macOS will return EPROTOTYPE instead of EPIPE.
> Because python doesn't handle this internally we have to do a workaround and retry the request.
> See https://bugs.python.org/issue33450

This is a backport of [[bitcoin/bitcoin#19368 | core#19368]]

Test Plan:
```
test/functional/test_runner.py
```
Run it with and without `bitcoind` already running to check that it still detects it.

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9968
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@S3RK @maflcko @laanwj @fanquake