-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
addresses #1247 #1277
addresses #1247 #1277
Conversation
Signed-off-by: Michael Wagler <[email protected]>
Signed-off-by: Michael Wagler <[email protected]>
Signed-off-by: Michael Wagler <[email protected]>
@aeneasr 👀when you get a chance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! To get tests working run:
$ export GO111MODULE=on
$ go get -u golang.org/x/crypto
$ git commit -a -m "vendor: Update crypto dependency"
$ git push origin
oauth2/fosite_store_helpers.go
Outdated
require.NoError(t, err) | ||
|
||
signature := uuid.New() | ||
err = m.F.CreateAccessTokenSession(ctx, signature, createTestRequest(signature)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to test this against all methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aeneasr sounds good. I just added a whole bunch of tests, let me know if you have suggestions.
There's a lot of repetition so I wrote helpers that take the methods being tested as parameters. I think it mostly works, but there are a couple things that are a bit ugly:
-For the Access token and Refresh token tests, I call doTestCommit
and do doTestRollback
twice each, to account for them having both a revoke and delete method
-The OpenIdConnectSession
tests couldn't use the helper functions, since GetOpenIDConnectSession
has a different signature than all the other getter methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Signed-off-by: Michael Wagler <[email protected]>
…Session to use transaction (ory#1247) Signed-off-by: Michael Wagler <[email protected]>
…ting tests (ory#1247) Signed-off-by: Michael Wagler <[email protected]>
Thank you, Michael! |
#1247