feat(main): RPC Router and Server implementation #1
Conversation
kjdelisle
force-pushed
the
initial-implementation
branch
from
411a53b
to
228fe56
Compare
| expect(controller.basicHello({})).to.equal('Hello, World!'); | ||
| }); | ||
|
|
||
| it('returns greetings for the world without valid input', () => { |
There was a problem hiding this comment.
it('returns greetings with valid input, ...
| it('routes to existing controller and method', async () => { | ||
| await router.routeRequest(request, response); | ||
| const stub = response.send as sinon.SinonStub; | ||
| expect(stub.called); |
There was a problem hiding this comment.
Have you seen this assertion to fail? I believe it's a no-op:
> expect(false)
Assertion {
obj: false,
anyOne: false,
negate: false,
params: { actual: false } }
I think you wanted to say expect(stub.called).to.be.true() but please don't do that - the assertion failure will be rather unhelpful. Use sinon assertions instead:
sinon.assert.called(stub);We tried to integrate should with sinon via https://github.com/shouldjs/sinon, but it did not work :(
There was a problem hiding this comment.
Please fix other places using expect(stub.called); too
|
|
||
| function getRequest(req?: Partial<express.Request>) { | ||
| const reqt = Object.assign( | ||
| <express.Request>{ |
There was a problem hiding this comment.
Uh oh, this is hacky and may easily break when express starts to require on another request property that's not mocked out here, or calls any of the ReadableStream APIs. Have you considered using Shot instead? We are shipping it as a part of our testlab, see packages/testlab/src/shot.ts
There was a problem hiding this comment.
@bajtos Would you be able to provide an example of how to use our testlab exports of Shot? I've noticed that we're not actually using it the way Shot's Github README advertises, and I'm not certain how I would use it to mock the handling of express requests and responses (given that it's for http.ServerRequest and http.ServerResponse).
I understand that the hard cast here could break if something leverages a function or property that I haven't faked, but I've gone out of my way to make sure that nothing calls functions on the request or response object in the rpc-router code above and beyond what's faked here.
There was a problem hiding this comment.
Technically speaking, no instance of an express server ever begins listening on an external port; these tests are essentially integration tests.
There was a problem hiding this comment.
Would you be able to provide an example of how to use our testlab exports of Shot?
Have you tried to search for the places in loopback-next that are using it? There are quire few test files leveraging mockResponse, see https://github.com/strongloop/loopback-next/search?utf8=%E2%9C%93&q=shot&type=
Looks like we need to improve the docs for this part of testlab! I created an issue - see loopbackio/loopback-next#760
| } | ||
|
|
||
| function getResponse(res?: Partial<express.Response>) { | ||
| const resp = <express.Response>{}; |
There was a problem hiding this comment.
Ditto here, please consider using Shot's Response mock instead.
connected to loopbackio/loopback.io#523
Currently no tests in place for RPC Server (router has most of the actual logic)