[Data] Simplify and remove the ordering dependency of download expression error handling tests #58518
+21
−66
Conversation
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
Refactor test_download_expression.py to follow unit testing best practices: - Remove assumptions about output ordering by explicitly sorting results - Reduce test complexity by using minimal inputs that verify the behavior - Improve code clarity by using from_items() instead of from_arrow() - Remove redundant test that didn't add coverage beyond existing tests Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request does a great job of refactoring the download expression tests to be simpler, more robust, and easier to maintain. The changes, such as using ray.data.from_items for better readability, adding sorting to eliminate flakiness, and simplifying complex tests, are all positive improvements. I have a couple of minor suggestions to further improve the robustness of the tests by consistently using pytest's tmp_path fixture, which will help avoid potential filesystem permission issues in different environments.
| ], | ||
| names=["uri"], | ||
| {"uri": f"local://{valid_file}", "id": 0}, | ||
| {"uri": "local:///nonexistent.txt", "id": 1}, |
Contributor
There was a problem hiding this comment.
Using an absolute path like local:///nonexistent.txt could lead to permission issues in restricted environments. It's better practice to use the tmp_path fixture for creating test file paths, even for non-existent files. This ensures that all file operations are contained within the temporary directory managed by pytest.
Suggested change
| {"uri": "local:///nonexistent.txt", "id": 1}, | |
| {"uri": f"local://{tmp_path}/nonexistent.txt", "id": 1}, |
Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>
xyuzh
reviewed
Nov 12, 2025
| ), | ||
| ], | ||
| names=["uri"], | ||
| {"uri": str(valid_file), "id": 0}, |
Contributor
There was a problem hiding this comment.
Not sure why would you change the format of the uri from f"local://{valid_file}" to str(valid_file)
Contributor
There was a problem hiding this comment.
It's ok to work without local:// prefix in this case, but I assume it's good to follow the pattern of other tests in the same file
|
|
||
| ds = ray.data.from_arrow(table) | ||
| # Create URIs that will fail size estimation (non-existent files). | ||
| ds = ray.data.from_items([{"uri": str(tmp_path / "nonexistent.txt")}]) |
xyuzh
reviewed
Nov 12, 2025
| assert results[2]["bytes"] is None | ||
|
|
||
| def test_download_expression_all_size_estimations_fail(self): | ||
| def test_download_expression_all_size_estimations_fail(self, tmp_path): |
Contributor
There was a problem hiding this comment.
change the name of the test as we only test 1 row here
Update test name and annotation to explain the purpose of the test Signed-off-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com>
xyuzh
changed the title
Signed-off-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com>
Signed-off-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com>
Signed-off-by: Robert Nishihara <robertnishihara@gmail.com>
robertnishihara
approved these changes
Nov 14, 2025
justinyeh1995
pushed a commit
to justinyeh1995/ray
that referenced
this pull request
Nov 14, 2025
…sion error handling tests (ray-project#58518) ## Description This PR refactors tests in `test_download_expression.py` to make them easier to maintain and less prone to brittle failures. Some of the previous tests were more complex than necessary and relied on assumptions that could occasionally cause false negatives. ### Key updates: * **Reduce flaky behavior**: Added explicit sorting by ID in `test_download_expression_handles_failed_downloads` to avoid relying on a specific output order, which isn’t guaranteed and could sometimes cause intermittent failures. * **Simplify test logic**: Reduced `test_download_expression_failed_size_estimation` from 30 URIs to just 1. A single failing URI is sufficient to confirm that failed downloads don’t trigger divide-by-zero errors, and this change makes the test easier to understand and faster to run. * **Improve readability**: Replaced `pa.Table.from_arrays()` with `ray.data.from_items()`, which makes the test setup more straightforward for future maintainers. * **Remove redundancy**: Deleted `test_download_expression_mixed_valid_and_invalid_size_estimation`, since its behavior is already covered by the other tests. Overall, these updates streamline the test suite, making it faster, clearer, and more robust while keeping the key behaviors fully verified. ## Related issue ray-project#58464 (comment) --------- Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu> Signed-off-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Signed-off-by: Robert Nishihara <robertnishihara@gmail.com> Co-authored-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Co-authored-by: Robert Nishihara <robertnishihara@gmail.com> Signed-off-by: justinyeh1995 <justinyeh1995@gmail.com>
ArturNiederfahrenhorst
pushed a commit
to ArturNiederfahrenhorst/ray
that referenced
this pull request
Nov 16, 2025
…sion error handling tests (ray-project#58518) ## Description This PR refactors tests in `test_download_expression.py` to make them easier to maintain and less prone to brittle failures. Some of the previous tests were more complex than necessary and relied on assumptions that could occasionally cause false negatives. ### Key updates: * **Reduce flaky behavior**: Added explicit sorting by ID in `test_download_expression_handles_failed_downloads` to avoid relying on a specific output order, which isn’t guaranteed and could sometimes cause intermittent failures. * **Simplify test logic**: Reduced `test_download_expression_failed_size_estimation` from 30 URIs to just 1. A single failing URI is sufficient to confirm that failed downloads don’t trigger divide-by-zero errors, and this change makes the test easier to understand and faster to run. * **Improve readability**: Replaced `pa.Table.from_arrays()` with `ray.data.from_items()`, which makes the test setup more straightforward for future maintainers. * **Remove redundancy**: Deleted `test_download_expression_mixed_valid_and_invalid_size_estimation`, since its behavior is already covered by the other tests. Overall, these updates streamline the test suite, making it faster, clearer, and more robust while keeping the key behaviors fully verified. ## Related issue ray-project#58464 (comment) --------- Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu> Signed-off-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Signed-off-by: Robert Nishihara <robertnishihara@gmail.com> Co-authored-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Co-authored-by: Robert Nishihara <robertnishihara@gmail.com>
Aydin-ab
pushed a commit
to Aydin-ab/ray-aydin
that referenced
this pull request
Nov 19, 2025
…sion error handling tests (ray-project#58518) ## Description This PR refactors tests in `test_download_expression.py` to make them easier to maintain and less prone to brittle failures. Some of the previous tests were more complex than necessary and relied on assumptions that could occasionally cause false negatives. ### Key updates: * **Reduce flaky behavior**: Added explicit sorting by ID in `test_download_expression_handles_failed_downloads` to avoid relying on a specific output order, which isn’t guaranteed and could sometimes cause intermittent failures. * **Simplify test logic**: Reduced `test_download_expression_failed_size_estimation` from 30 URIs to just 1. A single failing URI is sufficient to confirm that failed downloads don’t trigger divide-by-zero errors, and this change makes the test easier to understand and faster to run. * **Improve readability**: Replaced `pa.Table.from_arrays()` with `ray.data.from_items()`, which makes the test setup more straightforward for future maintainers. * **Remove redundancy**: Deleted `test_download_expression_mixed_valid_and_invalid_size_estimation`, since its behavior is already covered by the other tests. Overall, these updates streamline the test suite, making it faster, clearer, and more robust while keeping the key behaviors fully verified. ## Related issue ray-project#58464 (comment) --------- Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu> Signed-off-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Signed-off-by: Robert Nishihara <robertnishihara@gmail.com> Co-authored-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Co-authored-by: Robert Nishihara <robertnishihara@gmail.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
ykdojo
pushed a commit
to ykdojo/ray
that referenced
this pull request
Nov 27, 2025
…sion error handling tests (ray-project#58518) ## Description This PR refactors tests in `test_download_expression.py` to make them easier to maintain and less prone to brittle failures. Some of the previous tests were more complex than necessary and relied on assumptions that could occasionally cause false negatives. ### Key updates: * **Reduce flaky behavior**: Added explicit sorting by ID in `test_download_expression_handles_failed_downloads` to avoid relying on a specific output order, which isn’t guaranteed and could sometimes cause intermittent failures. * **Simplify test logic**: Reduced `test_download_expression_failed_size_estimation` from 30 URIs to just 1. A single failing URI is sufficient to confirm that failed downloads don’t trigger divide-by-zero errors, and this change makes the test easier to understand and faster to run. * **Improve readability**: Replaced `pa.Table.from_arrays()` with `ray.data.from_items()`, which makes the test setup more straightforward for future maintainers. * **Remove redundancy**: Deleted `test_download_expression_mixed_valid_and_invalid_size_estimation`, since its behavior is already covered by the other tests. Overall, these updates streamline the test suite, making it faster, clearer, and more robust while keeping the key behaviors fully verified. ## Related issue ray-project#58464 (comment) --------- Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu> Signed-off-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Signed-off-by: Robert Nishihara <robertnishihara@gmail.com> Co-authored-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Co-authored-by: Robert Nishihara <robertnishihara@gmail.com> Signed-off-by: YK <1811651+ykdojo@users.noreply.github.com>
SheldonTsen
pushed a commit
to SheldonTsen/ray
that referenced
this pull request
Dec 1, 2025
…sion error handling tests (ray-project#58518) ## Description This PR refactors tests in `test_download_expression.py` to make them easier to maintain and less prone to brittle failures. Some of the previous tests were more complex than necessary and relied on assumptions that could occasionally cause false negatives. ### Key updates: * **Reduce flaky behavior**: Added explicit sorting by ID in `test_download_expression_handles_failed_downloads` to avoid relying on a specific output order, which isn’t guaranteed and could sometimes cause intermittent failures. * **Simplify test logic**: Reduced `test_download_expression_failed_size_estimation` from 30 URIs to just 1. A single failing URI is sufficient to confirm that failed downloads don’t trigger divide-by-zero errors, and this change makes the test easier to understand and faster to run. * **Improve readability**: Replaced `pa.Table.from_arrays()` with `ray.data.from_items()`, which makes the test setup more straightforward for future maintainers. * **Remove redundancy**: Deleted `test_download_expression_mixed_valid_and_invalid_size_estimation`, since its behavior is already covered by the other tests. Overall, these updates streamline the test suite, making it faster, clearer, and more robust while keeping the key behaviors fully verified. ## Related issue ray-project#58464 (comment) --------- Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu> Signed-off-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Signed-off-by: Robert Nishihara <robertnishihara@gmail.com> Co-authored-by: Xinyu Zhang <60529799+xyuzh@users.noreply.github.com> Co-authored-by: Robert Nishihara <robertnishihara@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR refactors tests in
test_download_expression.pyto make them easier to maintain and less prone to brittle failures. Some of the previous tests were more complex than necessary and relied on assumptions that could occasionally cause false negatives.Key updates:
test_download_expression_handles_failed_downloadsto avoid relying on a specific output order, which isn’t guaranteed and could sometimes cause intermittent failures.test_download_expression_failed_size_estimationfrom 30 URIs to just 1. A single failing URI is sufficient to confirm that failed downloads don’t trigger divide-by-zero errors, and this change makes the test easier to understand and faster to run.pa.Table.from_arrays()withray.data.from_items(), which makes the test setup more straightforward for future maintainers.test_download_expression_mixed_valid_and_invalid_size_estimation, since its behavior is already covered by the other tests.Overall, these updates streamline the test suite, making it faster, clearer, and more robust while keeping the key behaviors fully verified.
Related issue
#58464 (comment)