-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Allow cosmos tests to run in parallel #37696
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
Closed
Closed
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
639d44b
Allow cosmos tests to run in parallel
JoasE 27cd846
Always use guid for emulator
JoasE 8cd4e79
Simplify & add cleanup
JoasE db16355
Northwind
JoasE 6306e48
Add some docs and add condition for flaky test
JoasE b6ec888
Do not share static instance across tests, fix other concurrent creat…
JoasE 2b88215
Merge branch 'main' of https://github.com/dotnet/efcore into feature/…
JoasE 5f2cd56
Remove local skip on locale
JoasE File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@AndriySvyryd do you think we can get rid of the GUID-named containers? I'm not sure what emulator limitations make this necessary and why we need to delete etc.
Uh oh!
There was an error while loading. Please reload this page.
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.
Its documented that the emulator will show performance degradation with >10 containers in this statement in the docs
Which I think is why its deleting the containers after every test.
Because different test classes might use the same test store, using a guid was a quick fix to allow parallel execution (as we otherwise will start deleting something while it's still being used.) There might be a better way to do this, or the performance of >10 containers isn't that bad (or is more about >10 active containers). I'm still looking into that.
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.
Ah thanks, I missed that note. I'm guessing that's also a good reason to not parallelize (or at least to keep the degree of parallelization down)...
But I'm still not sure why it's useful to include a random GUID inside the container name. That feels more like a practice for when a shared cloud database is used potentially in parallel, to ensure you don't have conflicts - not relevant for the emulator.
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.
@roji
The GUID guarantees that parallel execution becomes safe by ensuring every test gets its own isolated container.
Since different test classes might use the same fixture and therefore the same test store name, conflicts will occur with parallel test execution. The tests sharing fixtures are (usually, or as far as I saw until now) read only tests, but since we are deleting the containers one test might finish and delete the container while another is still using it. Using a GUID here is a quick fix, where another option would be to group these tests and create a collection fixture, tying the container lifetime to the collection fixture, but that might require significant changes in the setup with the specifications test or throughout the entire cosmos tests project. Which I am not sure is worth the effort at this moment as I am still trying to determine whether running tests in parallel is actually feasible with the emulator.
It currently seems to be stable locally with no limit on the parallelism (which would mean processor count, which is 16 for my laptop), and no significant performance increment compared to a limit of 3 threads. With by stable I mean running the complete test suite 15 times with no errors.
If I don't delete the shared containers the emulator will stop responding to create collection requests at some point, around 20 databases with each 1-3 containers.
I haven't tested this against a real cosmos db tho, and enabling multi threading is not conditional, so I still have to do that. Maybe I could handle the parallelism myself with semaphores in the test store completely, but it might be kinda funky, also affecting test run times for tests that don't use a fixture but call
InitializeAsyncin the test.I just want to mention that this is still an experiment, and I’m not sure yet whether it will succeed or if I’ll even complete it. Please don’t feel like you need to limit your questions though, I really appreciate the input and enjoy discussing it. I just don’t want you to feel like you have to spend time on it, unless you are ok with that!