-
Notifications
You must be signed in to change notification settings - Fork 569
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
fix: test using apps #1130
base: 10.1
Are you sure you want to change the base?
fix: test using apps #1130
Conversation
@@ -226,7 +229,7 @@ public void shouldProcessBulkImportUsersInLargeNumbersInTheSameTenant() throws E | |||
List<BulkImportUser> users = generateBulkImportUser(usersCount); | |||
BulkImport.addUsers(appIdentifier, storage, users); | |||
|
|||
waitForProcessingWithTimeout(appIdentifier, storage, 300); // 5 minutes | |||
waitForProcessingWithTimeout(appIdentifier, storage, 600); // 10 minutes |
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.
why did you increase the wait time here? GHA runs these slower?
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.
yea, giving more time for the GHA to succeed
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.
I played around with your branch and reviewed the changes that are related to the core test running (so no GHA, as those are seemingly changing at the moment of writing this), the conclusion is: I like the idea and agree with the implementation. I didn't catch any mistake by review, however, when running tests locally I have failing tests. I suspect that's on my docker env, but I'm not entirely sure.
(The symptoms are mostly SocketReadTimeOut
, as if the communication with the container had been halted or something.
Please ask at least one more dev to test these changes on the local machine both with startTestEnv
and with idea
(or any other IDE he is using).
Summary of change
(A few sentences about this PR)
Related issues
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your
changes work. Bonus points for screenshots and videos!)
Documentation changes
(If relevant, please create a PR in our docs repo, or create a checklist here
highlighting the necessary changes)
Checklist for important updates
coreDriverInterfaceSupported.json
file has been updated (if needed)pluginInterfaceSupported.json
file has been updated (if needed)build.gradle
getPaidFeatureStats
function in FeatureFlag.java filebuild.gradle
, please make sure to add themin
implementationDependencies.json
.getValidFields
inio/supertokens/config/CoreConfig.java
if new aliases were added for any coreconfig (similar to the
access_token_signing_key_update_interval
config alias).git tag
) in the formatvX.Y.Z
, and then find thelatest branch (
git branch --all
) whoseX.Y
is greater than the latest released tag.app_id_to_user_id
table, make sure to delete from this table when deletingthe user as well if
deleteUserIdMappingToo
is false.Remaining TODOs for this PR