test: changing signature of wait

rajarshimaitra commented at 7:49 AM on May 22, 2020: contributor

As a follow up on #18986(comment), jnewbery suggested that wait_until() is unnecessarily taking timeout_factor as a parameter, whereas the client should have this knowledge and can directly provide wait_until() the required timeout.

As it turned out almost nowhere the client is passing timeout_factor to wait_until() and changing the signature is enough to enforce the intended behavior.

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

fanquake added the label Tests on May 22, 2020

fanquake requested review from jnewbery on May 22, 2020

fanquake commented at 7:51 AM on May 22, 2020: member

You have committed your vscode settings as part of af83312ac57e6dfe2c7202db81ef54b7efd31170.

in test/functional/test_framework/mininode.py:375 in af83312ac5 outdated

 371 | @@ -372,7 +372,7 @@ def on_version(self, message):
 372 |      # Connection helper methods
 373 |  
 374 |      def wait_until(self, test_function, timeout):
 375 | -        wait_until(test_function, timeout=timeout, lock=mininode_lock, timeout_factor=self.timeout_factor)
 376 | +        wait_until(test_function, timeout=timeout, lock=mininode_lock)

jonatack commented at 8:09 AM on May 22, 2020:

This isn't a typo; belongs in the other commit?

(Also, generally one typo isn't worth a separate commit. As you spend more time reviewing PRs, you'll see these practices.)

rajarshimaitra commented at 10:52 AM on May 22, 2020:

Right, it's all mixed up. Fixed now.

jnewbery commented at 4:42 PM on May 22, 2020:

This breaks the factor argument. You need to call wait_until() with timeout = timeout * self.timeout_factor.

rajarshimaitra commented at 6:01 PM on May 28, 2020:

Agreed, fixed.

jonatack commented at 8:13 AM on May 22, 2020: contributor

Could you update the PR description to link directly to #18986 (review), the comment where this change was suggested, rather than to the PR? This avoids asking your reviewers to search through the whole PR discussion to find the suggestion.

Also, take your time to review your changes before opening a PR -- don't rush ;)

jonatack commented at 8:19 AM on May 22, 2020: contributor

Also, you can see that a maintainer changed your PR description. It's best to not put GitHub @-prefixed usernames in commits and PR descriptions; the latter because usernames in the description are copied into the merge commit by the merge script. This can cause annoying notifications for those concerned. An update to the merge script now warns maintainers if a merge message contains a @username, but it's better not to add them to begin with.

rajarshimaitra force-pushed on May 22, 2020

rajarshimaitra commented at 10:53 AM on May 22, 2020: contributor

Sorry for all those silly mistakes. Fixed them and rebased. All changes are put into single commit for easier review.

in test/functional/test_framework/mininode.py:208 in 530b448985 outdated

 204 | @@ -205,7 +205,7 @@ def _on_data(self):
 205 |                  self._log_message("receive", t)
 206 |                  self.on_message(t)
 207 |          except Exception as e:
 208 | -            logger.exception('Error reading message:', repr(e))
 209 | +            logger.exception('Error reading message: %s' % (repr(e)))

jnewbery commented at 1:59 PM on May 22, 2020:

nit: prefer using the "{}".format() style

jnewbery commented at 4:40 PM on May 22, 2020:

Actually, why is this even being changed in this PR? It seems completely unrelated to the other changes.

rajarshimaitra commented at 5:07 PM on May 22, 2020:

Yes, this line was giving error. So I changed it as a nit.

laanwj commented at 10:51 AM on May 28, 2020:

The previous formulation was wrong, so it makes sense to change this. I also prefer {}.format() syntax though, and it's what the code base has been moving towards for a while.

rajarshimaitra commented at 6:00 PM on May 28, 2020:

Fixed.

rajarshimaitra commented at 2:21 PM on May 22, 2020: contributor

There is a Travis failure happening due to feature_backwards_compatibility.py. This test is skipping with the following warning (probably why I missed it).

$ test/functional/feature_backwards_compatibility.py
2020-05-22T14:15:55.381000Z TestFramework (WARNING): Test Skipped: previous releases not available or disabled
2020-05-22T14:15:55.431000Z TestFramework (INFO): Stopping nodes
2020-05-22T14:15:55.432000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_8xy_o2kz
on exit
2020-05-22T14:15:55.432000Z TestFramework (INFO): Test skipped

Any pointer on how to enable the previous release?

in test/functional/test_framework/test_node.py:361 in 530b448985 outdated

 357 | @@ -358,7 +358,7 @@ def is_node_stopped(self):
 358 |          return True
 359 |  
 360 |      def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT):
 361 | -        wait_until(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor)
 362 | +        wait_until(self.is_node_stopped, timeout=timeout)

jnewbery commented at 2:24 PM on May 22, 2020:

Do you need to multiply timeout by timeout_factor here?

rajarshimaitra commented at 3:56 PM on May 22, 2020:

Timeout here is set to BITCOIND_PROC_WAIT_TIMEOUT which is hardcode as 60 secs. This is the only time this value is being called. As it has been specifically separated from self.options.timeout, would it make sense to multiply it with timeout_factor here? Thinking that I skipped it.

rajarshimaitra commented at 6:11 PM on May 27, 2020:

Polite reminder. :) Does the above rationale make sense or should I change it?

jnewbery commented at 8:00 PM on May 27, 2020:

Timeout here is set to BITCOIND_PROC_WAIT_TIMEOUT which is hardcode as 60 secs.

That's the default value. wait_until_stopped() can be called with any timeout.

rajarshimaitra commented at 6:02 PM on May 28, 2020:

Right that was just a default. agreed and fixed.

DrahtBot added the label Needs rebase on May 26, 2020

test: change signature of wait_until()

wait_until() does not take timeout_factor as an arguement anymore.
Clients should pass in the correct timeout value at the time of calling.

a082679368

rajarshimaitra force-pushed on May 28, 2020

rajarshimaitra commented at 6:06 PM on May 28, 2020: contributor

Thank you all for the reviews. Updated as per the suggestions, rebased, and retested. This is ready for a second look.

DrahtBot removed the label Needs rebase on May 28, 2020

in test/functional/test_framework/util.py:211 in a082679368

 207 | @@ -208,10 +208,9 @@ 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, timeout_factor=1.0):
 212 | +def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None):

glozow commented at 5:22 PM on May 30, 2020:

Mininode has its own wait_until function which automatically grabs the mininode_lock (in addition to including the mininode's timeout_factor) so I think it’s probably better to use that function if synchronization is needed. Would it be worth adding a comment about this? I’ve seen multiple reviews with reminders to pass in the mininode_lock or use another function that does it automatically. I wonder if a comment would help remind people earlier on.

rajarshimaitra commented at 2:03 PM on May 31, 2020:

Thank @gzhao408 for the review and testing. While I agree with the cautionary message of using the mininode wait_until, I am not sure whether this is the correct place to add that comment. This seems like a generalized function for wait_until that can be used in many places and not all of them might require the lock. It seems that comment can be placed in the mininode's definition itself as a reminder to use mininode.wait_until whenever required as opposed to util.wait_until.

glozow commented at 7:19 PM on June 8, 2020:

Well, no, my suggestion for the comment is more of a "don't use this." I don't think util.wait_until should ever be used except to define object-specific wait_until since they should be responsible for including the timeout_factor (that's the point of this PR right?). In functional tests, I think it's more appropriate to use mininode, test_node, or test_framework wait_until. See #19134 - your PRs are very intertwined.

rajarshimaitra commented at 9:48 AM on June 9, 2020:

Hmm I see your point now, a comment here saying "Dont Use it" can help as per #19134 modifications. I am also now confused about why there are so many wait_untils to begin with. Anyway, I am not very sure if this PR is adding any value. timeout_factor is not being used anywhere with wait_until anyway. I think I will wait for more ACKs before further working on this.

glozow commented at 5:58 PM on May 30, 2020: member

Looks good to me, tests passed and I did a code review for the places using test_framework.util's wait_until function. I left a comment because I think other wait_until functions are more favorable due to synchronization needs as well as timeout_factor. I also have a conceptual question that hopefully doesn't sound offensive/dumb: I don't really see many examples of timeout_factor being used - what are the use cases for it?

rajarshimaitra commented at 1:47 PM on May 31, 2020: contributor

I also have a conceptual question that hopefully doesn't sound offensive/dumb: I don't really see many examples of timeout_factor being used - what are the use cases for it?

I also have the same question. Almost all of the places timeout has been explicitly passed into wait_until and it was using the default factor of 1. Thus the change became so small and didn't require modification in those caller functions.

One reason I can think of is to easily bump up all the timeouts at once. PR #18716 that added this flag which was motivated by this comment.

It seems it was added to solve an issue rather than as a functionality. Maybe someone else can shed more light. Curious as well.

DrahtBot commented at 3:48 AM on June 10, 2020: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19208 (test: move sync_blocks and sync_mempool functions to test_framework.py by ycshao)

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.

DrahtBot added the label Needs rebase on Jun 16, 2020

DrahtBot commented at 9:30 PM on June 16, 2020: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

fanquake commented at 6:29 AM on April 8, 2021: member

@MarcoFalke can you concept ack / nack here? @jnewbery can you comment re #19048 (comment)?

jnewbery commented at 7:56 AM on April 8, 2021: contributor

@jnewbery can you comment

I think this can just be closed. It's required rebase for several months.

maflcko added the label Up for grabs on Apr 8, 2021

maflcko closed this on Apr 8, 2021

bitcoin locked this on Aug 16, 2022

maflcko removed the label Up for grabs on Dec 12, 2023

test: changing signature of wait_until(). #19048

Conflicts