refactor: recover tests for auth action#2859
Conversation
b-admike
left a comment
b-admike
left a comment
There was a problem hiding this comment.
I have two comments, otherwise LGTM overall.
| @@ -1,208 +0,0 @@ | |||
| // Copyright IBM Corp. 2019. All Rights Reserved. | |||
There was a problem hiding this comment.
Are we removing these tests because they are already covered/moved to some other place (Perhaps @emonddr's PR had them)?
There was a problem hiding this comment.
@b-admike These tests are written using a passport based strategy + passport adapter. While the old adapter will be deprecated and replaced by a new one to be created in a new module, so acceptance test for the passport strategy should be rewritten in that coming new module.
The new auth sysmte that Dom created is built with the extension point, so that the new acceptance tests also reflect that change:
see https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/__tests__/acceptance/jwt-auth-extension.acceptance.ts
and https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/__tests__/acceptance/basic-auth-extension.acceptance.ts
Are we removing these tests because they are already covered/moved to some other place
So to answer your question, yes or no, the acceptance tests for the new auth system is covered. The new adapter and its corresponding acceptance tests haven't created. The file you are looking at is removed because of the old adapter doesn't work with the new system anymore.
There was a problem hiding this comment.
Makes sense, thanks for the in-depth explanation @jannyHou :-).
packages/authentication/src/__tests__/unit/fixtures/mock-strategy-passport.ts
Show resolved
Hide resolved
b-admike
left a comment
b-admike
left a comment
There was a problem hiding this comment.
Thanks for answering my questions, LGTM 👍
| async authenticate(req: Request, options?: AuthenticateOptions) { | ||
| calledFlag = true; | ||
| await MockStrategy.prototype.authenticate.call(this, req, options); | ||
| await MockPassportStrategy.prototype.authenticate.call( |
There was a problem hiding this comment.
Please use await super.authenticate(...) instead.
jannyHou
force-pushed
the
refactor/passport-adapter
branch
3 times, most recently
from
411821e to
2ff383b
Compare
| async authenticate(req: Request, options?: AuthenticateOptions) { | ||
| calledFlag = true; | ||
| await MockStrategy.prototype.authenticate.call(this, req, options); | ||
| await super.authenticate.call(this, req, options); |
There was a problem hiding this comment.
Just super.authenticate(req, options);.
jannyHou
force-pushed
the
refactor/passport-adapter
branch
2 times, most recently
from
8988474 to
4758034
Compare
jannyHou
force-pushed
the
refactor/passport-adapter
branch
from
4758034 to
4031220
Compare
|
Great job, @jannyHou |
5 participants
Description
Fixes #2467
See discussion in the draft PR #2822.
And since a new package will be created for the new adapter(s), I will work on the spike #2676 (comment) first, then create the adapter's package in
loopback-labs.