Skip to content

tests: Remove unused constant MAX_INV_SZ#13068

Closed
practicalswift wants to merge 1 commit intobitcoin:masterfrom
practicalswift:MAX_INV_SZ
Closed

tests: Remove unused constant MAX_INV_SZ#13068
practicalswift wants to merge 1 commit intobitcoin:masterfrom
practicalswift:MAX_INV_SZ

Conversation

@practicalswift
Copy link
Copy Markdown
Contributor

Remove unused constant MAX_INV_SZ.

Last use removed in 9c92c8c.

@fanquake fanquake added the Tests label Apr 24, 2018
@Empact
Copy link
Copy Markdown
Contributor

Empact commented Apr 25, 2018

utACK d66943d there are other references, but all in .cpp/.h

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Apr 25, 2018

Concept ACK - though I wonder if we shouldn't, instead, add a test that uses this, e.g. tests that this is the maximum number of INVs returned?

@practicalswift
Copy link
Copy Markdown
Contributor Author

practicalswift commented Apr 25, 2018

@laanwj Yes, more testing would be nice! This PR is "up for grabs" for someone who can take on that task :-)

@practicalswift
Copy link
Copy Markdown
Contributor Author

practicalswift commented Apr 26, 2018

@fanquake I was about to push a new version with a short TODO about the lack of testing covering MAX_INV_SZ, but I saw that this PR was closed. Keeping unused variables around feels sloppy – better comment it out and add a comment about the lack of test coverage.

--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -31,7 +31,9 @@ MY_VERSION = 70014  # past bip-31 for ping/pong
 MY_SUBVERSION = b"/python-mininode-tester:0.0.3/"
 MY_RELAY = 1 # from version 70001 onwards, fRelay should be appended to version messages (BIP37)

-MAX_INV_SZ = 50000
+# TODO: Add a test test that uses MAX_INV_SZ - e.g. tests that this is the maximum number of INVs returned
+# MAX_INV_SZ = 50000
+
 MAX_BLOCK_BASE_SIZE = 1000000

 COIN = 100000000  # 1 btc in satoshis

Makes sense?

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Apr 26, 2018

@practicalswift No strong opinion about that, I can't re-open the PR for some reason, I think you need to push back the old branch before that is possible.

@practicalswift
Copy link
Copy Markdown
Contributor Author

@laanwj Actually I've already force pushed to the old branch – see below :-)

$ git diff HEAD~1..HEAD
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index ca2e425..43f37d9 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -31,7 +31,9 @@ MY_VERSION = 70014  # past bip-31 for ping/pong
 MY_SUBVERSION = b"/python-mininode-tester:0.0.3/"
 MY_RELAY = 1 # from version 70001 onwards, fRelay should be appended to version messages (BIP37)

-MAX_INV_SZ = 50000
+# TODO: Add a test test that uses MAX_INV_SZ - e.g. tests that this is the maximum number of INVs returned
+# MAX_INV_SZ = 50000
+
 MAX_BLOCK_BASE_SIZE = 1000000

 COIN = 100000000  # 1 btc in satoshis
$ git push -f
Everything up-to-date

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants