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.

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

feat object_store: moving tests from src/ to a tests/ folder and enabling access to test functions for enabling a shared integration test suite #5685

Closed
2 tasks
Silemo opened this issue Apr 24, 2024 · 3 comments · Fixed by #5709
Labels
enhancement Any new improvement worthy of a entry in the changelog

Comments

@Silemo
Copy link
Contributor

Silemo commented Apr 24, 2024

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Hi young dev here!

I am currently busy doing an external implementation (or to phrase it better, I am essential refactoring the datafusion-contrib/datafusion-objectstore-hdfs repository to support the latest 0.10 object store interface.

While my src implementation is complete, I am still lacking the tests. While looking at the implementation of the object_store interface in this repository, I've noticed that most of tests in ObjectStore implementations (Local, Azure etc.) call test functions defined in object_store/src/lib.rs. These tests offer a crate visibility, so even thou my implementation would benefit from it, I can't access it.

After a brief discussion with @tustvold, he suggested to generalise my need to create a suite of shared integration tests across object_store implementations.

Describe the solution you'd like

This solution comprises of mainly two points

  • Move the tests from the source files to a mirror into /tests folder
  • Change the visibility (from pub(crate) to pub) of test functions, ensuring this change is not in conflict with private accesses.

Describe alternatives you've considered

Alternatives to this feature would be to decouple the tests (completely refactor what is the current status of the repository) leading to code repetition (tests are doing a similar task, even for different implementations.

If there is no will of maintaining these tests (making them public), any external implementation of the object_store interface should resort to a copy-paste of the functions currently present (code duplication).

@Silemo Silemo added the enhancement Any new improvement worthy of a entry in the changelog label Apr 24, 2024
@Silemo
Copy link
Contributor Author

Silemo commented Apr 25, 2024

After some research and expanding my understanding of how the /tests folder works in Rust, I understood that it is not possible to migrate tests AND also make them visible to outside crates.

Truth is, to allow external implementation to access the same tests methods used in the ObjectStore implementations contained in the object_store library I don't see another way other than making them public.

As suggested by @tustvold, we could still "hide" them behind a feature flag. Let me know if this is an approach would work and possibly accepted as a PR

@tustvold
Copy link
Contributor

I think we've gotten our wires crossed, I'll find some time next week to sort this out for you

@Silemo
Copy link
Contributor Author

Silemo commented May 2, 2024

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants