Add sockets.js tests and minor fixes by vvmnnnkv · Pull Request #50 · OpenMined/syft.js

vvmnnnkv · 2019-09-02T11:01:30Z

Fixes #47

cereallarceny · 2019-09-02T11:42:57Z

Nice work @vvmnnnkv! I noticed that the tests on TravisCI never actually finish. See the above screenshot. I believe this is from the socket never being cleaned up and thus, it continues to "ping". The screenshot doesn't show it, but the Travis test kept going for 40 minutes with that same message every 20 seconds until I shut it down. Can you clean that up and I'll take another look?

# Conflicts: # src/sockets.js

codecov · 2019-09-02T16:26:11Z

Codecov Report

❗ No coverage uploaded for pull request base (grid-syft@da31141). Click here to learn what that means.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             grid-syft      #50   +/-   ##
============================================
  Coverage             ?   30.76%           
============================================
  Files                ?        2           
  Lines                ?      143           
  Branches             ?       13           
============================================
  Hits                 ?       44           
  Misses               ?       89           
  Partials             ?       10

Impacted Files	Coverage Δ
src/sockets.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da31141...9346128. Read the comment docs.

cereallarceny · 2019-09-02T17:33:19Z

src/sockets.js

    return new Promise((resolve, reject) => {
-      data.instanceId = this.instanceId;
+      // shallow copy + instanceId
+      let _data = Object.assign(


Why do it like this @vvmnnnkv? What was wrong with the old way this was being assigned?

It had side effect of adding instanceId property on the data object passed into .send().
Another way could be sending instanceId outside of data?
{type, instanceId, data}?

cereallarceny · 2019-09-03T10:44:40Z

test/sockets.test.js

-const onOpen = event => console.log('NEW OPEN', event);
-const onClose = event => console.log('NEW CLOSE', event);
-const onError = event => console.log('NEW ERROR', event);
+/**


We're not using JSDoc, feel free to throw these comments out for now.

cereallarceny · 2019-09-03T10:45:30Z

test/sockets.test.js

+ * @param event Event name
+ * @returns {Promise}
+ */
+function makeEventPromise(emitter, event) {


Always declare function in ES6 fashion:

const makeEventPromise = (emitter, event) { ... };

cereallarceny · 2019-09-03T10:45:54Z

test/sockets.test.js

+ * @param event Event name
+ * @returns {Promise}
+ */
+function makeEventPromise(emitter, event) {


Another thing: what does this function actually do?

This is for convenience. It wraps event callback in a promise. The promise is resolved when the event is triggered.
E.g. when we want to await for established connection on the mock server:
const socket = await makeEventPromise(mockServer, 'connection');

cereallarceny · 2019-09-03T10:46:33Z

test/sockets.test.js

+ */
+function makeEventPromise(emitter, event) {
+  let resolver;
+  let promise = new Promise(resolve => resolver = resolve);


Always use const if you can. If you cannot, use let. In this case, you should prefer const.

cereallarceny · 2019-09-03T10:47:50Z

test/sockets.test.js

+}

 describe('Sockets', () => {
+  // common test objects


Comments should always start capitalized. They should also be fairly descriptive. To me, I don't think you need any comments in this file at all. It's a test file - so it should be "strictly business". Go ahead and remove them unless they're absolutely critical to the understanding of something complex. :)

OK, removed some comments

cereallarceny · 2019-09-03T10:49:54Z

test/sockets.test.js


-  test('can communicate with a socket server', async () => {
-    const message = { message: 'This is a test' };
+  test('keep-alive messages', async() => {


Test messages ('keep-alive messages') should read as a full sentence with the description. Our description (written above) is 'Sockets', so a full sentence should be 'Sockets can have a custom keep-alive timeout' with 'Sockets' being the description message (from the describe() function) and 'can have a custom keep-alive timeout' being the test message (from the test() function).

cereallarceny · 2019-09-03T10:50:39Z

test/sockets.test.js

-      onError: event => onError(event)
+      keepAliveTimeout
    });
+    let serverSocket = await mockServer.connected;


Again... use const, everywhere. This will be the last comment I write about in my review, but please change all references that can be changed.

cereallarceny · 2019-09-03T10:52:11Z

test/sockets.test.js

+    // one message is sent right after establishing the connection hence +1 
+    // (do we really need that?)
+    expect(messages).toHaveLength(expectedMessagesCount + 1);
+    let expectedTypes  =[];


This spacing is incorrect. Is prettier running for you? Normally Prettier will fix things like this.

I thought prettier would run automatically :) Fixed

test/sockets.test.js

cereallarceny · 2019-09-04T14:04:26Z

src/sockets.js

+      onOpen,
+      onClose,
+      onMessage,
+      keepAliveTimeout


Actually, it would be better if we did:

onClose, onMessage, keepAliveTimeout = 20000

You can assign defaults to your arguments this way, rather than doing it below (on line 18, which you can now remove).

cereallarceny · 2019-09-04T14:04:38Z

src/sockets.js


    const keepAlive = () => {
-      const timeout = 20000;
+      const timeout = keepAliveTimeout || 20000;


You can remove this, see above comment.

add sockets.js tests and minor fixes

5ad6a39

cereallarceny self-requested a review September 2, 2019 11:40

Vova Manannikov added 3 commits September 2, 2019 17:49

replace non-finishing test

d0c0ecb

Merge branch 'grid-syft' into socket-tests

176785f

# Conflicts: # src/sockets.js

fix error in webrtc test

556ffbf

cereallarceny suggested changes Sep 3, 2019

View reviewed changes

cereallarceny and others added 3 commits September 3, 2019 13:40

Merge branch 'grid-syft' into socket-tests

5d0c98b

fix code style

8dfb8d3

revert data shallow copy

df31983

cereallarceny reviewed Sep 4, 2019

View reviewed changes

Update sockets.js

9346128

cereallarceny approved these changes Sep 4, 2019

View reviewed changes

cereallarceny merged commit 533ab48 into OpenMined:grid-syft Sep 4, 2019

Uh oh!

Conversation

vvmnnnkv commented Sep 2, 2019 • edited by cereallarceny Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cereallarceny commented Sep 2, 2019

Uh oh!

codecov bot commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vvmnnnkv commented Sep 2, 2019 •

edited by cereallarceny

Loading

codecov bot commented Sep 2, 2019 •

edited

Loading