{"action":"created","issue":{"url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/issues/850","repository_url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin","labels_url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/issues/850/labels{/name}","comments_url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/issues/850/comments","events_url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/issues/850/events","html_url":"https://github.com/thaliproject/Thali_CordovaPlugin/pull/850","id":170371521,"number":850,"title":"#777 Additional native unit tests","user":{"login":"mlesnic","id":19724972,"avatar_url":"https://avatars.githubusercontent.com/u/19724972?v=3","gravatar_id":"","url":"https://api.github.com/users/mlesnic","html_url":"https://github.com/mlesnic","followers_url":"https://api.github.com/users/mlesnic/followers","following_url":"https://api.github.com/users/mlesnic/following{/other_user}","gists_url":"https://api.github.com/users/mlesnic/gists{/gist_id}","starred_url":"https://api.github.com/users/mlesnic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mlesnic/subscriptions","organizations_url":"https://api.github.com/users/mlesnic/orgs","repos_url":"https://api.github.com/users/mlesnic/repos","events_url":"https://api.github.com/users/mlesnic/events{/privacy}","received_events_url":"https://api.github.com/users/mlesnic/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/labels/4%20-%20Done","name":"4 - Done","color":"CCCCCC"},{"url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/labels/cla-already-signed","name":"cla-already-signed","color":"009800"}],"state":"closed","locked":false,"assignee":{"login":"yaronyg","id":1480964,"avatar_url":"https://avatars.githubusercontent.com/u/1480964?v=3","gravatar_id":"","url":"https://api.github.com/users/yaronyg","html_url":"https://github.com/yaronyg","followers_url":"https://api.github.com/users/yaronyg/followers","following_url":"https://api.github.com/users/yaronyg/following{/other_user}","gists_url":"https://api.github.com/users/yaronyg/gists{/gist_id}","starred_url":"https://api.github.com/users/yaronyg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yaronyg/subscriptions","organizations_url":"https://api.github.com/users/yaronyg/orgs","repos_url":"https://api.github.com/users/yaronyg/repos","events_url":"https://api.github.com/users/yaronyg/events{/privacy}","received_events_url":"https://api.github.com/users/yaronyg/received_events","type":"User","site_admin":false},"assignees":[{"login":"yaronyg","id":1480964,"avatar_url":"https://avatars.githubusercontent.com/u/1480964?v=3","gravatar_id":"","url":"https://api.github.com/users/yaronyg","html_url":"https://github.com/yaronyg","followers_url":"https://api.github.com/users/yaronyg/followers","following_url":"https://api.github.com/users/yaronyg/following{/other_user}","gists_url":"https://api.github.com/users/yaronyg/gists{/gist_id}","starred_url":"https://api.github.com/users/yaronyg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yaronyg/subscriptions","organizations_url":"https://api.github.com/users/yaronyg/orgs","repos_url":"https://api.github.com/users/yaronyg/repos","events_url":"https://api.github.com/users/yaronyg/events{/privacy}","received_events_url":"https://api.github.com/users/yaronyg/received_events","type":"User","site_admin":false},{"login":"mlesnic","id":19724972,"avatar_url":"https://avatars.githubusercontent.com/u/19724972?v=3","gravatar_id":"","url":"https://api.github.com/users/mlesnic","html_url":"https://github.com/mlesnic","followers_url":"https://api.github.com/users/mlesnic/followers","following_url":"https://api.github.com/users/mlesnic/following{/other_user}","gists_url":"https://api.github.com/users/mlesnic/gists{/gist_id}","starred_url":"https://api.github.com/users/mlesnic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mlesnic/subscriptions","organizations_url":"https://api.github.com/users/mlesnic/orgs","repos_url":"https://api.github.com/users/mlesnic/repos","events_url":"https://api.github.com/users/mlesnic/events{/privacy}","received_events_url":"https://api.github.com/users/mlesnic/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/milestones/7","html_url":"https://github.com/thaliproject/Thali_CordovaPlugin/milestones/New%20Infra","labels_url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/milestones/7/labels","id":1498038,"number":7,"title":"New Infra","description":"This will be the first release of the new infrastructure working end to end.\r\n\r\n<!---\r\n@huboard:{\"order\":0.25}\r\n-->\r\n","creator":{"login":"yaronyg","id":1480964,"avatar_url":"https://avatars.githubusercontent.com/u/1480964?v=3","gravatar_id":"","url":"https://api.github.com/users/yaronyg","html_url":"https://github.com/yaronyg","followers_url":"https://api.github.com/users/yaronyg/followers","following_url":"https://api.github.com/users/yaronyg/following{/other_user}","gists_url":"https://api.github.com/users/yaronyg/gists{/gist_id}","starred_url":"https://api.github.com/users/yaronyg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yaronyg/subscriptions","organizations_url":"https://api.github.com/users/yaronyg/orgs","repos_url":"https://api.github.com/users/yaronyg/repos","events_url":"https://api.github.com/users/yaronyg/events{/privacy}","received_events_url":"https://api.github.com/users/yaronyg/received_events","type":"User","site_admin":false},"open_issues":63,"closed_issues":291,"state":"open","created_at":"2016-01-11T19:12:00Z","updated_at":"2016-08-31T23:37:39Z","due_on":null,"closed_at":null},"comments":50,"created_at":"2016-08-10T09:25:07Z","updated_at":"2016-09-01T00:29:11Z","closed_at":"2016-08-17T08:01:47Z","pull_request":{"url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/pulls/850","html_url":"https://github.com/thaliproject/Thali_CordovaPlugin/pull/850","diff_url":"https://github.com/thaliproject/Thali_CordovaPlugin/pull/850.diff","patch_url":"https://github.com/thaliproject/Thali_CordovaPlugin/pull/850.patch"},"body":"PR that implements #777\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/thaliproject/thali_cordovaplugin/850)\n<!-- Reviewable:end -->\n\r\n\r\n<!---\r\n@huboard:{\"order\":99.02696404565017,\"milestone_order\":850,\"custom_state\":\"\"}\n-->\r\n"},"comment":{"url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/issues/comments/243942235","html_url":"https://github.com/thaliproject/Thali_CordovaPlugin/pull/850#issuecomment-243942235","issue_url":"https://api.github.com/repos/thaliproject/Thali_CordovaPlugin/issues/850","id":243942235,"user":{"login":"yaronyg","id":1480964,"avatar_url":"https://avatars.githubusercontent.com/u/1480964?v=3","gravatar_id":"","url":"https://api.github.com/users/yaronyg","html_url":"https://github.com/yaronyg","followers_url":"https://api.github.com/users/yaronyg/followers","following_url":"https://api.github.com/users/yaronyg/following{/other_user}","gists_url":"https://api.github.com/users/yaronyg/gists{/gist_id}","starred_url":"https://api.github.com/users/yaronyg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yaronyg/subscriptions","organizations_url":"https://api.github.com/users/yaronyg/orgs","repos_url":"https://api.github.com/users/yaronyg/repos","events_url":"https://api.github.com/users/yaronyg/events{/privacy}","received_events_url":"https://api.github.com/users/yaronyg/received_events","type":"User","site_admin":false},"created_at":"2016-09-01T00:29:11Z","updated_at":"2016-09-01T00:29:11Z","body":"My top level concern with this PR is that it uses magic thread.sleep() wait times all over the place. That isn't safe. As you already experienced when you had to increase some wait times. There is nothing magical about a wait time. Something that usually takes 2 seconds can sometimes take 4 and then the tests fail for no obvious reason. Unreliable tests are useless because devs quickly learn to ignore them.\n\nSo every thread.sleep needs to be replaced with a production quality test check. That would be one where we actually check for the condition we are waiting for and only if it isn't there yet do we go for a very short sleep (no more than 200ms) and check again. So if we are waiting for a thread to start then use a semaphore (which still needs some kind of timer on it so that if there is a bug we don't wait forever and will error out). If we are waiting for a class to be created or destroyed then write a test to confirms if the class is created or destroyed and if it isn't yet then go to a short wait. And yes again we should check how many short waits we have had and if they exceed some limit then we need to error out.\n\nBut magic wait times are just not workable.\n\n---\n\nReviewed 6 of 12 files at r1, 1 of 1 files at r2, 15 of 15 files at r3.\nReview status: all files reviewed at latest revision, 13 unresolved discussions.\n\n---\n\n*[src/android/test/com/test/thalitest/RegisterExecuteUT.java, line 30 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQXz30ydzgWMFfghd5_:-KQXz30ydzgWMFfghd5a:bu6pz23) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/src/android/test/com/test/thalitest/RegisterExecuteUT.java#L30)):*\n> ```Java\n>         ConnectionHelperTest.mConnectionHelper = new ConnectionHelper();\n>         try {\n>                 if(methodName.equals(\"onPeerLost\")){\n> ```\n\nIsn't this what Switch was invented for? And please have a default so we can have some reasonable error if we get a call for a method we don't support.\n\n---\n\n*[src/android/test/com/test/thalitest/RegisterExecuteUT.java, line 31 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQXzJeCHX9kolfgGMnd:-KQXzJeCHX9kolfgGMne:b-3gej8u) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/src/android/test/com/test/thalitest/RegisterExecuteUT.java#L31)):*\n> ```Java\n>         try {\n>                 if(methodName.equals(\"onPeerLost\")){\n>                     Method onPeerLostMethod = ConnectionHelperTest.mConnectionHelper.getClass().getMethod(methodName, PeerProperties.class);\n> ```\n\nIf we are going to play reflection games then why have the 'if' test at all? Why not just use reflection directly to get the method and if it doesn't show up then throw an error?\n\nOtherwise I would suggest that if we are going to use a switch then we can just call the method properly, e.g. ConnectionHelperTest.mConnectionHelper.onPeerLost.\n\n---\n\n*[src/android/test/com/test/thalitest/RegisterExecuteUT.java, line 61 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQXzvbGF9CX7KGCO2f0:-KQXzvbGF9CX7KGCO2f1:bfnzns5) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/src/android/test/com/test/thalitest/RegisterExecuteUT.java#L61)):*\n> ```Java\n> \n>                 try {\n>                     Thread.sleep(2000);\n> ```\n\nWhat is the purpose of this delay?\n\n---\n\n*[src/android/test/com/test/thalitest/RegisterExecuteUT.java, line 96 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY-gGlEwyiQmQnfQdK:-KQY-gGlEwyiQmQnfQdL:b-toxm2x) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/src/android/test/com/test/thalitest/RegisterExecuteUT.java#L96)):*\n> ```Java\n>                 try {\n>                     if(!failures.equals(\"\")){\n>                         jsonObject.put(\"failures\", failures);\n> ```\n\nThis worries me as the failure string could potentially be quite large and I'm not sure what JXcore would do if we passed anything too big. Is there any reason we can't just return the failure data in the Android logs?\n\n---\n\n*[src/android/test/io/jxcore/node/ConnectionHelperTest.java, line 108 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY-vXLALPIJDFqDvJq:-KQY-vXLALPIJDFqDvJr:b-4e8bno) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/src/android/test/io/jxcore/node/ConnectionHelperTest.java#L108)):*\n> ```Java\n>     public void testDispose() throws Exception {\n>         mConnectionHelper.dispose();\n>         Thread.sleep(5000); //Wait for connectionHelper to dispose\n> ```\n\nPlease see my comments above about waits.\n\n---\n\n*[src/android/test/io/jxcore/node/ConnectivityMonitorTest.java, line 65 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY0kiMCk629tt1qysj:-KQY0kiNSmPs7hTpEjcg:be6jh7d) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/src/android/test/io/jxcore/node/ConnectivityMonitorTest.java#L65)):*\n> ```Java\n>             public void run() {\n>                 try {\n>                     Thread.sleep(5000);\n> ```\n\nPlease see previous comment. No magical time outs. Whatever you are waiting for needs to be a 'busy' wait where you are checking some condition to see if it has been achieved and if not then going to sleep. But these kind of random time outs just guarantee unstable tests that will randomly fail for no apparent reason.\n\n---\n\n*[src/android/test/io/jxcore/node/ConnectivityMonitorTest.java, line 121 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY0vp6ZnD1CAMGYwn-:-KQY0vp7wisbMWvWAmaf:b97cnq4) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/src/android/test/io/jxcore/node/ConnectivityMonitorTest.java#L121)):*\n> ```Java\n>         // Start monitoring connectivity, Wi-Fi and Bluetooth state changes.\n>         mConnectivityMonitor.start();\n>         Thread.sleep(5000);\n> ```\n\nInsert comment about not using Thread.sleep like this. And the fact that you had to increase the wait should tell you something.\n\n---\n\n*[src/android/test/io/jxcore/node/IncomingSocketThreadTest.java, line 110 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY3iqHcyowse78sLMo:-KQY3iqHcyowse78sLMp:b-djly1r) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/src/android/test/io/jxcore/node/IncomingSocketThreadTest.java#L110)):*\n> ```Java\n> \n>         mOutgoingSocketThread.start(); //Simulate end point to connect to\n>         Thread.sleep(3000); //Wait for thread to start\n> ```\n\nPlease just use a semaphore, not magical wait times.\n\n---\n\n*[src/android/test/io/jxcore/node/OutgoingSocketThreadMock.java, line 55 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY4pQ5QsxeCjlZpOan:-KQY4pQ6rm_C-gli-0N1:bq9fuih) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/src/android/test/io/jxcore/node/OutgoingSocketThreadMock.java#L55)):*\n> ```Java\n>         }\n> \n>         if (mServerSocket != null) {\n> ```\n\nIf mServerSocket is null don't we need to throw? I realize it should have been caught above but if this test fails then it wasn't.\n\n---\n\n*[test/www/jxcore/bv_tests/testNativeMethod.js, line 13 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY5oILI3btmjhUxn8_:-KQY5oIMo15xQ-DHDuZF:b32v14s) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/test/www/jxcore/bv_tests/testNativeMethod.js#L13)):*\n> ```JavaScript\n>         setup: function (t) {\n>             Mobile('peerAvailabilityChanged').registerToNative(function (peers) {\n>                 if (typeof peers.forEach !== 'function') {\n> ```\n\nPeers is required to be an array. If you aren't getting an array that is a test failure.\n\n---\n\n*[test/www/jxcore/bv_tests/testNativeMethod.js, line 20 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY65ERZ6ndq12c52tA:-KQY65ERZ6ndq12c52tB:bqgi3xv) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/test/www/jxcore/bv_tests/testNativeMethod.js#L20)):*\n> ```JavaScript\n>                 });\n>             });\n>         t.end();\n> ```\n\nPlease indent this line\n\n---\n\n*[test/www/jxcore/bv_tests/testNativeMethod.js, line 33 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY6FG9jW8Hk8392ILy:-KQY6FG9jW8Hk8392ILz:b-ppui6e) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/test/www/jxcore/bv_tests/testNativeMethod.js#L33)):*\n> ```JavaScript\n>                 console.log(result.Testing_);\n>                 setImmediate(function () {\n>                     t.equal(callbackPeer.peerIdentifier, '11:22:33:22:11:00-0',\n> ```\n\nIsn't this code depending on the peerAvailabilityChanged event happening before the setImmediate. Which is usually a good bet but not a 100% guaranteed one. The safe way to do this is take the registerToNative call out of setup and instead put it into the test. Then put this check into the callback function registered with registerToNative. You also need to put in a timer somewhere so that if there is a bug and the callback never happens the test will fail. But if the callback happens then the timer needs to be cancelled. This is a trivial use of setTimeout.\n\nThis comments applies to the next test too.\n\n---\n\n*[thali/package.json, line 33 \\[r3\\]](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-KQY6s5a_TsY4TU0bXXH:-KQY6s5bHz_NNIz5caJC:bzdew6j) ([raw file](https://github.com/thaliproject/thali_cordovaplugin/blob/9c91828358cada6cadfca14a9ca8625a14a7ea02/thali/package.json#L33)):*\n> ```JSON\n>     \"btconnectorlib2\": \"0.3.2\",\n>     \"jxcore-cordova\": \"0.1.2b\",\n>     \"jxcore-cordova-url\": \"https://jxcore.blob.core.windows.net/jxcore-release/jxcore-cordova/0.1.2b/release/io.jxcore.node.jx\"\n> ```\n\nIt doesn't really matter anymore since we have already fixed this but for future reference this URL points directly to storage rather than to the CDN. But we want to use the CDN because it has better availability. So please use jxcore.azureedge.net and not jxcore.blob.core.windows.net. This is just a FYI.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/thaliproject/thali_cordovaplugin/850#-:-KOq0NIqu2b95nv9wIhS:b-vvli7o)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"yaronyg"}}