nest
nest copied to clipboard
Allow creating a testing handler for HTTP/RPC/WS controllers
PR Checklist
Please check if your PR fulfills the following requirements:
- [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
PR Type
What kind of change does this PR introduce? See #3938
[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
[ ] Yes
[ ] No
Other information
Pull Request Test Coverage Report for Build 4e598554-7b3c-4414-85b4-3e7323b07c5f
- 0 of 0 changed or added relevant lines in 0 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage remained the same at 94.575%
Totals | |
---|---|
Change from base Build c31de5ab-d6ea-4463-9c86-599e16909615: | 0.0% |
Covered Lines: | 5300 |
Relevant Lines: | 5604 |
💛 - Coveralls
Looks great so far!
Thanks @kamilmysliwiec. I did the same work for RPC and WebSocket. Please let me know what could be improved!
I thought about adding a single public static method to Test, which would accept the same additional parameter as createContext
('http' | 'rpc' | 'ws'
), in order to chose the Testing Handler to be created. This would allow doing something like this:
await Test
.createTestingHandler({ class, methodName, protocol: 'http' })`
.setRequest(fakedRequest)
.run();
The Return type of createTestingHandler
would obviously depend on protocol
(either HttpTestingHandler
, RpcTestingHandler
, or WsTestingHandler
). What do you think?
I'll review as soon as possible. Thanks @VinceOPS!
Hey @kamilmysliwiec, I had a look into ExternalContextCreator
and thought about creating the context using the static method fromContainer
. However, it looks like it would (in the end) require a Module in order for the resolution of dependencies to work. Shall I change the API so it takes a Module (or Testing module) as a parameter?
Thanks
This looks great, thanks!
I'm sorry to keep you waiting @VinceOPS! The PR looks fantastic :fire:
ExternalContextCreator
is an injectable class that can be injected anywhere you want https://github.com/nestjs/graphql/blob/master/lib/services/resolvers-explorer.service.ts#L42 (inside the Nest ctx app though).
thought about creating the context using the static method fromContainer
This is somewhat more complicated though. The fromContainer
expects the NestContainer
instance which in turn is instantiated by the NestFactory
before the initialization process.
Perhaps, we should take this idea even further and consider the following use-case - what if someone wants to test the handler in isolation (without spinning up the HTTP server) BUT wants to have all the enhancers bound with { provide: APP_INTERCEPTOR ...etc.. }
available as well?
This wouldn't be possible with the current API, so it doesn't really cover all the use-cases.
What if changed this:
await Test
.createTestingHandler({ class, methodName, protocol: 'http' })`
.setRequest(fakedRequest)
.run();
to this:
const moduleRef = await Test.createTestingModule({
imports: [....] // whatever
}).compile() // classic approach
await moduleRef
.createTestingHandler({ class, methodName, protocol: 'http' })`
.setRequest(fakedRequest)
.run();
The TestingModule
(moduleRef
) extends the NestApplicationContext
which means that you can easily grab the ExternalContextCreator
using the this.get(ExternalContextCreator)
within the TestingModule
class :)
TL;DR; We could move the createTestingHandler
method into the TestingModule
where you can grab this ExternalContextCreator
from the context instead of manually creating it
Thank you for the explanations @kamilmysliwiec :+1:! I'm gonna spend some time on this ASAP. I like the idea of exposing this API via TestingModule :fire:.
Hi @kamilmysliwiec
I moved the code to TestingModule
and did set up the context creation. The basic (expected) behavior is good! You can have a look at the 3 test suites.
This it not complete, yet. I want to:
- Refactor the way the context are created. All this new code is making a lot of "noise" in
testing-module.ts
(any suggestion most welcome). I'm not done yet thinking about it (Are the HTTP/RPC/WS builders still relevant? Where external contexts should be created? etc). - As you already know and said, the way I'm passing the
instance
toExternalContextCreator
is not going to work with request-scoped instances - I made some nasty type-castings here and there :see_no_evil: #WIP
- I am still not able to use
@Payload
,@Ctx
(RPC) and@MessageBody
(WS)... It looks like they are not "injected".
At this point, I feel like your insights would be pretty helpful :grimacing: ! Thanks :pray:
@VinceOPS
I am still not able to use @Payload, @Ctx (RPC) and @MessageBody (WS)... It looks like they are not "injected".
Can you create tests for this? I'll fix this issue locally then :)
As you already know and said, the way I'm passing the instance to ExternalContextCreator is not going to work with request-scoped instances
Please, pull the latest version of @nestjs/{...}
packages. I've added a few additional features that should simplify this process. See comments (I'll add them in a second)
@kamilmysliwiec Heya Kamil! Guess who's back :ghost: :joy:
I know it's been a long time and you probably forgot everything about this PR :sob: :gun: ... Just in case you would find some time for this possible feature, I pushed the test case I could never pass! allows message body to be injected using decorator @MessageBody
in ws-testing-handler.spec.ts
. It's too bad, because it looks like everything else is working.
There are refactorings that I'm holding for now (e.g. almost duplicated createContext
functions in testing-module.ts
), but I think it's good enough for you to have a look at the issue of the request-scoped instance (cf the 3 occurences of // TODO: Kamil's gonna do something here, for Request-scoped instances
:grimacing:).
Please let me know if I can help :pray:
It seems that WsParamsFactory
retrieves undefined
value as input arguments (args === undefined
) and so it returns null
. I don't have too much time atm to investigate it further though. Will take a look asap
Hi @kamilmysliwiec! I had a look into it tonight and it looks like the callback created by createPipesFn
(fnApplyPipes
) indeed provides null
as data
to my websocket gateway method 😕
@WebSocketGateway()
class EventsGateway {
@SubscribeMessage('identity')
async identity(@MessageBody() data: string): Promise<string> {
return data;
}
}
There's one thing bugging me here... As far as I understood, MessageBody
here is supposed to add a WS Param type PAYLOAD
(3
) to my parameter... but somewhat, in the process, this is lost OR something does not care? 😂
I don't know the "context creator" enough to understand.
Hitting a breakpoint on this line (ExternalContextCreator.createPipesFn
):
args[index] = await this.getParamValue(
value,
{ metatype, type, data },
pipes.concat(paramPipes),
);
I can see that paramsOptions
is an Array(1)
with the item inside being:
{
index: 0,
extractValue: (...args) => paramsFactory.exchangeKeyForValue(numericType, data, args),
type: 3,
data: undefined,
pipes: [],
metatype: function String() { [native code] },
}
paramsOptions[0].metatype
is ''
, and paramsOptions[0].extractValue()
is null
.
Regarding this line (just before):
const value = extractValue(...params);
value
is obviously null
and params
is [undefined, "Hello World"]
.
Does it help? 🤯
Thanks!
I've looked into this issue once again and I can see that WsParamsFactory#exchangeKeyForValue
receives undefined
as args because ExternalContextCreator
passes them as a third argument, while WsParamsFactory
only takes 2:
So in fact, data
(which is undefined
) = our args
array.
Funny enough, this has changed in 8.0.0 so once you merge 8.0.0
branch in, the issue will be fixed (just tested) :)
One thing to keep in mind - remember to import from @nestjs/{..}
while referencing other packages:
(testing-module.ts above^)
Sounds great 😍
Gonna get back to it ASAP 😄! Thanks! I will keep you posted.
Thank you @kamilmysliwiec 🙏🏼 ! It's indeed working when rebased on branch 8.0.0 🎉
I have troubles using Pipes (test executes Pipes passed as classes in @Query
) when passing a class and not an instance (it's working with an instance 👍🏼). I assume this is just me missing something. I tried to add the pipe class to my providers
but it didn't change anything. Will have a second look at it ASAP.
@VinceOPS There might be several different reasons why this is happening. Maybe check if this line:
const module = this.getContextModuleName(instance.constructor);
in ExternalContextCreator#create
gives you any reasonable output (module
is required to retrieve pipe instances later on in the PipesContextCreator
).
Any news on this ?
@VinceOPS What's the status of this?
Hi @kamilmysliwiec, I'd like to continue developing this feature, but I'd need some help and clarification to get started
Are those changes still relevant for nestjs 10? What are the next steps?