test: migrate inspect and remote proxy tests to tap #198
Conversation
test/node/connection-inspect.js
Outdated
| test.assert(error, 'must return an error'); | ||
| test.equal(error.code, jstp.ERR_INTERFACE_NOT_FOUND, | ||
| 'error must be an ERR_INTERFACE_NOT_FOUND' | ||
| ); |
There was a problem hiding this comment.
Maybe merge this line with the previous one?
|
Blocked until #190. |
|
Still LGTM. |
|
Oh, only one more thing: can please test |
test/fixtures/application.js
Outdated
| }, | ||
| someOtherService: { | ||
| method(connection, argument, callback) { | ||
| callback(argument); |
There was a problem hiding this comment.
Is returning argument as an error intentional here?
There was a problem hiding this comment.
It's stub that is never called, but I'll fix it to behave properly if called
test/node/connection-inspect.js
Outdated
| const expectedInterfaces = Object.keys(app.interfaces); | ||
| let expectedTests = 1; | ||
| expectedInterfaces.forEach((iface) => { | ||
| expectedTests += 1; |
There was a problem hiding this comment.
May you merge this with the next line? Like this:
expectedTests += Object.keys(app.interfaces[iface]).length + 1;There was a problem hiding this comment.
I'd like to leave it this way because it "separates" check count for interfaces and methods.
There was a problem hiding this comment.
@nechaido, if that's the case, may you add a comment explaining this, to avoid confusion in the future?
Also, why not just add expectedInterfaces.length to expectedTests immediately after initialization, or even initialize expectedTests with expectedInterfaces.length + 1?
There was a problem hiding this comment.
@belochub I though It'll be more obvious, seems It's not, also I'm strongly opposed to leaving a comment. It should be written in a more obvious way that needs no explanation. Maybe I'll stop on my initial idea: also proposed by you:
let expectedTests = 1 + expectedInterfaces.length;
expectedTests += expectedInterfaces.reduce((methodsCount, iface) => {
methodsCount += Object.keys(app.interfaces[iface]).length;
});
Maybe I should merge this into one expression.
@metarhia/jstp do you have any ideas concerning this issue?
There was a problem hiding this comment.
Or even:
let expectedTests = 1 + expectedInterfaces.reduce((tests, iface) =>
tests + 1 + Object.keys(app.interfaces[iface]).length);
There was a problem hiding this comment.
The latter looks better to me, but I think it's better to add a comment explaining this.
test/node/connection-inspect.js
Outdated
| expectedInterfaces.forEach((iface) => { | ||
| connection.inspectInterface(iface, (error, methods) => { | ||
| test.assertNot(error, `must inspect ${iface}`); | ||
| Object.keys(app.interfaces[iface]).forEach((method) => { |
There was a problem hiding this comment.
You may avoid using braces and parenthesis here, there is just one statement in this function.
test/node/connection-inspect.js
Outdated
|
|
||
| expectedInterfaces.forEach((iface) => { | ||
| test.assert(iface in api, `api must include '${iface}'`); | ||
| Object.keys(app.interfaces[iface]).forEach((method) => { |
|
@aqrln added. |
test/node/connection-inspect.js
Outdated
| ); | ||
| }); | ||
|
|
||
| test.test('must throw error if interface does not exist', (test) => { |
There was a problem hiding this comment.
It doesn't really throw, it just returns an error.
|
Landed in 6be836e. |
Refs: metarhia/jstp#145 Refs: metarhia/jstp#146 Refs: metarhia/jstp#53 PR-URL: metarhia/jstp#198 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Refs: metarhia/jstp#145 Refs: metarhia/jstp#146 Refs: metarhia/jstp#53 PR-URL: metarhia/jstp#198 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Refs: #145
Refs: #146
Refs: #53.