tests: Add capability to disable RPC timeout in functional tests #18986

pull rajarshimaitra wants to merge 2 commits into bitcoin:master from rajarshimaitra:notimeout-flag changing 5 files +22 −16
  1. rajarshimaitra commented at 6:39 AM on May 16, 2020: contributor

    Many times, especially while debugging RPC callbacks to core using gdb, the test timeout kicks in before the response can get back. This can be annoying and requires restarting the functional test as well as gdb attachment.

    This PR adds a --notimeout flag into test_framework and sets the rpc_timeout accordingly if the flag is set.

    The same effect can be achieved with newly added --factor flag but keeping a separate flag that explicitly disables the timeout can be easier for new testers to find it out and separates its purpose from the --factor flag.

    Requesting review ryanofsky jnewbery as per the IRC discussion.

    Update: After initial round of review, the approach is modified to accommodate the functionality in already existing --factor flag. --factor is changed to --timeout-factor to express its intent better.

  2. fanquake added the label Tests on May 16, 2020
  3. fanquake requested review from jnewbery on May 16, 2020
  4. fanquake requested review from ryanofsky on May 16, 2020
  5. rajarshimaitra force-pushed on May 16, 2020
  6. DrahtBot commented at 8:31 AM on May 16, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  7. rajarshimaitra force-pushed on May 16, 2020
  8. in test/functional/test_framework/test_framework.py:233 in 6fea737de9 outdated 
     228 | @@ -227,6 +229,11 @@ def setup(self):
 229 |              if not self.supports_cli:
 230 |                  raise SkipTest("--usecli specified but test does not support using CLI")
 231 |              self.skip_if_no_cli()
 232 | +
 233 | +        #Diasble timeout if flag is set
    jonatack commented at 8:57 AM on May 16, 2020:

    s/#Diasble/# Disable/

    rajarshimaitra commented at 9:19 AM on May 16, 2020:

    Done.

  9. in test/functional/test_framework/test_framework.py:234 in 6fea737de9 outdated 
     228 | @@ -227,6 +229,11 @@ def setup(self):
 229 |              if not self.supports_cli:
 230 |                  raise SkipTest("--usecli specified but test does not support using CLI")
 231 |              self.skip_if_no_cli()
 232 | +
 233 | +        #Diasble timeout if flag is set
 234 | +        if self.options.notimeout:
    jonatack commented at 9:00 AM on May 16, 2020:

    Should it warn, raise, or not be set if --factor is passed?

    rajarshimaitra commented at 9:20 AM on May 16, 2020:

    I added a message in the help text. But yes, it would be a good idea to enforce it.

    jonatack commented at 9:48 AM on May 16, 2020:

    I'm thinking either --notimeout should override any --factor setting and output a warning if it does, or halt with an explanation. Depending on which, the helps should be updated. Curious what others think.

    rajarshimaitra commented at 12:35 PM on May 16, 2020:

    I think it's better to immediately raise an error stating these two should not be set together. Generally, someone wanting to disable timeout will not have a need to set --factor. Catching that would be sufficient. But there can be an edge case where for some reason they need --factor but also want to attach gdb to the process. Right now that's not possible without much deeper refactoring.

  10. jonatack commented at 9:00 AM on May 16, 2020: member

    Concept ACK

  11. jonatack commented at 9:06 AM on May 16, 2020: member

    @rajarshimaitra: don't forget to add the prefix "test: " to your PR title (see CONTRIBUTING.md for more) :bulb:

  12. in test/functional/test_framework/test_framework.py:174 in 6fea737de9 outdated 
     169 | @@ -170,6 +170,8 @@ def parse_args(self):
 170 |          parser.add_argument("--descriptors", default=False, action="store_true",
 171 |                              help="Run test using a descriptor wallet")
 172 |          parser.add_argument('--factor', type=float, default=1.0, help='adjust test timeouts by a factor')
 173 | +        parser.add_argument("--notimeout", dest="notimeout", default=False, action="store_true",
 174 | +                            help="Disable the test timeout, should not be used with --factor")
    MarcoFalke commented at 9:47 AM on May 16, 2020:

    why not make any negative factor map to factor=inf?

    MarcoFalke commented at 9:49 AM on May 16, 2020:

    (ofc with proper documentation)

    jonatack commented at 9:51 AM on May 16, 2020:

    i like the simplicity of this

    rajarshimaitra commented at 12:14 PM on May 16, 2020:

    That definitely can be done, but I feel it would unnecessarily complicate it for new testers. It won't be readily obvious unless they look it up. Also, I have issues with factor doc, it wasn't clear that it related to rpc_timeout until I look it up. (should it be changed?) Lastly I feel keeping the --notimeout flag explicit, helps differentiate the purpose of two.

    MarcoFalke commented at 12:44 PM on May 16, 2020:

    it wasn't clear that it related to rpc_timeout

    then that sounds like a documentation issue that should be fixed, not worked around

    rajarshimaitra commented at 1:11 PM on May 16, 2020:

    I hope its clear that the work is not around --factor. I have stated that the similar effect can be achieved by just using a higher factor. The addition only helps to reduce the burden of processing that by testers. There are people who are setting env variables and manually changing the code to get this behaviour. Having a flag only simplifies that issue.

  13. rajarshimaitra renamed this:
    Adds a new --notimeout flag in test_framework.
    Test: Adds a new --notimeout flag in test_framework.
    on May 16, 2020
  14. jnewbery commented at 5:44 PM on May 16, 2020: member

    I agree with @MarcoFalke. This functionality already exists with --factor. Rather than adding redundant code, it'd be better to improve the documentation, both in the help text and https://github.com/bitcoin/bitcoin/blob/master/test/README.md#troubleshooting-and-debugging-test-failures.

  15. rajarshimaitra commented at 7:09 PM on May 16, 2020: contributor

    I agree with @MarcoFalke. This functionality already exists with --factor. Rather than adding redundant code, it'd be better to improve the documentation, both in the help text and https://github.com/bitcoin/bitcoin/blob/master/test/README.md#troubleshooting-and-debugging-test-failures.

    Acknowledged. The factor doc can be improved and the debugging doc can be amended with fixing around this possible problem. On the general approach, is it better to disable timeout by setting a negative factor as @MarcoFalke suggested and amending the doc or people should just use a higher factor?

    My personal view is, there should be an easy way to disable the timeout without with or without a new flag.

    Also regarding this point, is it (at least theoretically) possible to handle such edge cases?

  16. jnewbery commented at 8:05 PM on May 16, 2020: member

    is it better to disable timeout by setting a negative factor as @MarcoFalke suggested and amending the doc or people should just use a higher factor?

    My two cents: setting factor=0 should disable all timeouts.

    I also think the factor option and variable name should be changed everywhere. The name factor by itself is almost meaningless. Perhaps timeout_factor?

  17. rajarshimaitra commented at 5:49 AM on May 17, 2020: contributor

    My two cents: setting factor=0 should disable all timeouts.

    That with added doc should serve the purpose. But It also should handle other areas properly where factor value is being used. If everyone agrees, I can start implementing this.

    I also think the factor option and variable name should be changed everywhere. The name factor by itself is almost meaningless. Perhaps timeout_factor?

    Precisely my thought. The name doesn't disclose the intent. timout_factor would be more revealing even if a little long.

    Curious what others think.

  18. rajarshimaitra renamed this:
    Test: Adds a new --notimeout flag in test_framework.
    test: Adds a new --notimeout flag in test_framework.
    on May 17, 2020
  19. rajarshimaitra renamed this:
    test: Adds a new --notimeout flag in test_framework.
    tests: Adds a new --notimeout flag in test_framework.
    on May 17, 2020
  20. jonatack commented at 10:55 AM on May 17, 2020: member

    My two cents: setting factor=0 should disable all timeouts.

    I also think the factor option and variable name should be changed everywhere. The name factor by itself is almost meaningless. Perhaps timeout_factor?

    SGTM

  21. rajarshimaitra force-pushed on May 18, 2020
  22. rajarshimaitra commented at 4:43 AM on May 18, 2020: contributor

    Thanks everyone for all the inputs. I have modified the feature accordingly.

    • --factor is changed to --timeout-factor to better express intent and help doc updated. Internally all self.options.factor modified to self.options.timeout_factor.

    • setting --timeout-factor 0 will disable RPC timeout for functional tests. Fixed few typos as I was touching these files anyway.

    • /test/README.md updated to add a note on how to handle potential RPC timeout while attaching gdb. (even though small change, to adhere to the general guideline, its made into a separate commit)

    • Verified all existing tests are passing as it is.

    • Verified all existing tests are passing with --timeout-factor 0.

    I think this PR is now ready for a second round of review. Polite request to reviewers to take a second look.

  23. rajarshimaitra requested review from jonatack on May 18, 2020
  24. rajarshimaitra requested review from MarcoFalke on May 18, 2020
  25. rajarshimaitra renamed this:
    tests: Adds a new --notimeout flag in test_framework.
    tests: Add capability to disable RPC timeout in functional tests
    on May 18, 2020
  26. in test/functional/test_framework/test_framework.py:106 in 9eae0c4f13 outdated 
     101 | @@ -102,7 +102,10 @@ def __init__(self):
 102 |          self.bind_to_localhost_only = True
 103 |          self.set_test_params()
 104 |          self.parse_args()
 105 | -        self.rpc_timeout = int(self.rpc_timeout * self.options.factor) # optionally, increase timeout by a factor
 106 | +        if self.options.timeout_factor == 0 :
 107 | +            self.rpc_timeout = 99999
    MarcoFalke commented at 11:18 AM on May 18, 2020: 
                self.options.timeout_factor = 99999

    This should apply to all timeouts, otherwise the tests will crash in sync_* calls

    rajarshimaitra commented at 3:25 PM on May 18, 2020:

    Right, I missed that part. Fixed.

  27. in test/functional/test_framework/test_framework.py:175 in 9eae0c4f13 outdated 
     171 | @@ -169,7 +172,7 @@ def parse_args(self):
 172 |                              help="set a random seed for deterministically reproducing a previous test run")
 173 |          parser.add_argument("--descriptors", default=False, action="store_true",
 174 |                              help="Run test using a descriptor wallet")
 175 | -        parser.add_argument('--factor', type=float, default=1.0, help='adjust test timeouts by a factor')
 176 | +        parser.add_argument('--timeout-factor', dest="timeout_factor", type=float, default=1.0, help='adjust RPC timeout by a factor. Setting it to 0 disables the timeout')
    MarcoFalke commented at 11:18 AM on May 18, 2020: 
            parser.add_argument('--timeout-factor', dest="timeout_factor", type=float, default=1.0, help='adjust test timeouts by a factor. Setting it to 0 disables the timeout')

    The previous wording was correct. This is not only about rpc timeouts

    rajarshimaitra commented at 3:25 PM on May 18, 2020:

    Reverted.

  28. in test/functional/test_framework/util.py:212 in 9eae0c4f13 outdated 
     207 | @@ -208,10 +208,13 @@ 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, factor=1.0):
 212 | -    if attempts == float('inf') and timeout == float('inf'):
 213 | +def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0):
 214 | +    if timeout_factor == 0:
    MarcoFalke commented at 11:20 AM on May 18, 2020:

    Why is this needed? The condition should never be True.

    rajarshimaitra commented at 3:26 PM on May 18, 2020:

    This was required because I was setting the factor to 0. After the above changes it's not required anymore. Reverted.

  29. test: Add capability to disable RPC timeout in functional tests.
    Modifies the existing --factor flag to --timeout-factor to better express intent.
Adds rules to disable timeout if --timeout-factor is set to 0.
Modfies --timeout-factor help doc to inform users about this feature.
    784ae09625
  30. docs: Add notes on how to diasble rpc timeout in functional tests while attatching gdb. 38c3dd9c70
  31. rajarshimaitra force-pushed on May 18, 2020
  32. rajarshimaitra commented at 4:02 PM on May 18, 2020: contributor

    Addressed @MarcoFalke 's comments.

  33. MarcoFalke commented at 4:29 PM on May 18, 2020: member

    ACK 38c3dd9c706e7e84b2a4dbaf1424a3f1c3b694fc and thanks for fixing up all my typos :sweat_smile:

  34. in test/functional/test_framework/util.py:211 in 38c3dd9c70 
     207 | @@ -208,10 +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, factor=1.0):
 212 | +def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0):
    jnewbery commented at 4:58 PM on May 18, 2020:

    (for a follow-up PR) this is a really weird interface. The client is saying: 'take these two numbers, multiply them and use that as the timeout'. The client is able to do that multiplication itself! Why does it need to pass in two different values?

    rajarshimaitra commented at 5:38 PM on May 18, 2020:

    Thanks for the review. The process of making a PR was educational and made me go deeper into stuffs. Couldn't have done without your review club.

    And also thanks for the follow-up idea. Yes it does seem redundant. Surely this can be improved. I will look into this. :+1:

    rajarshimaitra commented at 6:39 PM on May 18, 2020:

    On an initial look it seems this is fairly trivial. There is no test function that calls wait_until() with timeout_factor. Either uses default or specifies a specific timeout. The only place timeout_factor is used is at node initialization and peer connection logic, which already knows how to handle it. So it seems it just requires changing the wait_until() signature. Let me know if I am missing something there.

  35. jnewbery commented at 4:59 PM on May 18, 2020: member

    ACK 38c3dd9c706e7e84b2a4dbaf1424a3f1c3b694fc.

    Thanks for doing this @rajarshimaitra. I've added a suggestion for a follow-up PR if you're interested.

  36. in test/README.md:229 in 38c3dd9c70 
     224 | @@ -225,6 +225,10 @@ gdb /home/example/bitcoind <pid>
 225 |  Note: gdb attach step may require ptrace_scope to be modified, or `sudo` preceding the `gdb`.
 226 |  See this link for considerations: https://www.kernel.org/doc/Documentation/security/Yama.txt
 227 |  
 228 | +Often while debugging rpc calls from functional tests, the test might reach timeout before
 229 | +process can return a response. Use `--timeout-factor 0` to disable all rpc timeouts for that partcular
    MarcoFalke commented at 12:02 AM on May 19, 2020: 
    process can return a response. Use `--timeout-factor 0` to disable all rpc timeouts for that particular
  37. MarcoFalke merged this on May 19, 2020
  38. MarcoFalke closed this on May 19, 2020
  39. MarcoFalke commented at 12:03 AM on May 19, 2020: member

    Looks like there is a typo? Mind to fix it up in your follow-up pull request?

  40. fanquake referenced this in commit 19612ca2eb on Jun 29, 2020
  41. Fabcien referenced this in commit f647670d9a on Feb 1, 2021
  42. DrahtBot locked this on Feb 15, 2022
Contributors
rajarshimaitraDrahtBotjonatackMarcoFalkejnewbery


jonatackryanofsky

Labels
Tests
Linked (view graph)
#19048 test: changing signature of wait_until().
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: 2026-04-16 21:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me