[tests] Clean up addrman_tests.cpp #10371

pull jimmysong wants to merge 1 commits into bitcoin:master from jimmysong:update_addressman_test changing 1 files +87 −90

jimmysong commented at 4:13 PM on May 9, 2017: contributor

Cleanup request from #10287. Change "Test #:" comments to "Test:" Change BOOST_CHECK(... = ...) to BOOST_CHECK_EQUAL(..., ...)

Requested per @EthanHeilman
in src/test/addrman_tests.cpp:398 in 4460a06a12 outdated
```
 395 |      addrman.Add(addr4, source2);
 396 |      addrman.Add(addr5, source1);
 397 |  
 398 |      // GetAddr returns 23% of addresses, 23% of 5 is 1 rounded down.
 399 | -    BOOST_CHECK(addrman.GetAddr().size() == 1); 
 400 | +    BOOST_CHECK_EQUAL(addrman.GetAddr().size(), 1); 
```
MarcoFalke commented at 4:36 PM on May 9, 2017:

Please no trailing whitespace. This will cause issues in version control when used with editors that remove trailing whitespace by default.

jimmysong commented at 4:42 PM on May 9, 2017:

sorry, not sure how that got there. fixing now.

MarcoFalke commented at 5:10 PM on May 9, 2017:

It has been there before, though no need to drag along. :)
jimmysong force-pushed on May 9, 2017
MarcoFalke added the label Refactoring on May 9, 2017
MarcoFalke added the label Tests on May 9, 2017
in src/test/addrman_tests.cpp:302 in b81103aad0 outdated
```
 295 | @@ -296,23 +296,23 @@ BOOST_AUTO_TEST_CASE(addrman_find)
 296 |      addrman.Add(addr2, source2);
 297 |      addrman.Add(addr3, source1);
 298 |  
 299 | -    // Test 17: ensure Find returns an IP matching what we searched on.
 300 | +    // Test: ensure Find returns an IP matching what we searched on.
 301 |      CAddrInfo* info1 = addrman.Find(addr1);
 302 |      BOOST_CHECK(info1);
 303 |      if (info1)
```
jnewbery commented at 7:04 PM on May 9, 2017:

I was going to suggest you update this if statement to follow our style guidelines (continuation on the same line if the block is one line, or braces), but really I don't think an if statement has any business being here at all. This is a test case - we should be asserting all our expectations (and indeed we do in the line above).

Suggest you remove the if line and run BOOST_CHECK(info1->ToString() == "250.1.2.1:8333") directly. Same for other 2 ifs below.

jimmysong commented at 7:19 PM on May 9, 2017:

Thanks for the review! Changed.

MarcoFalke commented at 7:23 PM on May 9, 2017:

This was added as a guard against null pointer dereference

jimmysong commented at 7:37 PM on May 9, 2017:

That makes sense. Still trying to figure this out. @jnewbery, what's the coding standard that I should change the if to?

jnewbery commented at 7:48 PM on May 9, 2017:

Doesn't BOOST_CHECK(info1) check that info1 is non-null?

And if not, surely it's better to fail the test than silently continue?

MarcoFalke commented at 8:01 PM on May 9, 2017:

According to http://www.boost.org/doc/libs/1_64_0/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_level.html you could use BOOST_REQUIRE()

jimmysong commented at 9:01 PM on May 9, 2017:

attempt 3. Please let me know if this is satisfies everything @jnewbery @MarcoFalke
jnewbery commented at 7:04 PM on May 9, 2017: member

Looks good to me. Tested ACK b81103aad0189075e89de5626679ed57f563846d with one nit.
jimmysong force-pushed on May 9, 2017

[tests] Clean up addrman_tests.cpp

Cleanup request from #10287.
Change "Test #:" comments to "Test:"
Change BOOST_CHECK(... = ...) to BOOST_CHECK_EQUAL(..., ...)
Remove three unnecessary if statements

a80f295666

jimmysong force-pushed on May 9, 2017
jnewbery commented at 9:12 PM on May 9, 2017: member

looks great. Tested ACK a80f295666ba15d795be00773f768d7167599a63
MarcoFalke commented at 9:23 PM on May 9, 2017: member

utACK a80f295666ba15d795be00773f768d7167599a63
MarcoFalke merged this on May 9, 2017
MarcoFalke closed this on May 9, 2017
MarcoFalke referenced this in commit 4b766fcdd4 on May 9, 2017
PastaPastaPasta referenced this in commit d7232a68e1 on Jun 10, 2019
PastaPastaPasta referenced this in commit dbaf812ccb on Jun 10, 2019
PastaPastaPasta referenced this in commit 2349873bf1 on Jun 10, 2019
PastaPastaPasta referenced this in commit 513ef9862c on Jun 11, 2019
PastaPastaPasta referenced this in commit a5d603c0dc on Jun 11, 2019
PastaPastaPasta referenced this in commit 77649e769d on Jun 15, 2019
PastaPastaPasta referenced this in commit 420a2a78e4 on Jun 19, 2019
PastaPastaPasta referenced this in commit cefe6e3243 on Jun 19, 2019
PastaPastaPasta referenced this in commit 3eb2b5a2c1 on Jun 19, 2019
PastaPastaPasta referenced this in commit 3ce6f014db on Jun 19, 2019
PastaPastaPasta referenced this in commit 173ed85a3d on Jun 19, 2019
PastaPastaPasta referenced this in commit cbb7a8ecc0 on Jun 19, 2019
PastaPastaPasta referenced this in commit 89fcdc4c04 on Jun 19, 2019
PastaPastaPasta referenced this in commit 7df4885ca9 on Jun 20, 2019
barrystyle referenced this in commit 850f738bab on Jan 22, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10287 [tests] Update Unit Test for addrman.h/addrman.cpp