[Data] Remove test_large_args_scheduling_strategy
#58966
Merged
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
Remove test that tests implementation details rather than behavior and conflates multiple concerns. Signed-off-by: Balaji Veeramani <[email protected]>
bveeramani
changed the title
test_large_args_scheduling_strategy
![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 removes the test_large_args_scheduling_strategy test from test_stats.py, along with its associated constants. The rationale for this removal is well-justified: the test was brittle due to hardcoded implementation details, it tested the format of statistics rather than the actual scheduling behavior, and it conflated multiple concerns. The changes are clean and correctly remove the specified test and its unused helper variables. This is a good cleanup that improves the quality and maintainability of the test suite. I have no further comments.
owenowenisme
approved these changes
Nov 25, 2025
SheldonTsen
pushed a commit
to SheldonTsen/ray
that referenced
this pull request
Dec 1, 2025
## Summary This PR removes `test_large_args_scheduling_strategy` from `test_stats.py` because its flaky and not worth keeping (It tests implementation details rather than behavior and conflates multiple concerns) See https://buildkite.com/ray-project/premerge/builds/54495#019ab720-249f-49c5-8e25-5e9005cc41e2 ## Motivation 1. **Hardcodes scheduling strategy values** - The test assumes large args use `'DEFAULT'` and small args use `'SPREAD'`. If these defaults change in `context.py`, the test fails even though the system is working correctly. 2. **Tests stats format, not scheduling behavior** - The test doesn't verify that the correct scheduling strategy is actually passed to Ray tasks. It only checks that a specific string appears in stats output. 3. **Mixes two concerns** - The test conflates: - Scheduling strategy selection based on data size (belongs in a map-related test) - Stats output including scheduling strategy info (belongs in a general stats formatting test) Signed-off-by: Balaji Veeramani <[email protected]>
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.
Summary
This PR removes
test_large_args_scheduling_strategyfromtest_stats.pybecause its flaky and not worth keeping (It tests implementation details rather than behavior and conflates multiple concerns)See https://buildkite.com/ray-project/premerge/builds/54495#019ab720-249f-49c5-8e25-5e9005cc41e2
Motivation
Hardcodes scheduling strategy values - The test assumes large args use
'DEFAULT'and small args use'SPREAD'. If these defaults change incontext.py, the test fails even though the system is working correctly.Tests stats format, not scheduling behavior - The test doesn't verify that the correct scheduling strategy is actually passed to Ray tasks. It only checks that a specific string appears in stats output.
Mixes two concerns - The test conflates: