-
Notifications
You must be signed in to change notification settings - Fork 52
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
Start adding tests for MSC4140 #734
Conversation
as it is currently unused
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.
Great work so far! Tests are clear and easy to read.
deployment.StopServer(t, hsName) | ||
time.Sleep(1 * time.Second) | ||
deployment.StartServer(t, hsName) |
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 worry that Synapse may not always start in 10s, especially when running with a ton of workers. See the list of workers we run in the Synapse repo's Complement CI.
Unfortunately it doesn't look like we're testing with Synapse workers in Complement's CI. Have you tried running with workers locally? Otherwise, perhaps we can just merge these tests and see how they do on the Synapse PR's CI?
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've done some light local testing with workers, though not in Complement runs.
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.
Let's merge it and see, I suppose. We can always bump this timeout as a quick fix before converting it to a unit test, if necessary.
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.
This LGTM! Thanks for addressing the changes.
deployment.StopServer(t, hsName) | ||
time.Sleep(1 * time.Second) | ||
deployment.StartServer(t, hsName) |
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.
Let's merge it and see, I suppose. We can always bump this timeout as a quick fix before converting it to a unit test, if necessary.
Signed-off-by: Andrew Ferrazzutti [email protected]
Pull Request Checklist