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

INDY-1911: add integration tests #1034

Merged

Conversation

Toktar
Copy link
Contributor

@Toktar Toktar commented Dec 17, 2018

Signed-off-by: toktar renata.toktar@dsr-corporation.com

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
@Toktar Toktar changed the title [WIP][ci skip][INDY-1911] add tests INDY-1911: add tests Dec 17, 2018
@Toktar Toktar changed the title INDY-1911: add tests INDY-1911: add integration tests Dec 17, 2018
@Toktar
Copy link
Contributor Author

Toktar commented Dec 17, 2018

test this please

ensure_all_nodes_have_same_data(looper, nodes=txnPoolNodeSet)

for node in txnPoolNodeSet:
assert node.viewNo == view_no
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are already relying in this test on a fact that view change starts on some nodes I think it's better to do something like this:

Suggested change
assert node.viewNo == view_no
assert node.viewNo > view_no

Another option is to just get rid of this check, since actually all that we want here is to check that nodes will eventually come to consensus after some panics and restarts, and this is already done by ensureElectionsDone and ensure_all_nodes_have_same_data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

nodes_to_restart = txnPoolNodeSet[1:3]

# waiting to discard InstanceChange
looper.runFor(tconf.OUTDATED_INSTANCE_CHANGES_CHECK_INTERVAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it's better to wait for a little bit more than timeout interval:

Suggested change
looper.runFor(tconf.OUTDATED_INSTANCE_CHANGES_CHECK_INTERVAL)
looper.runFor(tconf.OUTDATED_INSTANCE_CHANGES_CHECK_INTERVAL +1)

Another option (although a bit more intrusive) is to actually wait until nodes don't have INSTANCE_CHANGE from panic_node anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to wait INSTANCE_CHANGE removing

def check():
assert all(panic_node.view_changer.instanceChanges.has_inst_chng_from(view_no + 1, node.name)
for node in nodes_to_restart)
assert not panic_node.view_changer.instanceChanges.has_inst_chng_from(view_no + 1, panic_node.name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second assert looks a bit excessive here. Also in first assert it might make sense to actually check that all nodes received instance changes from nodes_to_restart, not just panic_node

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the second assert we check that panic_node hasn't InstanceChange from itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this assert because added this check before nodes restarting

looper.removeProdable(name=node_to_disconnect.name)

# add node_to_disconnect to pool
node_to_disconnect = start_stopped_node(node_to_disconnect, looper, tconf,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that this is enough to clear state of node_to_disconnect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, beacause start_stopped_node create a new TestNode

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
@andkononykhin andkononykhin merged commit 96f0c97 into hyperledger:master Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants