Fix flappy integration test #88

mennopruijssers · 2016-09-13T13:29:36Z

This PR fixes three issues:

A race condition where a test completes and times out at the same time: this is fixed by breaking out the event-loop early after a time out.
Start running the test before the SUT is ready: fixed by waiting for an initial ping from the SUT.
The implementation of lookup in the integration tests is incorrect. Sometimes it returns an incorrect result

mennopruijssers · 2016-09-13T13:54:36Z

Test's pass locally on both go and node:

ringpop-go:

make test-integration
test/run-integration-tests
[fetch ringpop-common] + cd test/ringpop-common
[fetch ringpop-common] + git fetch origin fix-flappy
[build testpop] + make testpop
[build testpop] rm -f testpop
[build testpop] go build ./scripts/testpop/
[fetch ringpop-common] From ssh://github.com/uber/ringpop-common
[fetch ringpop-common]  * branch            fix-flappy -> FETCH_HEAD
[fetch ringpop-common] + git checkout FETCH_HEAD
[fetch ringpop-common] Previous HEAD position was 4e35f91... wait for first ping in prepare cluster
[fetch ringpop-common] HEAD is now at 0209af7... Don’t conumse initial ping
[fetch ringpop-common] + cd -
[fetch ringpop-common] + cd test/ringpop-common/test
[fetch ringpop-common] + cd -
[test-runner] Running test for cluster size 1...
[test-runner] Running test for cluster size 2...
[test-runner] Running test for cluster size 3...
[test-runner] Running test for cluster size 4...
[test-runner] Running test for cluster size 5...
[test-runner] Running test for cluster size 10...
Tests passed

ringpop-node:

npm run test-shared-integration-tests

> [email protected] test-shared-integration-tests /Users/menno/Sources/Uber/nodejs/ringpop-node
> test/run-shared-integration-tests

[fetch ringpop-common] + cd test/ringpop-common
[fetch ringpop-common] + git fetch origin fix-flappy
[fetch ringpop-common] From ssh://github.com/uber/ringpop-common
[fetch ringpop-common]  * branch            fix-flappy -> FETCH_HEAD
[fetch ringpop-common] + git checkout FETCH_HEAD
[fetch ringpop-common] Previous HEAD position was 4e35f91... wait for first ping in prepare cluster
[fetch ringpop-common] HEAD is now at 0209af7... Don’t conumse initial ping
[fetch ringpop-common] + cd -
[fetch ringpop-common] + cd test/ringpop-common/test
[fetch ringpop-common] + npm install
[fetch ringpop-common] npm WARN package.json [email protected] No license field.
[fetch ringpop-common] + cd -
[test-runner] Running test for cluster size 1...
[test-runner] Running test for cluster size 2...
[test-runner] Running test for cluster size 3...
[test-runner] Running test for cluster size 4...
[test-runner] Running test for cluster size 5...
[test-runner] Running test for cluster size 10...
Tests passed

thanodnl · 2016-09-13T14:16:01Z

test/test-util.js

@@ -121,8 +121,9 @@ function testStateTransitions(ns, initial, newState, finalState, incNoDelta, sta
 function prepareCluster(insert_fns) {
    return function(t, tc, n) {
        return [
-            dsl.assertDetectChecksumMethod(t, tc),
+            dsl.waitForPing(t, tc, false),


Maybe we should describe why we are waiting for a ping, and why we don't consume it. Both are very subtile but important facts.

thanodnl · 2016-09-13T14:21:48Z

LGTM

thanodnl · 2016-09-13T17:48:30Z

Nice find on the bug in the lookup function of the test coordinator, fix is legit.

CorgiMan · 2016-09-14T15:56:17Z

test/test-util.js

+            // By waiting for the first ping we make sure the SUT is ready to go.
+            // We don't consume it so other tests (especially the ping-tests)
+            // can still assert implementation details related to ping distribution.
+            dsl.waitForPing(t, tc, false),


I'm a bit confused. Shouldn't the joins happen before the ping?

The idea behind it is to wait until the node starts pinging. At that point we can consider it to be fully ready and start running the other validations.

But wouldn't it make sense to first wait for the joins, and then for the pinging. I don't think the node will ping before it has joined.

Makes sense, I'll reorder them

After a discussion with Nils we changed it back (and added a comment). The reason why we're waiting for the initial ping first is so we wait until the node is fully ready before checking the number of joins. This way we're able to detect if there are too many joins during standup.

makes sense, thanks!

CorgiMan · 2016-09-15T09:31:52Z

Looks good!

Menno Pruijssers added 3 commits September 13, 2016 15:10

stop processing events after time out

54fdb13

wait for first ping in prepare cluster

4e35f91

Don’t conumse initial ping

0209af7

thanodnl reviewed Sep 13, 2016
View reviewed changes

add comment on wait for ping

be35a4b

fix lookup function

865869a

Fixed lookup for real

7b065e8

CorgiMan reviewed Sep 14, 2016
View reviewed changes

Menno Pruijssers added 3 commits September 15, 2016 10:40

improve lookup function

3f2d476

reorder wait for ping/joins

ac228ac

Revert reordering

009b649

CorgiMan approved these changes Sep 15, 2016

View reviewed changes

thanodnl approved these changes Sep 15, 2016

View reviewed changes

mennopruijssers merged commit a21877d into master Sep 15, 2016

mennopruijssers deleted the fix-flappy branch September 15, 2016 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flappy integration test #88

Fix flappy integration test #88

mennopruijssers commented Sep 13, 2016 •

edited

Loading

mennopruijssers commented Sep 13, 2016 •

edited

Loading

thanodnl Sep 13, 2016

thanodnl commented Sep 13, 2016

thanodnl commented Sep 13, 2016

CorgiMan Sep 14, 2016 •

edited

Loading

mennopruijssers Sep 15, 2016

CorgiMan Sep 15, 2016

mennopruijssers Sep 15, 2016

mennopruijssers Sep 15, 2016 •

edited

Loading

CorgiMan Sep 15, 2016

CorgiMan commented Sep 15, 2016

Fix flappy integration test #88

Fix flappy integration test #88

Conversation

mennopruijssers commented Sep 13, 2016 • edited Loading

mennopruijssers commented Sep 13, 2016 • edited Loading

thanodnl Sep 13, 2016

Choose a reason for hiding this comment

thanodnl commented Sep 13, 2016

thanodnl commented Sep 13, 2016

CorgiMan Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

mennopruijssers Sep 15, 2016

Choose a reason for hiding this comment

CorgiMan Sep 15, 2016

Choose a reason for hiding this comment

mennopruijssers Sep 15, 2016

Choose a reason for hiding this comment

mennopruijssers Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

CorgiMan Sep 15, 2016

Choose a reason for hiding this comment

CorgiMan commented Sep 15, 2016

mennopruijssers commented Sep 13, 2016 •

edited

Loading

mennopruijssers commented Sep 13, 2016 •

edited

Loading

CorgiMan Sep 14, 2016 •

edited

Loading

mennopruijssers Sep 15, 2016 •

edited

Loading