#86

[vasilevskayaem]

#86

---

This change is

[ThaliTester]

PR is added to the queue for testing as 4. task. (b532c5c)

[msftclas]

Hi @vasilevskayaem, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[ThaliTester]

Test (Success) 83412154 build is completed (b532c5c)

See https://github.com/ThaliTester/TestResults/tree/83412154b532c5c__86__vasilevskayaem/ for the logs

[ThaliTester]

Test 83412154b532c5c(b532c5c) has failed

See https://github.com/ThaliTester/TestResults/tree/83412154b532c5c__86__vasilevskayaem/ for the fail logs

[ThaliTester]

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

[ThaliTester]

Test (Success) 83412154 build is completed (f1c1c14)

See https://github.com/ThaliTester/TestResults/tree/83412154f1c1c14__86__vasilevskayaem/ for the logs

[ThaliTester]

Test 83412154f1c1c14(f1c1c14) has failed

See https://github.com/ThaliTester/TestResults/tree/83412154f1c1c14__86__vasilevskayaem/ for the fail logs

[ThaliTester]

PR is added to the queue for testing as 4. task. (945d897)

[ThaliTester]

Test (Success) 83412154 build is completed (945d897)

See https://github.com/ThaliTester/TestResults/tree/83412154945d897__86__vasilevskayaem/ for the logs

[ThaliTester]

Test 83412154945d897(945d897) has successfully finished without an error

See https://github.com/ThaliTester/TestResults/tree/83412154945d897__86__vasilevskayaem/ for the logs

[msftclas]

@vasilevskayaem, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[evabishchevich]

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

---

BtConnectorLib/btconnectorlib2/src/main/java/org/thaliproject/p2p/btconnectorlib/utils/PeerModel.java, line 199 [r3] (raw file):

                PeerProperties oldPeerProperties = null;
                Timestamp lastUpdatePeerPropertiesTime = mDiscoveredPeers.get(peerPropertiesToAddOrUpdate);
                if (lastUpdatePeerPropertiesTime != null && System.currentTimeMillis() - lastUpdatePeerPropertiesTime.getTime() > mSettings.DEFAULT_PEER_PROPERTIES_UPDATE_PERIOD_IN_MILLISECONDS) {

Please, stand out this check into a separate function. Don't place so much code on one line. It's about 200 symbols.
Don't use variable to access to static members of the class, use DiscoveryManagerSettings.DEFAULT_PEER_PROPERTIES_UPDATE_PERIOD_IN_MILLISECONDS.

---

BtConnectorLib/btconnectorlib2/src/test/java/org/thaliproject/p2p/btconnectorlib/utils/PeerModelTest.java, line 349 [r3] (raw file):

        HashMap<PeerProperties, Timestamp> mDiscoveredPeers = new HashMap<>();
        long initialTime = System.currentTimeMillis();
        mDiscoveredPeers.put(mMockPeerProperties, new Timestamp(initialTime - DiscoveryManagerSettings.DEFAULT_PEER_PROPERTIES_UPDATE_PERIOD_IN_MILLISECONDS-1));

Too long line

---

BtConnectorLib/btconnectorlib2/src/test/java/org/thaliproject/p2p/btconnectorlib/utils/PeerModelTest.java, line 356 [r3] (raw file):

        doNothing().when(mMockListener).onPeerUpdated(isA(PeerProperties.class));
        when(mMockDiscoveryManagerSettings.getPeerExpiration()).thenReturn(DiscoveryManagerSettings.DEFAULT_PEER_EXPIRATION_IN_MILLISECONDS);

Too long line

---

Comments from Reviewable

[evabishchevich]

Reviewed 1 of 2 files at r1, 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[ThaliTester]

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

[ThaliTester]

Test (Success) 83412154 build is completed (9cf5167)

See https://github.com/ThaliTester/TestResults/tree/834121549cf5167__86_vasilevskayaem/ for the logs

[ThaliTester]

Test 834121549cf5167(9cf5167) has failed

See https://github.com/ThaliTester/TestResults/tree/834121549cf5167__86_vasilevskayaem/ for the fail logs

[ThaliTester]

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

[ThaliTester]

Test (Success) 83412154 build is completed (b1a4712)

See https://github.com/ThaliTester/TestResults/tree/83412154b1a4712__86_vasilevskayaem/ for the logs

[ThaliTester]

Test 83412154b1a4712(b1a4712) has successfully finished without an error

See https://github.com/ThaliTester/TestResults/tree/83412154b1a4712__86_vasilevskayaem/ for the logs

[evabishchevich]

Review status: 2 of 5 files reviewed at latest revision, 4 unresolved discussions.

---

BtConnectorLib/btconnectorlib2/src/main/java/org/thaliproject/p2p/btconnectorlib/utils/PeerModel.java, line 194 [r5] (raw file):

     * @return The found not expired peer properties or null otherwise.
     */
    public PeerProperties getNotExpiredPeerProperties(PeerProperties peerPropertiesToAddOrUpdate) {

We need a little bit different logic. We shouldn't notify node layer that there is a new peer in case if we've seen that peer before. We have to implement next scenario:

1. A1 advertises X
2. A2 discovers X, check current timer. If there is no timer, add the new peer, notify the node layer and start the timer.
3. A2 discovers X, check the timer. Timer is on, update timestamp.
4. A2 discovers X, check the timer. Timer is on, update timestamp.
5. A2 discovers X, check the timer. Timer is on, update timestamp.
6. A2 discovers Y, check the timer. Timer is on, add the new peer.
7. A2 discovers X, check the timer. Timer is on, update timestamp.
8. The timer has expired. Notify node layer that X peer - updated, Y peer - added. Start the timer.
9. A2 discovers X, check the timer. Timer is on, update timestamp.
10. A2 discovers Z, check the timer. Timer is on, add the new peer.
11. The timer has expired. Notify node layer that X peer - updated, Z peer - added. Start the timer.
12. No events
13. The timer has expired. No updates - no need to notify node layer.
14. A2 discovers X, check the timer. Timer is off, update timestamp, notify the node layer and start the timer.
    Go to point 3 of this list.

---

Comments from Reviewable

[evabishchevich]

Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[vasilevskayaem]

work started in vNext_vasilevskayaem_86, after changes appears next core test fails, continue working

--- Failed tests ---
2016-09-09T10:28:32.191Z - warn: 64. #startUpdateAdvertisingAndListening - destroying remote peers connection kills the local connection - fail
2016-09-09T10:28:32.191Z - warn: 67. We do not emit peerAvailabilityChanged events until one of the start methods is called - fail
2016-09-09T10:28:32.191Z - warn: 68. Test updating advertising and parallel data transfer - fail
2016-09-09T10:28:32.191Z - warn: 69. initial peer discovery - fail
2016-09-09T10:28:32.191Z - warn: 70. update peer discovery 1 - fail
2016-09-09T10:28:32.191Z - warn: 71. update peer discovery 2 - fail
2016-09-09T10:28:32.191Z - warn: 72. check latest peer discovery - fail
2016-09-09T10:28:32.191Z - warn: 73. no peer discovery - fail
2016-09-09T10:28:32.191Z - warn: 107. Action fails because of a bad hostname. - fail
2016-09-09T10:28:32.192Z - info:
2016-09-09T11:12:23.860Z - info: Running on android test: 59. Can connect to a remote peer
2016-09-09T11:13:48.396Z - warn: Failed on android test: 59. Can connect to a remote peer

[yaronyg]

I would suggest that we close this PR and put a note in #86 which branch has the relevant code.

[larryonoff]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

BtConnectorLib/settings.gradle, line 2 at r5 (raw file):

include ':btconnectorlib2'
gradle.ext.version = "0.3.4_alpha";

Is it the old lib version ?

---

Comments from Reviewable

[vasilevskayaem]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

BtConnectorLib/settings.gradle, line 2 at r5 (raw file):

Previously, larryonoff (larryonoff) wrote…

Is it the old lib version ?

no. I've got some tests failures in native mode with those changes so this branch wasn't merged into master

---

Comments from Reviewable