[Data] Fixture to keep unit tests apart from integration directory#61505
[Data] Fixture to keep unit tests apart from integration directory#61505Hyunoh-Yeo wants to merge 14 commits intoray-project:masterfrom
Conversation
Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a helpful pytest fixture to enforce separation between unit and integration tests within the tests/ directory. The use of an opt-in mechanism with MIGRATED_FILES is a good strategy for incremental adoption. The implementation is solid, but I have a few suggestions to improve documentation and code style for better maintainability.
…vior Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
…vior Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
…vior Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
Signed-off-by: Hyunoh-Yeo <hyunoh.yeo@gmail.com>
| return | ||
|
|
||
| # Skip if the test is marked as an integration test | ||
| if request.node.get_closest_marker("integration_test"): |
There was a problem hiding this comment.
I'm thinking if we need integration_test marker. Our purpose here is to prevent people adding unit test into python/ray/data/tests, so maybe we only need to introduce unit_for_integration for unit tests under python/ray/data/tests, and consider other tests without this marker as integration test?
There was a problem hiding this comment.
I was thinking of that too, but the issue was that if we do that, we cannot identify every indirect call of ray cluster that does not use ray.init() directly or explicitly having the ray_start fixture, which means the CI might incorrectly call errors to integration tests as unit tests, which can be confusing. Having integration_test was the best call to let developers themselves to identify what their tests are doing and locate to the appropriate positions.
There was a problem hiding this comment.
Actually for this, I might need confirmation whether we enforce every tests with direct and indirect calls of ray cluster to have ray_start fixtures. I was taking the safer option
|
Could we use |
|
@machichima Thank you for your comments! The current approach is intentional. we want to confirm the test itself is passing while separately flagging the missing marker. If we fail in setup, we lose visibility into whether the test logic is actually correct. If the test failes before the marker check, developers can first work on have their test working and then add markers for the final check. Without this, the developers who were unaware of the marker will follow: With this: |
Description
Adds a pytest fixture warn_if_unit_in_integration to tests/conftest.py that warns developers when a test in the integration directory has no Ray cluster dependency and no intent marker. Also adds two new markers @pytest.mark.integration_test and @pytest.mark.unit_for_integration for developers to document intent explicitly.
The fixture is opt-in via MIGRATED_FILES and applied incrementally as files are reviewed and split. This PR opts in test_arrow_block.py and test_transform_pyarrow.py as the first two files, adding the appropriate markers to all tests in both files.
*Since
pytest.iniignores all warnings withignore:.*the fixture raises exceptions instead of warnings to ensure CI fails when tests lack proper markers. This only applies to MIGRATED_FILES which I have done migrating and separating integration tests with unit tests.Related issues
Closes #61339
Additional information
The fixture only applies to test files directly in tests/ (not subdirectories like tests/unit/), and only to files explicitly added to MIGRATED_FILES. See the design doc for full motivation and design decisions.
Local Test file and results
(I left some intentional errors each corresponds to the fixture's behavior)