Skip to content

Conversation

@aslonnie
Copy link
Collaborator

otherwise, the ordering or messages looks strange on windows.

otherwise, the ordering or messages looks strange on windows.

Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
@aslonnie aslonnie requested a review from a team as a code owner October 22, 2025 21:40
@aslonnie
Copy link
Collaborator Author

testing with windows core test: https://buildkite.com/ray-project/postmerge/builds/13952

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 correctly fixes a message ordering issue on Windows CI builds by redirecting progress messages to stderr. The changes are straightforward and effective. I have one suggestion regarding the repeated use of print(..., file=sys.stderr) for these CI messages. To improve maintainability, it would be beneficial to abstract this into a dedicated helper function. This would make the code cleaner and the intent of these special print statements more explicit.

Comment on lines +281 to +284
print("--- No tests to run", file=sys.stderr)
sys.exit(0)

print(f"+++ Running {len(test_targets)} tests")
print(f"+++ Running {len(test_targets)} tests", file=sys.stderr)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

These changes correctly redirect CI progress messages to stderr. Since this pattern is now used on line 260 as well, consider abstracting it into a helper function to improve maintainability and clarify that these are special CI-related outputs. For example:

def _log_ci_status(message: str) -> None:
    """Prints a status message to stderr for CI visibility."""
    print(message, file=sys.stderr)

This would centralize the logic and make the code easier to read and maintain.

@aslonnie aslonnie added the go add ONLY when ready to merge, run all tests label Oct 22, 2025
@aslonnie aslonnie merged commit a6193b2 into master Oct 22, 2025
6 checks passed
@aslonnie aslonnie deleted the lonnie-251022-testerlog branch October 22, 2025 22:43
xinyuangui2 pushed a commit to xinyuangui2/ray that referenced this pull request Oct 27, 2025
otherwise, the ordering or messages looks strange on windows.

Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
Signed-off-by: xgui <xgui@anyscale.com>
landscapepainter pushed a commit to landscapepainter/ray that referenced this pull request Nov 17, 2025
otherwise, the ordering or messages looks strange on windows.

Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
Aydin-ab pushed a commit to Aydin-ab/ray-aydin that referenced this pull request Nov 19, 2025
otherwise, the ordering or messages looks strange on windows.

Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
Signed-off-by: Aydin Abiar <aydin@anyscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants