test: remove unused variables in `p2p_invalid_block` #28412

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2023-08-test-remove-unused-p2p-invalid-block changing 1 files +0 −2

brunoerg commented at 12:12 PM on September 5, 2023: contributor

No description provided.
test: remove unused variables in `p2p_invalid_block` 3eb03803c4
DrahtBot commented at 12:12 PM on September 5, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Sep 5, 2023
maflcko commented at 12:32 PM on September 5, 2023: member

Shouldn't vulture be able to detect those with our setting of 100%?
fanquake commented at 12:43 PM on September 5, 2023: member

Shouldn't vulture be able to detect those with our setting of 100%?

Yea, this seems to keep coming up. Looks like dropping the confidence level to 60% would at least produce some output (none of the code changed in this PR).
maflcko commented at 12:46 PM on September 5, 2023: member

IIRC dropping the confidence would lead to false positives, which we want to avoid.
fanquake commented at 12:48 PM on September 5, 2023: member

IIRC dropping the confidence would lead to false positives

Yea, most of the output produced at 60 is that (or code we wouldn't want to remove). I guess having Vulture at 100 when it doesn't seem to work, means we could just remove it?

Type	Reviewers
ACK	stickies-v

brunoerg commented at 1:12 PM on September 5, 2023: contributor

Shouldn't vulture be able to detect those with our setting of 100%?

It should, but it doesn't work as expected. It doesn't detect scenarios like:

abc = 1
abc = 2
call(abc)

Even dropping confidence, it catches only some stuff from our test framework (false flag). E.g.:

test/functional/p2p_invalid_block.py:32: unused method 'set_test_params' (60% confidence)
test/functional/p2p_invalid_block.py:33: unused attribute 'num_nodes' (60% confidence)
test/functional/p2p_invalid_block.py:34: unused attribute 'setup_clean_chain' (60% confidence)
test/functional/p2p_invalid_block.py:35: unused attribute 'extra_args' (60% confidence)
test/functional/p2p_invalid_block.py:37: unused method 'run_test' (60% confidence)

maflcko commented at 1:28 PM on September 5, 2023: member

Maybe report it upstream?
brunoerg commented at 2:25 PM on September 5, 2023: contributor

Maybe report it upstream?

Seems good. https://github.com/jendrikseipp/vulture/issues/315 is useful for us as well.
stickies-v approved
stickies-v commented at 2:37 PM on September 7, 2023: contributor

ACK 3eb03803c4111d09c63550a6a6c09afbe3430bf6

I was confused by the PR and commit title though as we're not removing any variables, and they are used. I'd just call it "unused code".
fanquake merged this on Sep 7, 2023
fanquake closed this on Sep 7, 2023
Frank-GER referenced this in commit 9c1c27b7e6 on Sep 8, 2023
bitcoin locked this on Sep 6, 2024

Contributors

Labels

test: remove unused variables in `p2p_invalid_block` #28412

Reviews