test: add factor option to adjust test timeouts #18617

pull brakmic wants to merge 1 commits into bitcoin:master from brakmic:test-timeouts-with-factor changing 5 files +30 −20
  1. brakmic commented at 8:52 am on April 13, 2020: contributor

    This PR adds a new option factor that can be used to adjust timeouts in various functional tests. Several timeouts and functions from authproxy, mininode, test_node and util have been adapted to use this option. The factor-option definition is located in test_framework.py.

    Fixes #18266 Also Fixes https://github.com/bitcoin/bitcoin/issues/18834

  2. fanquake added the label Tests on Apr 13, 2020
  3. in test/functional/test_framework/mininode.py:588 in f35acfc01c outdated 
    584@@ -585,7 +585,7 @@ def send_blocks_and_test(self, blocks, node, *, success=True, force_send=False,
585                     self.send_message(msg_block(block=b))
586             else:
587                 self.send_message(msg_headers([CBlockHeader(block) for block in blocks]))
588-                wait_until(lambda: blocks[-1].sha256 in self.getdata_requests, timeout=timeout, lock=mininode_lock)
589+                wait_until(lambda: blocks[-1].sha256 in self.getdata_requests, timeout=timeout * self.options.factor, lock=mininode_lock)
    MarcoFalke commented at 1:57 pm on April 13, 2020:
    Given that they are all based on wait_until wouldn’t it make more sense to treat wait_until as the basic mechanism for timed waits and only multiply it in there?
    brakmic commented at 1:59 pm on April 13, 2020:
    Meanwhile, I have changed a few other things, because I faced various p2p errors (the P2PInterface didn’t have their factor variable, so I had to redo a few things). And yes, wait_until will be changed too.
  4. DrahtBot commented at 4:39 pm on April 13, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18788 (wallet: tests: Update more tests to work with descriptor wallets by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. brakmic commented at 12:28 pm on April 14, 2020: contributor

    @MarcoFalke

    This one fails on travis, but succeeds on my macOS. Is there anything I could do to make it pass?

    0mempool_reorg.py                                 | ✖ Failed  | 773 s

    Error Log

    0raise AssertionError("Mempool sync timed out:{}".format("".join("\n  {!r}".format(m) for m in pool)))
1AssertionError: Mempool sync timed out:
  6. DrahtBot added the label Needs rebase on Apr 27, 2020
  7. DrahtBot removed the label Needs rebase on Apr 27, 2020
  8. in test/functional/test_framework/mininode.py:369 in 06cc37d064 outdated 
    365@@ -366,7 +366,7 @@ def on_version(self, message):
366 
367     def wait_for_disconnect(self, timeout=60):
368         test_function = lambda: not self.is_connected
369-        wait_until(test_function, timeout=timeout, lock=mininode_lock)
370+        wait_until(test_function, timeout=timeout, lock=mininode_lock, factor=self.factor)
    MarcoFalke commented at 2:07 pm on May 1, 2020: 
    0        self.wait_until(test_function, timeout=timeout)

    Could this be a member function, so that the lock and factor members don’t have to be passed each time?

  9. in test/functional/test_framework/util.py:215 in 06cc37d064 outdated 
    212+def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, factor=1.0):
213     if attempts == float('inf') and timeout == float('inf'):
214-        timeout = 60
215+        timeout = 60 * factor
216+    else:
217+        timeout = timeout * factor
    MarcoFalke commented at 2:11 pm on May 1, 2020: 
    0    timeout = timeout * factor

    can remove the else branch?

  10. in test/functional/test_framework/test_node.py:334 in 06cc37d064 outdated 
    331     @contextlib.contextmanager
332     def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2):
333         if unexpected_msgs is None:
334             unexpected_msgs = []
335-        time_end = time.time() + timeout
336+        time_end = time.time() + int(timeout * self.factor)
    MarcoFalke commented at 2:11 pm on May 1, 2020: 
    0        time_end = time.time() + timeout * self.factor

    why is this needed?

    brakmic commented at 2:45 pm on May 1, 2020:
    I had problems in test_node.py’s wait_for_rpc_connection. More precisely, the polling for-loop that refused to iterate over float values. Hence the conversion to int. But if there is a way to make it work without conversion, I’ll gladly try it out. Right now, I am going through your list of nits and adapting the code.
    MarcoFalke commented at 2:50 pm on May 1, 2020:
    time.time() is a float, so I don’t see how this can cause issues
  11. in test/functional/test_framework/test_node.py:79 in 06cc37d064 outdated 
    75@@ -76,7 +76,7 @@ def __init__(self, i, datadir, *, chain, rpchost, timewait, bitcoind, bitcoin_cl
76         self.stderr_dir = os.path.join(self.datadir, "stderr")
77         self.chain = chain
78         self.rpchost = rpchost
79-        self.rpc_timeout = timewait
80+        self.rpc_timeout = int(timewait)
    MarcoFalke commented at 2:12 pm on May 1, 2020:
    why is this needed, when authproxy does the casting already?
    brakmic commented at 2:53 pm on May 1, 2020:
    In some previous variant of my code I encountered such a problem. Maybe just because I was still writing it. Right now, it seems that all those conversions aren’t needed anymore. I’m just executing some tests and they’re all passing.
  12. in test/functional/test_framework/test_node.py:491 in 06cc37d064 outdated 
    488@@ -488,6 +489,7 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs):
489             kwargs['dstaddr'] = '127.0.0.1'
490 
491         p2p_conn.peer_connect(**kwargs, net=self.chain)()
    MarcoFalke commented at 2:13 pm on May 1, 2020: 
    0        p2p_conn.peer_connect(**kwargs, net=self.chain, factor=self.factor)()
    brakmic commented at 2:57 pm on May 1, 2020:
    I don’t understand this change?
    brakmic commented at 3:00 pm on May 1, 2020:
    Ah, yes, now I see it. It’s being forwarded to derived class P2PInterface!
  13. MarcoFalke approved
  14. MarcoFalke commented at 2:13 pm on May 1, 2020: member
    ACK, just style nits
  15. MarcoFalke commented at 2:14 pm on May 1, 2020: member
    Could you please bump the factor to 2.0 for the valgrind run and mark this pull as fixing #18834 ?
  16. brakmic commented at 2:16 pm on May 1, 2020: contributor

    Could you please bump the factor to 2.0 for the valgrind run and mark this pull as fixing #18834 ?

    Yes, sure, will do it.

  17. in test/functional/test_framework/util.py:211 in e8b50ac9c1 outdated 
    207@@ -208,9 +208,10 @@ def str_to_b64str(string):
208 def satoshi_round(amount):
209     return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
210 
211-def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None):
212+def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, factor=2.0):
    MarcoFalke commented at 10:51 pm on May 2, 2020:

    Why increase the factor for everything? valgrind should be sufficient:

     0diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
 1index d5b38a1d88..c850c13bbe 100644
 2--- a/ci/test/00_setup_env_native_valgrind.sh
 3+++ b/ci/test/00_setup_env_native_valgrind.sh
 4@@ -13,7 +13,7 @@ export NO_DEPENDS=1
 5 if [[ "${TRAVIS}" == "true" && "${TRAVIS_REPO_SLUG}" != "bitcoin/bitcoin" ]]; then
 6   export TEST_RUNNER_EXTRA="wallet_disable"  # Only run wallet_disable as a smoke test to not hit the 50 min travis time limit
 7 else
 8-  export TEST_RUNNER_EXTRA="--exclude rpc_bind"  # Excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
 9+  export TEST_RUNNER_EXTRA="--exclude rpc_bind --factor=2"  # rpc_bind excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
10 fi
11 export GOAL="install"
12 export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no CC=clang CXX=clang++"  # TODO enable GUI
    brakmic commented at 11:07 pm on May 2, 2020:
    True. Have adapted the scripts.
  18. in test/functional/test_framework/util.py:213 in e8b50ac9c1 outdated 
    207@@ -208,9 +208,10 @@ def str_to_b64str(string):
208 def satoshi_round(amount):
209     return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
210 
211-def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None):
212+def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, factor=2.0):
213     if attempts == float('inf') and timeout == float('inf'):
214-        timeout = 60
215+       timeout = 60 * factor
    MarcoFalke commented at 10:51 pm on May 2, 2020: 
    0        timeout = 60
  19. in test/functional/test_framework/test_node.py:328 in e8b50ac9c1 outdated 
    324@@ -324,13 +325,13 @@ def is_node_stopped(self):
325         return True
326 
327     def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT):
328-        wait_until(self.is_node_stopped, timeout=timeout)
329+        wait_until(self.is_node_stopped, timeout=timeout, lock=None, factor=self.factor)
    MarcoFalke commented at 10:55 pm on May 2, 2020: 
    0        wait_until(self.is_node_stopped, timeout=timeout, factor=self.factor)

    None is the default value already

  20. MarcoFalke approved
  21. MarcoFalke commented at 10:55 pm on May 2, 2020: member
    ACK
  22. in test/functional/test_framework/test_framework.py:104 in 341c646dfc outdated 
    100@@ -101,6 +101,7 @@ def __init__(self):
101         self.bind_to_localhost_only = True
102         self.set_test_params()
103         self.parse_args()
104+        self.rpc_timeout = int(self.rpc_timeout * self.options.factor) # optionally, increase timeout by a factor
    MarcoFalke commented at 11:34 pm on May 2, 2020: 
    0        self.rpc_timeout = self.rpc_timeout * self.options.factor # optionally, increase timeout by a factor

    Is this still needed?

    brakmic commented at 11:37 pm on May 2, 2020:

    I think it is. When I execute a test like this it throws the one error we talked about previously (the range poll function).

    0./test/functional/p2p_tx_download.py                                                     (4s 89ms)  
12020-05-02T23:36:00.054000Z TestFramework (INFO): Initializing test directory /var/folders/7q/4ffytzk562dd2ky4bfg9_w7h0000gn/T/bitcoin_func_test_54bxj
2085
32020-05-02T23:36:00.247000Z TestFramework (ERROR): Assertion failed
4Traceback (most recent call last):
5  File "/Users/brakmic/projects/bitcoin-dev/btc-factor/test/functional/test_framework/test_framework.py", line 452, in start_nodes
6    node.wait_for_rpc_connection()
7  File "/Users/brakmic/projects/bitcoin-dev/btc-factor/test/functional/test_framework/test_node.py", line 217, in wait_for_rpc_connection
8    for _ in range(poll_per_s * self.rpc_timeout):
9TypeError: 'float' object cannot be interpreted as an integer
  23. in test/functional/test_framework/util.py:213 in 341c646dfc outdated 
    207@@ -208,9 +208,10 @@ def str_to_b64str(string):
208 def satoshi_round(amount):
209     return Decimal(amount).quantize(Decimal('0.00000001'), rounding=ROUND_DOWN)
210 
211-def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None):
212+def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, factor=1.0):
213     if attempts == float('inf') and timeout == float('inf'):
214-        timeout = 60
215+       timeout = 60
    MarcoFalke commented at 11:34 pm on May 2, 2020:
    why change the whitespace to 3 spaces? Seems unrelated
    brakmic commented at 11:40 pm on May 2, 2020:
    Unintentional. Corrected. :)
  24. MarcoFalke approved
  25. MarcoFalke commented at 11:34 pm on May 2, 2020: member
    ACK
  26. test: add factor option to adjust test timeouts 
    Co-authored-by: MarcoFalke <falke.marco@gmail.com>
    2742c34286
  27. MarcoFalke commented at 0:07 am on May 3, 2020: member
    Thanks! ACK 2742c3428633b6ceaab6714635dc3adb74bf121b
  28. MarcoFalke merged this on May 3, 2020
  29. MarcoFalke closed this on May 3, 2020
  30. brakmic deleted the branch on May 3, 2020
  31. ComputerCraftr referenced this in commit f97a9abc4b on Jun 10, 2020
  32. deadalnix referenced this in commit 6f4e54072a on Jan 22, 2021
  33. DrahtBot locked this on Feb 15, 2022


brakmic MarcoFalke DrahtBot

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me