-
Notifications
You must be signed in to change notification settings - Fork 3
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
Allow concurrent use of welkin clients #67
Conversation
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.
All LGTM - appreciate this change. Only thing is if you could figure out why that test is failing?
I can't reproduce the failing test anymore after hundreds of test runs; looks like this is safe to merge! |
Great, thank you @jdiaz5513 ! |
@jdiaz5513 can you run |
@edcohen08 sorry that took me a bit; let's see if that fixes the packaging! |
|
is this good to merge? it looks like the changes are pretty small |
Yep, I think it should be. Just needs a review and approval. |
Without this patch any attempt to use the Welkin client concurrently will result in a file access error.
This is because of the temporary file created for auth: the
shelf
library makes no attempt to lock the file beforehand and cannot handle concurrent write access.I added portalocker as a cross-platform way to obtain a lock (on a separate temp file) while accessing that DB file.
I've also added pytest-xdist to make sure the tests do not fail when run in parallel (they did before!).
There is one newly skipped test that fails intermittently (~1 out of 20 times). I'm not sure what to do about it, or if it is even safe to skip. We're trying to add parallelism to our own test suite so that particular issue may come to bite us anyway. It is interesting that it's only happening to a single test.