Tests: Add logging in loops in p2p_sendhears.py #12849

pull ccdle12 wants to merge 1 commits into bitcoin:master from ccdle12:tests-adding-logging-in-p2p_sendheaders changing 1 files +3 −6

ccdle12 commented at 1:36 PM on March 31, 2018: contributor
PR for #12453

New contributor looking at mainly the 'good first issues'.

From my understanding of the issue:
- Track the iteration level of loop when test fails in Travis CI
Further clarification on the issue would be greatly appreciated to narrow down what can be fixed or updated.
fanquake added the label Tests on Mar 31, 2018

in test/functional/p2p_sendheaders.py:384 in 9f08dcb84a outdated

 379 | @@ -380,6 +380,8 @@ def test_nonnull_locators(self, test_node, inv_node):
 380 |          # PART 3.  Headers announcements can stop after large reorg, and resume after
 381 |          # getheaders or inv from peer.
 382 |          for j in range(2):
 383 | +            self.log.info("Part 3: iteration level: {}".format(j))
 384 | +

MarcoFalke commented at 2:37 PM on March 31, 2018:

I'd slightly prefer if the comments in the loops down there were transformed into debug statements. (Where we check for i == ... and j == ...)

in test/functional/p2p_sendheaders.py:409 in 059292b65d outdated

 407 | @@ -406,6 +408,8 @@ def test_nonnull_locators(self, test_node, inv_node):
 408 |              test_node.wait_for_block(new_block_hashes[-1])
 409 |  
 410 |              for i in range(3):
 411 | +                self.log.debug("Part 3: i == {}".format(i))
 412 | +
 413 |                  # Mine another block, still should get only an inv

MarcoFalke commented at 4:11 PM on March 31, 2018:

What I meant was to transfer those comments from # ... to self.log.debug(...

ccdle12 commented at 4:42 PM on March 31, 2018:

Ahh sorry, I think I get what you mean now

in test/functional/p2p_sendheaders.py:384 in f051bc8fb2 outdated

 380 | @@ -381,7 +381,7 @@ def test_nonnull_locators(self, test_node, inv_node):
 381 |          # getheaders or inv from peer.
 382 |          for j in range(2):
 383 |              # First try mining a reorg that can propagate with header announcement
 384 | -            new_block_hashes = self.mine_reorg(length=7)
 385 | +            new_block_hashes = self.mine_reorg(length=7)

jnewbery commented at 8:50 PM on April 2, 2018:

nit: you've added trailing whitespace here.

in test/functional/p2p_sendheaders.py:414 in f051bc8fb2 outdated
```
 411 | @@ -412,11 +412,13 @@ def test_nonnull_locators(self, test_node, inv_node):
 412 |                  test_node.check_last_announcement(inv=[tip], headers=[])
 413 |                  if i == 0:
 414 |                      # Just get the data -- shouldn't cause headers announcements to resume
```
jnewbery commented at 8:51 PM on April 2, 2018:

You can remove this comment now that you have a log saying the same thing!

Same for comments lower down.

ccdle12 commented at 10:16 PM on April 2, 2018:

Thanks @jnewbery, I appreciate the patience for a newbie, have pushed a new commit removing the comments above the debug prints.
jnewbery commented at 8:52 PM on April 2, 2018: member

Thanks for your first contribution @ccdle12!

I'm not sure if you've dropped a commit, but this PR isn't doing what the comments suggest.

A couple of nits inline.

in src/test/.vscode/settings.json:5 in 9bd3e1fedd outdated

   0 | @@ -0,0 +1,5 @@
   1 | +{
   2 | +    "files.associations": {
   3 | +        "*.ipp": "cpp"
   4 | +    }
   5 | +}

MarcoFalke commented at 10:16 PM on April 2, 2018:

Unwanted file?

in test/functional/p2p_sendheaders.py:414 in 9bd3e1fedd outdated

 410 | @@ -411,21 +411,18 @@ def test_nonnull_locators(self, test_node, inv_node):
 411 |                  inv_node.check_last_announcement(inv=[tip], headers=[])
 412 |                  test_node.check_last_announcement(inv=[tip], headers=[])
 413 |                  if i == 0:
 414 | -                    # Just get the data -- shouldn't cause headers announcements to resume

MarcoFalke commented at 10:16 PM on April 2, 2018:

Please remove this as well.

MarcoFalke commented at 10:16 PM on April 2, 2018: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Adding logging for loop iteration level in p2p_sendheaders.py

Changing logs to debug level and format of statment to j == ...

Updating debug statements to include comments when checking for iteration level of i and j

Removing comments above log prints in Part 3

Removing unwanted file

Added log prints in loops in p2p_sendheaders

8dd547d82b

ccdle12 force-pushed on Apr 2, 2018
jnewbery commented at 4:09 PM on April 3, 2018: member

As it stands, this doesn't seem to be that useful. It'll print out the same log many times over as it iterates over i and j.
MarcoFalke commented at 5:01 PM on April 3, 2018: member

utACK 8dd547d82b70678c19bee110950d962d4e62ed5a. Seems like a tiny improvement over what it was before.
MarcoFalke merged this on Apr 7, 2018
MarcoFalke closed this on Apr 7, 2018
MarcoFalke referenced this in commit 83c7533eb1 on Apr 7, 2018
PastaPastaPasta referenced this in commit 1d13370973 on Dec 16, 2020
PastaPastaPasta referenced this in commit f1aabcf489 on Dec 18, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12453 Intermittent failure in p2p_sendheaders.py