Tests: Contract testing for the procedure AddTimeData #14881

mmachicao commented at 3:55 AM on December 6, 2018: contributor

Objective: Make the contract for AddTimeData testable.

The procedure AddTimeData is stateful, so any testing beyond trivialities requires some modification in the code:

Separation between orchestration (lock/unlock) and algorithm
Pushing static (stateful) variables out of the procedure body

Unit testing of procedure AddTimeData 088985125f

Test AddTimeDataAlgorithm b34dd93ff1

Unit testing of procedure AddTimeData

Pushed static variables ouf of the procedure to remove statefulness
Introduced procedure AddTimeDataAlgorithm to test the contract

7a8498033e

Merge remote-tracking branch 'origin/timedata_contract_test' into timedata_contract_test c4cd6dabde

fanquake added the label Refactoring on Dec 6, 2018

fanquake added the label Tests on Dec 6, 2018

function inet_pton requires inet.h on windows 305ebd25db

in src/timedata.h:81 in c4cd6dabde outdated

  75 | @@ -75,4 +76,8 @@ int64_t GetTimeOffset();
  76 |  int64_t GetAdjustedTime();
  77 |  void AddTimeData(const CNetAddr& ip, int64_t nTime);
  78 |  
  79 | +/** Functions to enable testing the contract for AddTimeData */
  80 | +int CountOffsetSamples();
  81 | +void AddTimeDataAlgorithm(const CNetAddr& ip, const int64_t nOffsetSample, std::set<CNetAddr>& knownSet, CMedianFilter<int64_t>& offsetFilter, int64_t& offset);

Empact commented at 8:56 AM on December 6, 2018:

Could move these declarations to the test file, so that production code does not have access to the declarations.

Would benefit from EXCLUSIVE_LOCKS_REQUIRED on AddTimeDataAlgorithm - which if I'm not mistaken could be applied to the body of the method rather than the declaration.

mmachicao commented at 8:28 PM on December 6, 2018:

Hi Empact.

Thank you for the feedback, but I prefer to keep modifications to a minimum. Apart from this, I am a newbie with this code and not really proficient with C/C++, which is actually why I am focusing on unit testing in the first place. Functions added are either read only or do not modify the runtime data. That is why I did this in the first place.

First priority here is to dig out the contract and make it testable the way it is now. I consider this as a prerequisite for any subsequent changes.

If you have any hints, then please be more explicit, as I partly understand your approach, but do not really know how to implement it in a safe manner.

in src/test/timedata_tests.cpp:6 in 305ebd25db outdated

   2 | @@ -3,6 +3,7 @@
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  //
   5 |  #include <netaddress.h>
   6 | +#include <arpa/inet.h>

ken2812221 commented at 1:59 PM on December 7, 2018:

You could include compat.h instead.

mmachicao commented at 12:22 AM on December 11, 2018:

Hi. Appveyor still fails with the same message. Any other suggestion?

mmachicao commented at 1:05 AM on December 11, 2018:

Hi. A quick update.

compat.h includes the appropriate include files for windows. Unfortunately inet_pton has a slightly differen signature in win32. Any hints how to handle this without having a windows machine at hand?. Any suggestions are welcome, since this keeps me from completing the contract tests.

Thx.

ken2812221 commented at 12:22 PM on December 11, 2018:

You can rebase your PR on #14922. That PR can fix the problem.

The minimum required Windows version for inet_pton is Vista, but _WIN32_WINNT was set to 0x0501 (Windows XP)

in src/test/timedata_tests.cpp:224 in 305ebd25db outdated

 219 | +    BOOST_CHECK_EQUAL(offset, pre);
 220 | +    BOOST_CHECK_EQUAL(offsetFilter.size(), size);
 221 | +}
 222 | +
 223 | +
 224 | +CNetAddr utilBuildAddress(std::string address)

practicalswift commented at 8:19 PM on December 7, 2018:

address should be const ref?

in src/test/timedata_tests.cpp:233 in 305ebd25db outdated

 228 | +    CNetAddr addr = CNetAddr(sa.sin_addr);
 229 | +    return addr;
 230 | +}
 231 | +
 232 | +
 233 | +void utilPreconditionIsAtLeastFiveEntriesRequired(std::string baseip, int basevalue)

practicalswift commented at 8:21 PM on December 7, 2018:

Same here: baseip should be const ref?

in src/test/timedata_tests.cpp:240 in 305ebd25db outdated

 235 | +    for (int i = CountOffsetSamples(); i < 5; i++) { // precondition 1: at least 5 entries required to compute any offset
 236 | +        int val = basevalue + i;
 237 | +        std::stringstream stream;
 238 | +        stream << baseip << val;
 239 | +        std::string ip = stream.str();
 240 | +        AddTimeData(utilBuildAddress(ip.c_str()), val);

practicalswift commented at 8:21 PM on December 7, 2018:

The call to c_str is not needed here. The function takes std::string.

in src/test/timedata_tests.cpp:55 in 305ebd25db outdated

  50 | +    BOOST_CHECK_EQUAL(filter.sorted()[0], 10);
  51 | +    BOOST_CHECK_EQUAL(filter.sorted()[1], 100);
  52 | +}
  53 | +
  54 | +
  55 | +CNetAddr utilBuildAddress(std::string address);

practicalswift commented at 8:24 PM on December 7, 2018:

Could be dropped if utilBuildAddress was defined up here instead?

practicalswift commented at 8:25 PM on December 7, 2018:

See style guide for the function naming guidelines. Apply to all functions in this PR.

mmachicao commented at 11:19 PM on December 10, 2018:

Thank you for the feeback. All suggestions are done.

in src/test/timedata_tests.cpp:57 in 305ebd25db outdated

  52 | +}
  53 | +
  54 | +
  55 | +CNetAddr utilBuildAddress(std::string address);
  56 | +
  57 | +void utilPreconditionIsAtLeastFiveEntriesRequired(std::string baseip, int basevalue);

practicalswift commented at 8:24 PM on December 7, 2018:

Same here. Move definition of utilPreconditionIsAtLeastFiveEntriesRequired here instead.

DrahtBot commented at 3:57 AM on December 10, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14336 (net: implement poll by pstratem)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

enforced code conventions and added compat.h since arpa/inet.h not available on windows 8078dad339

compat.already included in netaddress.h shall support windows vista 0c56993ffc

Merge branch 'master' into timedata_contract_test 27cd95e936

Tests for ComputeTimeSmart falsely assumed that time offset is zero 89dd88b964

Initialize sockaddr_in with zero before use. Use sizeof in comparison operators for CNetAddr bf41e77dbb

util_AddTimeDataIgnoreSampleWithDuplicateIP is using ip out of range b6f8f5ae7b

in src/test/timedata_tests.cpp:96 in b6f8f5ae7b outdated

  91 | +    AddTimeData(UtilBuildAddress("1.1.1.211"), 211);
  92 | +
  93 | +    BOOST_CHECK_EQUAL(CountOffsetSamples(), samples + 1); // sample was added
  94 | +    BOOST_CHECK(GetTimeOffset() != offset);               // and new offset was computed
  95 | +}
  96 | +

markaw67 commented at 4:10 AM on December 26, 2018:

Is this the correct address?

fanquake deleted a comment on Dec 26, 2018

Using memcpy to build sockaddr_in because

 compat.h does not support inet_pton with current #define _WIN32_WINNT 0x0501 and
 #define _WIN32_WINNT 0x0600 has other integration issues

51a65850eb

applied clang formatter 09d9b92274

Moved statefull tests for AddTimeData to stateless AddTimeDataAlgorithm 176215f354

improve test descriptions 407e255646

mmachicao commented at 5:43 PM on December 28, 2018: contributor

This has become illegible.

Am replacing with: #15052

mmachicao closed this on Dec 28, 2018

MarcoFalke commented at 6:10 PM on December 28, 2018: member

Instead of closing/reopening pull request you can force push (and squash) your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

MarcoFalke locked this on Sep 8, 2021