Skip to content
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

test: add a missing regression test for GH-329 #359

Closed
wants to merge 1 commit into from

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented Aug 6, 2018

Add a missing regression test for connectAndInspect with empty array of
interfaces.

@aqrln aqrln added the test label Aug 6, 2018
@aqrln aqrln requested a review from nechaido August 6, 2018 15:55
Copy link
Member

@nechaido nechaido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Though, should we check that no inspect message is sent?

@aqrln
Copy link
Member Author

aqrln commented Aug 7, 2018

@nechaido PTAL

server = jstp.net.createServer([app]);

server.on('connect', (connection) => {
connection.on('incomingMessage', (message) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be better to use server's log event here:

server.on('log', (connection, event) => {
  if (event === 'inspect') {
    inspectSent = true;
  }
});

@aqrln
Copy link
Member Author

aqrln commented Aug 7, 2018

@nechaido @belochub I've pushed new commits to address the feedback, PTAL

Copy link
Member

@nechaido nechaido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit.
Also there is no need in subtest in this case, so the test may be written this way:

'use strict';
const test = require('tap');
const jstp = require('../..')

const app = new jstp.Application('app', {});
const server = jstp.net.createServer([app]);
server.on('log', (connection, event) => {
  if (event === 'inspect') {
    test.fail('must not send inspect messages');
  }
});

server.listen(0, () => {
  test.plan(4);
  jstp.net.connectAndInspect(
    app.name,
    null,
    [],
    server.address().port,
    (error, connection, api) => {
      test.pass('must invoke the callback');
      test.assertNot(error, 'must connect to server');
      test.equal(
        Object.keys(api).length,
        0,
        'remote proxy object must be empty'
      );
      test.assertNot(inspectSent, 'must not send inspect message');
      connection.close();
      server.close();
    }
  );
});

test.equal(
Object.keys(api).length,
0,
'remote proxy object must be empty'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be misleading, as api is an object that contains RemoteProxy, but remote proxy object may be interpreted as 'instance of RemoteProxy'.

@aqrln
Copy link
Member Author

aqrln commented Aug 7, 2018

@nechaido @belochub please take another look

Copy link
Member

@nechaido nechaido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aqrln
Copy link
Member Author

aqrln commented Aug 7, 2018

Unrelated macOS failure:

not ok 30 - test/node/resendable-call-from-client.js # time=241094.551ms
  ---
  timeout: 240000
  file: test/node/resendable-call-from-client.js
  command: /home/travis/.nvm/versions/node/v6.14.3/bin/node
  args:
    - test/node/resendable-call-from-client.js
  stdio:
    - 0
    - pipe
    - 2
  cwd: /home/travis/build/metarhia/jstp
  ...
{
    ok 1 - must connect without errors
    not ok 2 - timeout!
      ---
      signal: SIGTERM
      handles:
        - type: ChildProcess
          events:
            - internalMessage
            - message
        - type: Pipe
        - type: Socket
          events:
            - end
            - finish
            - _socketEnd
            - data
            - error
            - close
      expired: TAP
      stack: |
        emit (node_modules/nyc/node_modules/signal-exit/index.js:77:11)
        process.listener (node_modules/nyc/node_modules/signal-exit/index.js:91:7)
        processEmit (node_modules/nyc/node_modules/signal-exit/index.js:155:32)
        processEmit (node_modules/signal-exit/index.js:155:32)
      test: TAP
      ...
    
    1..2
    # failed 1 of 2 tests
    # time=185627.749ms
    
    not ok 3 - timeout!
      ---
      expired: test/node/resendable-call-from-client.js
      ...
}

Add a missing regression test for connectAndInspect with empty array of
interfaces.
@aqrln aqrln added the client label Aug 7, 2018
belochub pushed a commit that referenced this pull request Aug 8, 2018
Add a missing regression test for connectAndInspect with empty array of
interfaces.

PR-URL: #359
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
@belochub
Copy link
Member

belochub commented Aug 8, 2018

Landed in b18215b.

@belochub belochub closed this Aug 8, 2018
@belochub belochub deleted the test-regress-329 branch August 8, 2018 10:19
belochub pushed a commit that referenced this pull request Aug 30, 2018
Add a missing regression test for connectAndInspect with empty array of
interfaces.

PR-URL: #359
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
@belochub belochub mentioned this pull request Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants