Skip to content

Conversation

bveeramani
Copy link
Member

Why are these changes needed?

Our CI runs tests with Bazel. Rather than running the tests with pytest, Bazel's executes the test Python files directly. So, to ensure that the tests actually run, we need to invoke pytest explicitly in our test files like this:

if __name__ == "__main__":
    import sys

    sys.exit(pytest.main(["-v", __file__]))

This PR adds a lint to enforce this.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run pre-commit jobs to lint the changes in this PR. (pre-commit setup)
  • 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 :(

Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>
Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>
@bveeramani bveeramani requested a review from a team as a code owner October 9, 2025 16:17
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 introduces a valuable linting check to ensure all tests in ray.data are executable, which is a great improvement for CI reliability. The changes correctly add the necessary pytest.main call to several test files. My review includes a few minor suggestions to improve code style consistency in the newly added code snippets by grouping imports, following PEP 8 guidelines. This also includes updating the semgrep rule's suggested snippet to reflect this style.

@bveeramani bveeramani requested a review from aslonnie October 9, 2025 16:19
types-PyYAML==6.0.12.2
black==22.10.0
semgrep==1.32.0
semgrep==1.136.0
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed to upgrade the version to make this work properly

Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>
@@ -0,0 +1,2 @@
def test_ham():
assert False
Copy link

Choose a reason for hiding this comment

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

Bug: Test File Commit Causes CI and Lint Failures

A temporary test file was accidentally committed. It contains an assert False that will cause CI failures, and it's missing the pytest.main() invocation, which violates a new lint rule and will also cause lint failures.

Fix in Cursor Fix in Web

@ray-gardener ray-gardener bot added data Ray Data-related issues devprod labels Oct 9, 2025
@aslonnie
Copy link
Collaborator

aslonnie commented Oct 9, 2025

I think we already have a lint for this?
https://github.com/ray-project/ray/blob/master/ci/lint/check-pytest-format.sh

@aslonnie aslonnie requested review from elliot-barn and removed request for aslonnie October 9, 2025 20:53
Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

leaving this for @elliot-barn to review. hail lord of lints!

I think we already have a lint for that, if we want to migrate to use semgrep, maybe remove the old one?

@bveeramani
Copy link
Member Author

leaving this for @elliot-barn to review. hail lord of lints!

I think we already have a lint for that, if we want to migrate to use semgrep, maybe remove the old one?

@aslonnie do you remember what the old one is called? Don't think it's working properly, because there are several Data tests that aren't actually run

@aslonnie
Copy link
Collaborator

aslonnie commented Oct 9, 2025

@elliot-barn
Copy link
Contributor

https://github.com/ray-project/ray/blob/master/ci/lint/check-pytest-format.sh

Lets use semgrep and get rid of the pytest_checker script? Ill open a PR with these changes and pre-commit checks

@bveeramani
Copy link
Member Author

Closing in favor of #57617

@bveeramani bveeramani closed this Oct 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

data Ray Data-related issues devprod

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants