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

Fix less travelled paths #646

Closed
wants to merge 2 commits into
base: vNext_vjrantal_500
from

Conversation

Projects
None yet
7 participants
@tobybrad
Contributor

tobybrad commented Mar 19, 2016

This change is Reviewable

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Mar 19, 2016

Member

PR is added to the queue for testing as 1. task. (c6bc463)

Member

ThaliTester commented Mar 19, 2016

PR is added to the queue for testing as 1. task. (c6bc463)

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Mar 19, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Mar 19, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Mar 21, 2016

Member

PR is added to the queue for testing as 1. task. (508fffe)

Member

ThaliTester commented Mar 21, 2016

PR is added to the queue for testing as 1. task. (508fffe)

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Mar 21, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Mar 21, 2016

@vjrantal

This comment has been minimized.

Show comment
Hide comment
@vjrantal

vjrantal Mar 21, 2016

Member

Review status: 0 of 6 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


a discussion (no related file):
This test still hangs for me locally and in both CI environments. We now support running individual tests in coordinated desktop environment so you can reproduce this like jx runCoordinatedTests.js --filter bv_tests/testThaliMobileNative.js. Please check first locally that this test passes before pushing the next commit.

The latest commit seemed to be about making the data sending test cross-platform, but just wanted to point out that without the changes done in vNext_vjrantal_500_tobe_fix_ios this test already worked on desktop and on Android.

There is some whitespace and linting issues, but I didn't comment on those since I think most important would be to get the basic logic right. I am fine merging with the linting issue since I can easily fix them afterwards.


src/ios/MultipeerConnectivity/THEMultipeerClient.m, line 300 [r2] (raw file):
If you want to change the error message, it must be also changed in specification and Android implementation, because otherwise, it is hard to write cross-platform node code. If not changing everywhere, I suggest we don't change here either.


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 145 [r2] (raw file):
Why would a peer become unreachable in an environment where the devices are next to each other (which is the case when we run the tests)? If this happens, I'd fail the test overall since sounds like an error scenario.

Now that there is a return here, doesn't it mean that if this code path would get run, we would not call successCb nor would be schedule a connect retry so wouldn't the test just hang?


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 147 [r2] (raw file):
In what scenarios the peer would already be connected while this test runs? The test starts from a stopped state and we first call all the starts. The, we try to connect to the first peer we see. Why would that peer already be connected? Sounds like an error if we end up to this code path.


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 220 [r2] (raw file):
If we get a failure to connect after 10 retries, I think the test should overall be marked as a failure so I'd remove this check.


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 235 [r2] (raw file):
Now that the connecting-check was turned into a connected-check, here is what I think might happen.

We start concurrently connecting to every peer we discover. For example, if in CI there would be more than two Thali apps running. If all would go great and all connect-attempts would succeed, the test would fail, because t.end() would be called more than once.

I suggest we keep the test logic like this:

  • We do peer discovery as long as we have found a peer
  • We try to connect to only that peer max 10 times and if that fails, we mark the test overall as a failure

test/www/jxcore/bv_tests/testThaliMobileNative.js, line 271 [r2] (raw file):
This isn't used anywhere.


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 357 [r2] (raw file):
Same as earlier comment.


Comments from the review on Reviewable.io

Member

vjrantal commented Mar 21, 2016

Review status: 0 of 6 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


a discussion (no related file):
This test still hangs for me locally and in both CI environments. We now support running individual tests in coordinated desktop environment so you can reproduce this like jx runCoordinatedTests.js --filter bv_tests/testThaliMobileNative.js. Please check first locally that this test passes before pushing the next commit.

The latest commit seemed to be about making the data sending test cross-platform, but just wanted to point out that without the changes done in vNext_vjrantal_500_tobe_fix_ios this test already worked on desktop and on Android.

There is some whitespace and linting issues, but I didn't comment on those since I think most important would be to get the basic logic right. I am fine merging with the linting issue since I can easily fix them afterwards.


src/ios/MultipeerConnectivity/THEMultipeerClient.m, line 300 [r2] (raw file):
If you want to change the error message, it must be also changed in specification and Android implementation, because otherwise, it is hard to write cross-platform node code. If not changing everywhere, I suggest we don't change here either.


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 145 [r2] (raw file):
Why would a peer become unreachable in an environment where the devices are next to each other (which is the case when we run the tests)? If this happens, I'd fail the test overall since sounds like an error scenario.

Now that there is a return here, doesn't it mean that if this code path would get run, we would not call successCb nor would be schedule a connect retry so wouldn't the test just hang?


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 147 [r2] (raw file):
In what scenarios the peer would already be connected while this test runs? The test starts from a stopped state and we first call all the starts. The, we try to connect to the first peer we see. Why would that peer already be connected? Sounds like an error if we end up to this code path.


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 220 [r2] (raw file):
If we get a failure to connect after 10 retries, I think the test should overall be marked as a failure so I'd remove this check.


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 235 [r2] (raw file):
Now that the connecting-check was turned into a connected-check, here is what I think might happen.

We start concurrently connecting to every peer we discover. For example, if in CI there would be more than two Thali apps running. If all would go great and all connect-attempts would succeed, the test would fail, because t.end() would be called more than once.

I suggest we keep the test logic like this:

  • We do peer discovery as long as we have found a peer
  • We try to connect to only that peer max 10 times and if that fails, we mark the test overall as a failure

test/www/jxcore/bv_tests/testThaliMobileNative.js, line 271 [r2] (raw file):
This isn't used anywhere.


test/www/jxcore/bv_tests/testThaliMobileNative.js, line 357 [r2] (raw file):
Same as earlier comment.


Comments from the review on Reviewable.io

@yaronyg yaronyg added this to the New Infra milestone Mar 21, 2016

@yaronyg yaronyg assigned yaronyg and unassigned tobybrad Mar 21, 2016

@yaronyg yaronyg added the 3 - Working label Mar 21, 2016

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Mar 21, 2016

Member

Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


a discussion (no related file):
I don't get a hang, what I get is an infinite list of identified peers creating an infinite number of connections. I think this is largely because of a bug in the wifiBasedNativeMock. So I'm sending this back to @tobybrad to review and respond to the comments but I'm going to go fix the native mock and see if that fixes the issues with the tests on the desktop. For now @tobybrad should just worry about the tests passing on iOS.


src/ios/MultipeerConnectivity/THEMultipeerManager.m, line 347 [r2] (raw file):
So I vaguely remember that we had issues with announcements just sort of disappearing and we needed to restart the announcements on a fairly frequent basis to keep things moving along. I also remember that this wasn't supposed to disturb any existing MCSessions and that we would use the same peerIdentifier across different starts so that other clients would realize that this is, in fact, the same client that they saw previously. So are you saying that the announcements are so unstable that we have to restart every 10 seconds?

But how does this lead to the connection issues that @vjrantal raised in the js code? Shouldn't these restarts be effectively invisible to the node.js layer outside of getting an additional stream of peer updates that always have the same peer ID?


Comments from the review on Reviewable.io

Member

yaronyg commented Mar 21, 2016

Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


a discussion (no related file):
I don't get a hang, what I get is an infinite list of identified peers creating an infinite number of connections. I think this is largely because of a bug in the wifiBasedNativeMock. So I'm sending this back to @tobybrad to review and respond to the comments but I'm going to go fix the native mock and see if that fixes the issues with the tests on the desktop. For now @tobybrad should just worry about the tests passing on iOS.


src/ios/MultipeerConnectivity/THEMultipeerManager.m, line 347 [r2] (raw file):
So I vaguely remember that we had issues with announcements just sort of disappearing and we needed to restart the announcements on a fairly frequent basis to keep things moving along. I also remember that this wasn't supposed to disturb any existing MCSessions and that we would use the same peerIdentifier across different starts so that other clients would realize that this is, in fact, the same client that they saw previously. So are you saying that the announcements are so unstable that we have to restart every 10 seconds?

But how does this lead to the connection issues that @vjrantal raised in the js code? Shouldn't these restarts be effectively invisible to the node.js layer outside of getting an additional stream of peer updates that always have the same peer ID?


Comments from the review on Reviewable.io

@yaronyg yaronyg assigned tobybrad and unassigned yaronyg Mar 21, 2016

@vjrantal vjrantal closed this Mar 22, 2016

@vjrantal vjrantal removed the in progress label Mar 22, 2016

@vjrantal vjrantal reopened this Mar 22, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Mar 22, 2016

Member

PR is added to the queue for testing as 1. task. (508fffe)

Member

ThaliTester commented Mar 22, 2016

PR is added to the queue for testing as 1. task. (508fffe)

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Mar 22, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Mar 22, 2016

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Jul 20, 2016

Member

@baydet This is now morphing per the mail thread into figuring out how to diff this with the version I talked about in email and then decide what to keep and what to drop.

Member

yaronyg commented Jul 20, 2016

@baydet This is now morphing per the mail thread into figuring out how to diff this with the version I talked about in email and then decide what to keep and what to drop.

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Aug 15, 2016

Member

I'm closing this since we are rewriting the code base

Member

yaronyg commented Aug 15, 2016

I'm closing this since we are rewriting the code base

@yaronyg yaronyg closed this Aug 15, 2016

@yaronyg yaronyg removed the in progress label Aug 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment