Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creates unittests for addrman, makes addrman more testable. #6720

Merged
merged 1 commit into from Oct 7, 2015

Conversation

@eth0
Copy link

@eth0 eth0 commented Sep 24, 2015

  • Creates unittests for addrman, while not all addrman behavior is covered by these tests they provide a useful starting place and increase code coverage.
  • Minor modifications to addrman to allow deterministic and effective testing.
@laanwj laanwj added the Tests label Sep 24, 2015
@jonasschnelli
jonasschnelli reviewed Sep 24, 2015
View changes
src/addrman.h Outdated
@@ -567,6 +569,14 @@ class CAddrMan
Check();
}
}


#ifdef __ADDRMAN_TEST__

This comment has been minimized.

@jonasschnelli

jonasschnelli Sep 24, 2015
Member

Would it make sense to link this to the configure flag --enable-tests?

@eth0
Copy link
Author

@eth0 eth0 commented Sep 24, 2015

@jonasschnelli What advantages are offered by using the --enable-tests flag?

Also how would one learn the value of --enable-tests at the level necessary to change code behavior? I suppose you could move the MakeDeterministic method into its own source file and then only include that source when building tests but that is a bit complicated.

I could also remove the ifdef ADDRMAN_TEST check completely.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Sep 24, 2015

@eth0: maybe remote the #ifdef __ADDRMAN_TEST__ completely or export the ENABLE_TESTS over AC_DEFINE_UNQUOTED() in configure.ac and use this define to determine if you compile the MakeDeterministic() function?

Is the newOnly related to the new unit-tests?

Nice work!
Concept ACK.

Adds several unittests for addrman to verify it works as expected.
Makes small modifications to addrman to allow deterministic and targeted tests.
@eth0
Copy link
Author

@eth0 eth0 commented Sep 24, 2015

@jonasschnelli
I removed the macro completely.

newOnly is related to the unit-tests since I need a way to test if an addr is in the new or tried tables.

I am developing some non-unittest code which builds on the newOnly functionality but it is independent of this pull-request.

@dcousens
Copy link
Contributor

@dcousens dcousens commented Sep 25, 2015

concept ACK, non test-code utACK

@EthanHeilman
Copy link
Contributor

@EthanHeilman EthanHeilman commented Sep 30, 2015

@dcousens Anything I can do to move this along?
I've tested this code in the Bitcoin client and the unittests all pass.

@jgarzik
Copy link
Contributor

@jgarzik jgarzik commented Oct 1, 2015

ut ACK - doesn't test very much, but what it does test, looks correct based on quick review :)

@laanwj laanwj merged commit 1534d9a into bitcoin:master Oct 7, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Oct 7, 2015
1534d9a Creates unittests for addrman, makes addrman testable. Adds several unittests for addrman to verify it works as expected. Makes small modifications to addrman to allow deterministic and targeted tests. (EthanHeilman)
@laanwj
Copy link
Member

@laanwj laanwj commented Oct 7, 2015

utACK

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jan 9, 2016
Adds several unittests for addrman to verify it works as expected.
Makes small modifications to addrman to allow deterministic and targeted tests.

Github-Pull: bitcoin#6720
Rebased-From: 1534d9a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.