Add tests for (or remove) untested unused methods #15814

practicalswift commented at 1:35 PM on April 14, 2019: contributor

Changes:

Add tests for untested unused methods (where YAGNI does not hold).
Remove untested unused methods (where YAGNI holds).

Rationale:

Quoting the words of wisdom from Beyoncé's hit song "Unused Method Put a Test on It" from the 2008 album "I Am... A Unit of Test":

Cause if you liked it, then you should have put a test on it If you liked it, then you should have put a test on it Don't be mad once you see that they remove it If you liked it, then you should have put a test on it Oh, oh, oh Oh, oh, oh, oh, oh, oh Oh, oh, oh Oh, oh, oh Oh, oh, oh, oh, oh, oh Oh, oh, oh Cause if you liked it, then you should have put a test on it If you liked it, then you should have put a test on it Don't be mad once you see that they remove it If you liked it, then you should have put a test on it

Notes to reviewers:

I believe this change to be exhaustive -- no unused methods should be left to discover in the code base (excluding dependencies) after the merge of this PR. Any counter-examples would be very welcome so that I can revisit my methodology!
Let me know if there are any open PR:s making use of any of these methods, or if there is any compelling evidence that the YAGNI principle does not hold for any of those methods :-)

fanquake added the label Refactoring on Apr 14, 2019

in src/psbt.cpp:19 in 43b4206534 outdated

  15 | @@ -16,11 +16,6 @@ PartiallySignedTransaction::PartiallySignedTransaction(const CMutableTransaction
  16 |      outputs.resize(tx.vout.size());
  17 |  }
  18 |  
  19 | -bool PartiallySignedTransaction::IsNull() const

fanquake commented at 2:19 AM on April 15, 2019:

I'm pretty sure all of the PSBT related isNull() methods are used. cc @gwillen.

gwillen commented at 2:40 AM on April 15, 2019:

I would believe that they're not. The refactor of PSBT methods went through several iterations and there was definitely dead code at least at one point.

Also, the fact that I can build and run tests with no complaint with the methods removed, suggests they are indeed unused.

More comments below, though.

practicalswift commented at 9:29 AM on April 15, 2019:

PartiallySignedTransaction::IsNull(), PSBTInput::IsNull() and PSBTOutput::IsNull() were all globally unreferenced, unused and untested prior to this PR. In the updated PR they are tested and thus used from the tests at least :-)

gwillen commented at 2:51 AM on April 15, 2019: contributor

I'm not sure how I feel about this PR.

I like the idea of removing code that is untested and unreferenced, since it at risk of bitrotting, or being buggy and unnoticed.

That said, I would say that PartiallySignedTransaction::IsNull and CBlock::ToString are both (1) essentially trivial methods with a negligible possibility of error, and (2) seem like they are fulfilling some part of the interface contract of their respective classes.

For PST::IsNull, my sense is that by convention we often use value objects as "nullable", rather than wrap them in an Option type, if their contents are effectively all nullable. Having some sort of method to test for nullity is part of the contract required to use them in that way. (I feel more strongly about PST than PBSTInput/Output, because the latter are less likely to be useful to code external to the module, and so anybody doing fancy things with them is more likely to feel licensed to modify them to suit.)

For CBlock::ToString, it appears to be mainly intended for debugging, and I expect it's the case that many objects have ToString methods intended to fulfill this same sort of "can be printed for debugging" contract. I expect this method has been called with useful results in debug printfs that were never checked in, and might be again in the future. (I'm not generally working with CBlock objects, so I'm speculating, but if other nearby objects tend to have ToString methods that are called that would strengthen my case.)

The BlockFilter constructor appears to be one among many, and I'm not close enough to that code to have an opinion on whether that specific variant is useful past the removal of whatever was calling it.

The comments on CBloomFilter suggest that the tweak is only useful for testing, so the reset method is unlikely to be useful outside the module's own tests, which I think makes it a good candidate for removal (again under the reasoning that anybody who's working with it is probably editing the module itself, and is likely to feel licensed to add it back if they need it.)

promag commented at 7:19 AM on April 15, 2019: member

Add trivial test for bitcoinconsensus_version() to make it both technically in use and technically tested :-)

You could separate this to a different and non controversial pull.

How did you find the remaining changes?

practicalswift commented at 7:25 AM on April 15, 2019: contributor

@gwillen Thanks for reviewing! For the cases where the consensus is that YAGNI does not hold I'll simply add tests (like bitcoinconsensus_version()).

practicalswift renamed this:
~~Remove all untested unused methods. Add test for bitcoinconsensus_version() to make it technically in use.~~
Add tests for or remove all untested unused methods
on Apr 15, 2019

practicalswift force-pushed on Apr 15, 2019

gwillen commented at 7:50 AM on April 15, 2019: contributor

@practicalswift If you're writing the tests, I 100% cannot complain about this solution! :-)

in src/test/script_tests.cpp:183 in 6f7ceb6736 outdated

 179 | @@ -180,6 +180,10 @@ void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScript
 180 |      }
 181 |  
 182 |  #if defined(HAVE_CONSENSUS_LIB)
 183 | +    // Make sure bitcoinconsensus_version() is referenced at least once from

merland commented at 8:24 AM on April 15, 2019:

The comment does not convey the "root why", which I guess is "to avoid the risk of it becoming flagged as unused" or something along those lines. (Maybe overkill for most people, but why not be as clear as possible... )

practicalswift commented at 9:21 AM on April 15, 2019:

Thanks for reviewing! Fixed!

practicalswift force-pushed on Apr 15, 2019

practicalswift commented at 9:31 AM on April 15, 2019: contributor

@gwillen The updated version adds some basic tests for the unused methods instead of removing. Now they are at least used from the tests :-) Please review!

Still removing the unused BlockFilter ctor and CBloomFilter::reset since YAGNI likely holds for them. Looks OK? :-)

practicalswift renamed this:
~~Add tests for or remove all untested unused methods~~
Add tests for (or remove) all untested unused methods
on Apr 15, 2019

laanwj commented at 11:47 AM on April 15, 2019: member

That said, I would say that PartiallySignedTransaction::IsNull and CBlock::ToString are both (1) essentially trivial methods with a negligible possibility of error, and (2) seem like they are fulfilling some part of the interface contract of their respective classes.

Yes. I've argued this before, and I felt exactly the same when seeing this PR title. Sometimes there's an unwritten assumption about what methods to expect on a certain API, and this is broken by ripping them out—just because they happen to be unused and untested at the moment. The better thing to do would be to add tests.

(removing things can be argued on a case-by-case basis, obviously, but don't blanket-do this)

Edit: okay, this PR is not as bad as the title sounds. Still, what I'm personally much more interested in would be "used, but untested" methods, because it means they do affect outcomes and need tests.

practicalswift commented at 12:04 PM on April 15, 2019: contributor

@laanwj Yes, adding tests is the right thing to do in the cases you describe. The resulting removals look OK?

laanwj commented at 12:53 PM on April 15, 2019: member

These are only:

BlockFilter::BlockFilter(BlockFilterType filter_type, const uint256& block_hash, std::vector<unsigned char> filter)
CBloomFilter::reset(const unsigned int nNewTweak)

As for the first one, this was recently added in #14172 by @jimpo. It looks like it was added as a lead-up to #14121 which is open and still unmerged. I would (unless Jim comments otherwise) advise against removing it.

The second one has been unused since 2015 or so (#7113) so removing it sounds fine to me.

Remove untested unused methods 30c7944dcd

Add tests for untested unused methods 9f49abff6a

practicalswift force-pushed on Apr 15, 2019

practicalswift commented at 2:51 PM on April 15, 2019: contributor

@laanwj Thanks for reviewing the patch! Feedback now addressed. Please re-review :-)

gwillen commented at 11:01 PM on April 15, 2019: contributor

Thanks, I like that much better! utACK as to the addition of the PSBT tests; I don't feel comfortable speaking to anything outside that.

practicalswift renamed this:
~~Add tests for (or remove) all untested unused methods~~
Add tests for (or remove) untested unused methods
on May 7, 2019

practicalswift commented at 9:20 PM on June 11, 2019: contributor

@MarcoFalke Can we move forward with this one? The updated version adding basic unit tests should hopefully be non-controversial :-)

in src/wallet/test/psbt_wallet_tests.cpp:64 in 9f49abff6a

  60 | +    // Call FillPSBT and perform some very basic IsNull sanity checks
  61 |      PartiallySignedTransaction psbtx;
  62 | +    BOOST_CHECK(psbtx.IsNull());
  63 |      CDataStream ssData(ParseHex("70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f000000000000000000"), SER_NETWORK, PROTOCOL_VERSION);
  64 |      ssData >> psbtx;
  65 | +    BOOST_CHECK(!psbtx.IsNull());

fanquake commented at 8:38 AM on June 14, 2019:

@achow101 did you want to take a look here?

practicalswift commented at 2:07 PM on June 29, 2019: contributor

Closing this "ham-handed" PR :-)

Context (from date of PR submission):

[wumpus] lool another day another ham-handed pracicalswift PR

practicalswift closed this on Jun 29, 2019

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 16, 2022