Skip to content

Conversation

crypdick
Copy link
Contributor

Why are these changes needed?

Adds a multi-node test for symmetric_run.py

Related issue number

#55119
#55111

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@crypdick crypdick requested a review from a team as a code owner September 15, 2025 22:29
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a multi-node test for symmetric_run.py and improves its module docstring. The changes are a good addition and increase test coverage. I have a couple of suggestions to improve the new test's maintainability and robustness by refactoring duplicated code and making assertions less brittle.

Signed-off-by: Ricardo Decal <[email protected]>

common_args = ["--address", address, "--min-nodes", "3", "--", "echo", "ok"]

with patch("subprocess.run") as mock_run, patch(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not mock subprocess.run but let it actually starts Ray nodes.

@ray-gardener ray-gardener bot added core Issues that should be addressed in Ray Core release-test release test labels Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues that should be addressed in Ray Core release-test release test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants