-
Notifications
You must be signed in to change notification settings - Fork 2
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
test: removes e2e tests in favor of the sdk-test suite #131
Conversation
assert.Equal(t, "errorCode: invalid_request", splitError[1]) | ||
assert.Equal(t, "statusCode: 400", splitError[2]) | ||
} | ||
|
||
// should be run with the -race flag, i.e. `go test -race -run TestAppJWKSCacheWriteConcurrency` | ||
func TestAppJWKSCacheWriteConcurrency(t *testing.T) { |
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.
thought it would be better to leave this test in this repo since the issue was raised here and it's easier to understand the regression at this level
@@ -88,7 +19,7 @@ func TestAppJWKSCacheWriteConcurrency(t *testing.T) { | |||
go func() { | |||
defer wg.Done() | |||
|
|||
_, err := passage.New(PassageAppID, PassageApiKey) | |||
_, err := passage.New("passage", "some-api-key") |
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.
figured this is the safest and easiest way to keep this test functional
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.
Does this test require the app ID to be valid? If not, maybe having "some-app-id"
as the placeholder could make that extra clear.
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.
the app ID must be valid so it can reach out and cache the JWKS
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.
👍 makes sense.
Is this initialization behavior unique to this SDK?
It might be helpful to add a comment here noting that a real network request will be made with the app ID to cache the JWKs.
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.
no, all SDKs try to get the JWKs immediately upon initialization. added a comment in 387b088
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.
Left a suggestion but overall LGTM.
|
What's New?
passage
appScreenshots (if appropriate):
Type of change
Checklist:
Additional context