STITCH-3383 add client sdk FunctionCredential #329
Conversation
mburdette
left a comment
mburdette
left a comment
There was a problem hiding this comment.
Looks good, just some nits really
| CustomProviderConfig, | ||
| CustomProvider | ||
| CustomProvider, | ||
| FunctionProviderConfig, |
There was a problem hiding this comment.
[nit/questions] I think in other places we are calling this CustomFunction not sure if its worth making the change here as well?
| CustomProviderConfig, | ||
| CustomProviderConfig, | ||
| FunctionProvider, | ||
| FunctionProviderConfig, |
| CustomProviderConfig, | ||
| CustomProvider, | ||
| FunctionProvider, | ||
| FunctionProviderConfig, |
| expect(FunctionAuthProvider.TYPE).toEqual(user.identities[0].providerType); | ||
| expect(client.auth.isLoggedIn).toBeTruthy(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
can we also add some fail tests as well as tests that actually use whats in the payload?
There was a problem hiding this comment.
We check the contents of the payload– what do you mean "use what's in the payload"?
There was a problem hiding this comment.
in your function, use something from the payload, to verify that you are sending it correctly and is usable...your function right now just returns "foo" so for all I know the sdk isn't sending the function credential payload correctly...
| expect(client.auth.listUsers().length).toBe(2); | ||
| }); | ||
|
|
||
| it("should authenticate using a custom function", async () => { |
There was a problem hiding this comment.
can we also add some fail tests as well as tests that actually use whats in the payload?
|
|
||
| expect(user.id).toBeDefined(); | ||
| expect(user.identities[0].id).toBeDefined(); | ||
| expect("foo").toEqual(user.identities[0].id); |
There was a problem hiding this comment.
[nit] if you gonna use a string in two places use a const
|
|
||
| expect(user.id).toBeDefined(); | ||
| expect(user.identities[0].id).toBeDefined(); | ||
| expect("foo").toEqual(user.identities[0].id); |
There was a problem hiding this comment.
[nit] if you gonna use a string in two places use a const
|
|
||
| expect(user.id).toBeDefined(); | ||
| expect(user.identities[0].id).toBeDefined(); | ||
| expect("foo").toEqual(user.identities[0].id); |
There was a problem hiding this comment.
[nit] if you use a string in two places use a const
| expect(FunctionAuthProvider.TYPE).toEqual(user.identities[0].providerType); | ||
| expect(client.auth.isLoggedIn).toBeTruthy(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
can we also add some fail tests as well as tests that actually use whats in the payload?
There was a problem hiding this comment.
I'll add a fail test, but it will only be to assert that we catch the error properly. Other testing should be done serverside.
There was a problem hiding this comment.
the fail test should be testing that login fails when the functions returns something that isn't a string or it throws an error. "catch the error" has nothing to do with it. The point of the test is to make sure that something bad doesn't in your sdk code, when login fails. Obviously the backend is testing this as well.
|
@mburdette PTAL |
mburdette
left a comment
mburdette
left a comment
There was a problem hiding this comment.
LGTM thanks for getting this done
3 participants
No description provided.