Skip to content
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.

Sign up for GitHub

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

Jump to bottom

#1014 storage fixtures #1054

Merged
merged 7 commits into from
Nov 28, 2023
Merged

#1014 storage fixtures #1054

merged 7 commits into from
Nov 28, 2023

Conversation

qc00
Copy link
Contributor

@qc00 qc00 commented Nov 13, 2023

Reference Issues/PRs

#1014

What does this implement or fix?

Introducing the StorageFixtures framework

  • Standard, not pytest-coupled API for storage fixture life cycles
  • Methods for setting permissions and shutting down the fixtures independent of pytest for error handling testing
  • Proper cleanup and exception handling to allow tests to run concurrently and independently of earlier failures
  • PrefixingLibraryAdapterDecorator to support isolating Mongo fixture instances, but can be used with any other Storage type

@qc00 qc00 added the refactor label Nov 13, 2023
@qc00 qc00 force-pushed the 1014-storage-fixtures branch 7 times, most recently from eca0d3b to 8c0632c Compare November 14, 2023 04:30
@qc00 qc00 marked this pull request as ready for review November 14, 2023 15:37
@qc00 qc00 force-pushed the 1014-storage-fixtures branch 4 times, most recently from a8a4750 to bc52811 Compare November 15, 2023 09:39
@qc00 qc00 changed the title 1014 storage fixtures #1014 storage fixtures Nov 15, 2023
@qc00 qc00 force-pushed the 1014-storage-fixtures branch 4 times, most recently from 38c856e to 314f20c Compare November 15, 2023 14:42
@poodlewars
Copy link
Collaborator

Would be nice to add some evidence (sorted output of pytest discovery before and after and a diff, maybe?) to demonstrate that the test cases we execute are similar before and after this change.

@qc00
Copy link
Contributor Author

qc00 commented Nov 17, 2023

@poodlewars There's no change in this PR on what storage the tests will run against; that's in the next PR.

This PR is mainly moving the same fixtures to a new API framework and using the API in tests.

poodlewars
poodlewars reviewed Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@poodlewars poodlewars left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--WIP review: I haven't read the diff in the tests/ tree yet.--

Done now.

python/arcticdb/arctic.py Outdated Show resolved Hide resolved
python/arcticdb/arctic.py Outdated Show resolved Hide resolved
python/arcticdb/storage_fixtures/azure.py Outdated Show resolved Hide resolved
python/arcticdb/storage_fixtures/azurite_patching.py Outdated Show resolved Hide resolved
python/arcticdb/storage_fixtures/azurite_patching.py Outdated Show resolved Hide resolved
python/arcticdb/storage_fixtures/s3.py Show resolved Hide resolved
python/arcticdb/storage_fixtures/s3.py Show resolved Hide resolved
python/arcticdb/storage_fixtures/s3.py Outdated Show resolved Hide resolved
python/arcticdb/storage_fixtures/utils.py Show resolved Hide resolved
python/arcticdb/storage_fixtures/utils.py Show resolved Hide resolved
poodlewars
poodlewars reviewed Nov 20, 2023
View reviewed changes
python/arcticdb/arctic.py Show resolved Hide resolved
python/tests/conftest.py Show resolved Hide resolved
python/tests/conftest.py Show resolved Hide resolved
python/tests/conftest.py Show resolved Hide resolved
python/tests/conftest.py Show resolved Hide resolved
python/tests/integration/arcticdb/test_arctic.py Show resolved Hide resolved
python/tests/scripts/test_update_storage.py Show resolved Hide resolved
python/tests/scripts/test_update_storage.py Show resolved Hide resolved
python/tests/scripts/test_update_storage.py Show resolved Hide resolved
python/tests/scripts/test_update_storage.py Show resolved Hide resolved
@poodlewars
Copy link
Collaborator

@poodlewars There's no change in this PR on what storage the tests will run against; that's in the next PR.

This PR is mainly moving the same fixtures to a new API framework and using the API in tests.

I've found at least a few tests deleted in this PR, so I do think that some comparison would be helpful.

@poodlewars
Copy link
Collaborator

poodlewars commented Nov 20, 2023 •
edited
Loading

Thanks for this, overall this looks like a big big cleanup to some tech debt that has been accruing for a long time. It's really nice seeing the cleaned up form taking shape. To summarize my main concerns,

  • The complexity of the Azurite patching script. I think we should ideally contribute the fix upstream. If we can't, I think that the complexity is possibly not worth it for the increased test coverage it can buy us,
  • The complexity of the PrefixingLibraryAdapterDecorator. Happy to see what a second reviewer thinks about this one.
  • The removed tests, and if there are any other removed tests that I've missed. Hopefully some pytest discovery output can put my mind at rest about this.

My other comments are much more minor and localized.

@qc00
Copy link
Contributor Author

qc00 commented Nov 21, 2023 •
edited
Loading

The removed tests, and if there are any other removed tests that I've missed. Hopefully some pytest discovery output can put my mind at rest about this.

Please refer to the files at https://arcticdb.slack.com/archives/C0553TEJ63F/p1700586140886959

Both files are obtained using pytest --co. after.txt has these string replacements to match the fixture naming of the original:

Regex: \[(s3|lmdb|mem)- => \U$0
[azurite- => [Azure-
[mongo- => [Mongo-
[real_s3- => [Real_S3-

The only meaningful difference is test_separation_between_libraries_with_prefixes no longer parameterizes Mongo because the test code used to skip on mongo.

@qc00 qc00 force-pushed the 1014-storage-fixtures branch 3 times, most recently from 0581e3f to d314348 Compare November 28, 2023 01:31
@qc00
Revert "REVERT ME: Temporarily skip all Mongo tests to fix CI"
ecff4e4 
This reverts commit 1b92535.
@qc00
Revert "Change tmpdir to tmp_path (#1093)"
2198ba6 
This reverts commit 5505fa4.
@qc00
Reapply parts of the previous commit that isn't superseded
55c0f73 
... without formatting to avoid conflicts
@qc00
StorageFixtures framework
4add295 
* Standard, not pytest-coupled API for storage fixture life cycles
* Methods for setting permissions and shutting down the fixtures independent of pytest for error handling testing
* Proper cleanup and exception handling to allow tests to run concurrently and independently of earlier failures
* PrefixingLibraryAdapterDecorator to support isolating Mongo fixture instances, but can be used with any other Storage type
@qc00
Simple S3 rate limiting
e213112
@qc00
Make tests use StorageFixtures correctly
3e7be3b 
+ simplified some of them
+ remove pytest-split
@qc00
Properly use tmp_path
03dd083
@qc00 qc00 merged commit e283662 into master Nov 28, 2023
131 checks passed
@qc00 qc00 deleted the 1014-storage-fixtures branch November 28, 2023 22:56
@qc00 qc00 mentioned this pull request Nov 29, 2023
Merged
@qc00 qc00 mentioned this pull request Dec 8, 2023
Merged
@alexowens90 alexowens90 mentioned this pull request Jan 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@G-D-Petrov G-D-Petrov G-D-Petrov left review comments

@poodlewars poodlewars poodlewars approved these changes

Assignees
No one assigned
Labels
refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@qc00 @poodlewars @G-D-Petrov