/bitcoin

Tests: address placement should be deterministic by default #10765

Open
wants to merge 1 commit into
from
+6 −34

Conversation

Reviewers

@jonasschnelli jonasschnelli

@promag promag

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
6 participants
@ReneNyffenegger @jonasschnelli @TheBlueMatt @MarcoFalke @promag @fanquake
@ReneNyffenegger

ReneNyffenegger commented Jul 7, 2017

Better version of wrong and closed pull request #10764

fanquake added the Tests label Jul 7, 2017

src/test/addrman_tests.cpp
@@ -15,9 +15,13 @@ class CAddrManTest : public CAddrMan
uint64_t state;
public:
- CAddrManTest()
+ CAddrManTest(bool makeDeterministic=true)
@promag

promag Jul 10, 2017

Contributor

Well there is no need for constructor argument, just call MakeDeterministic().

@ReneNyffenegger

ReneNyffenegger Jul 10, 2017

I've added the parameter bool makeDeterministic=true in case a future test explicitly doesn't want to call MakeDeterministic().

src/test/addrman_tests.cpp
{
state = 1;
+
+ if (makeDeterministic)
@jonasschnelli

jonasschnelli Jul 13, 2017

Member

brackets

@jonasschnelli
Member

jonasschnelli commented Jul 13, 2017

@ReneNyffenegger:
Maybe do git reset --hard origin/masterfollowed by a git cherry-pick 717dfaa4b507232b7169593b80c1a5b3b3ddf553 and then push with --force.
@ReneNyffenegger

ReneNyffenegger commented Jul 13, 2017

I really have no idea what's going on with this pull request. I've added the brackets for the if statement - now this pull request says that I want to merge 62 commits ... This is if course not true.

@ReneNyffenegger ReneNyffenegger added a commit to ReneNyffenegger/bitcoin that referenced this pull request Jul 13, 2017

@ReneNyffenegger ReneNyffenegger As per 
  bitcoin#10765 (comment)

  git reset --hard origin/master
  git cherry-pick 717dfaa

Then manually redo the wanted changes.
 b47c851
@ReneNyffenegger

ReneNyffenegger commented Jul 13, 2017

@jonasschnelli I've done the git reset --hard..., git cherry-pick..., git push --force, as proposed. I have to admit I have no idea what exactly that did to either my forked repo and/or the github bitcoin/bitcoin repository.
The changes seem right, though.
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 14, 2017

utACK
@MarcoFalke
Member

MarcoFalke commented Jul 16, 2017

Please run through clang format to fix white space and braces. Also adjust the commit message with git commit --amend -m 'Tests: address placement should be deterministic by default' or similar.
@ReneNyffenegger

ReneNyffenegger commented Jul 17, 2017

done clang formatting and amended commit message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment