[ci][microcheck/13] sync microcheck determination logic

Signed-off-by: can <can@anyscale.com>

ci/ray_ci/automation/determine_microcheck_step_ids.py

@@ -20,8 +20,8 @@ def main() -> None:
     ci_init()
     steps = (
         Test.gen_microcheck_step_ids(LINUX_TEST_PREFIX, BAZEL_WORKSPACE_DIR)
-        + Test.gen_microcheck_step_ids(WINDOWS_TEST_PREFIX, BAZEL_WORKSPACE_DIR)
-        + Test.gen_microcheck_step_ids(MACOS_TEST_PREFIX, BAZEL_WORKSPACE_DIR)
+        .union(Test.gen_microcheck_step_ids(WINDOWS_TEST_PREFIX, BAZEL_WORKSPACE_DIR))
+        .union(Test.gen_microcheck_step_ids(MACOS_TEST_PREFIX, BAZEL_WORKSPACE_DIR))
     )
 
     print(",".join(steps))

ci/ray_ci/test_tester.py

@@ -13,17 +13,9 @@
     _get_container,
     _get_all_test_query,
     _get_test_targets,
-<<<<<<< HEAD
-    _get_high_impact_test_targets,
     _get_new_tests,
-=======
->>>>>>> [ci][microcheck/13] sync microcheck test determinator
     _get_flaky_test_targets,
     _get_tag_matcher,
-    _get_new_tests,
-    _get_changed_files,
-    _get_changed_tests,
-    _get_human_specified_tests,
 )
 from ray_release.test import Test, TestState
 
@@ -128,6 +120,9 @@ def test_get_test_targets() -> None:
         ), mock.patch(
             "ray_release.test.Test.gen_from_s3",
             return_value=test_objects,
+        ), mock.patch(
+            "ci.ray_ci.tester._get_new_tests",
+            return_value=set(),
         ), mock.patch(
             "ray_release.test.Test.gen_microcheck_tests",
             return_value={test.get_target() for test in test_objects},
@@ -212,113 +207,19 @@ def test_get_all_test_query() -> None:
     )
 
 
-<<<<<<< HEAD
-def test_get_high_impact_test_targets() -> None:
-    test_harness = [
-        {
-            "input": [],
-            "new_tests": set(),
-            "changed_tests": set(),
-            "human_tests": set(),
-            "output": set(),
-        },
-        {
-            "input": [
-                _stub_test(
-                    {
-                        "name": "linux://core_good",
-                        "team": "core",
-                    }
-                ),
-                _stub_test(
-                    {
-                        "name": "linux://serve_good",
-                        "team": "serve",
-                    }
-                ),
-            ],
-            "new_tests": {"//core_new"},
-            "changed_tests": {"//core_new"},
-            "human_tests": {"//human_test"},
-            "output": {
-                "//core_good",
-                "//core_new",
-                "//human_test",
-            },
-        },
-    ]
-    for test in test_harness:
-        with mock.patch(
-            "ray_release.test.Test.gen_high_impact_tests",
-            return_value={"step": test["input"]},
-        ), mock.patch(
-            "ray_release.test.Test.get_new_tests",
-            return_value=test["new_tests"],
-        ), mock.patch(
-            "ray_release.test.Test.get_changed_tests",
-            return_value=test["changed_tests"],
-        ), mock.patch(
-            "ray_release.test.Test.get_human_specified_tests",
-            return_value=test["human_tests"],
-        ):
-            assert (
-                _get_high_impact_test_targets(
-                    "core",
-                    "linux",
-                    LinuxTesterContainer("test", skip_ray_installation=True),
-                )
-                == test["output"]
-            )
-
-
 @mock.patch("ci.ray_ci.tester_container.TesterContainer.run_script_with_output")
 @mock.patch("ray_release.test.Test.gen_from_s3")
 def test_get_new_tests(mock_gen_from_s3, mock_run_script_with_output) -> None:
     mock_gen_from_s3.return_value = [
         _stub_test({"name": "linux://old_test_01"}),
         _stub_test({"name": "linux://old_test_02"}),
     ]
-    mock_check_output.return_value = b"//old_test_01\n//new_test"
-    assert _get_new_tests("linux") == {"//new_test"}
-
-
-@mock.patch.dict(
-    os.environ,
-    {"BUILDKITE_PULL_REQUEST_BASE_BRANCH": "base", "BUILDKITE_COMMIT": "commit"},
-)
-@mock.patch("subprocess.check_call")
-@mock.patch("subprocess.check_output")
-def test_get_changed_files(mock_check_output, mock_check_call) -> None:
-    mock_check_output.return_value = b"file1\nfile2\n"
-    assert _get_changed_files() == {"file1", "file2"}
-
-
-@mock.patch("ci.ray_ci.tester._get_test_targets_per_file")
-@mock.patch("ci.ray_ci.tester._get_changed_files")
-def test_get_changed_tests(
-    mock_get_changed_files, mock_get_test_targets_per_file
-) -> None:
-    mock_get_changed_files.return_value = {"test_src", "build_src"}
-    mock_get_test_targets_per_file.side_effect = (
-        lambda x: {"//t1", "//t2"} if x == "test_src" else {}
-    )
-
-    assert _get_changed_tests() == {"//t1", "//t2"}
-
-
-@mock.patch.dict(
-    os.environ,
-    {"BUILDKITE_PULL_REQUEST_BASE_BRANCH": "base", "BUILDKITE_COMMIT": "commit"},
-)
-@mock.patch("subprocess.check_call")
-@mock.patch("subprocess.check_output")
-def test_get_human_specified_tests(mock_check_output, mock_check_call) -> None:
-    mock_check_output.return_value = b"hi\n@microcheck //test01 //test02\nthere"
-    assert _get_human_specified_tests() == {"//test01", "//test02"}
+    mock_run_script_with_output.return_value = "//old_test_01\n//new_test"
+    assert _get_new_tests(
+        "linux", LinuxTesterContainer("test", skip_ray_installation=True)
+    ) == {"//new_test"}
 
 
-=======
->>>>>>> [ci][microcheck/13] sync microcheck test determinator
 def test_get_flaky_test_targets() -> None:
     test_harness = [
         {

ci/ray_ci/tester.py

@@ -392,11 +392,15 @@ def _get_test_targets(
     if get_high_impact_tests:
         # run high impact test cases, so we include only high impact tests in the list
         # of targets provided by users
+        prefix = f"{operating_system}:"
+        # TODO(can): we should also move the logic of _get_new_tests into the
+        # gen_microcheck_tests function; this is currently blocked by the fact that
+        # we need a container to run _get_new_tests
         high_impact_tests = Test.gen_microcheck_tests(
-            prefix=f"{operating_system}:",
+            prefix=prefix,
             bazel_workspace_dir=bazel_workspace_dir,
             team=team,
-        ).union(_get_new_tests(f"{operating_system}:", container)
+        ).union(_get_new_tests(prefix, container))
         final_targets = high_impact_tests.intersection(final_targets)
 
     return list(final_targets)

release/ray_release/test.py

@@ -197,36 +197,29 @@ def gen_from_s3(cls, prefix: str):
             for file in files
         ]
 
-    def _get_step_ids(self) -> List[str]:
-        """
-        Obtain the rayci step id for this test from the most recent test results
-        """
-        step_ids = []
-        recent_results = self.get_test_results()
-        if not recent_results:
-            return step_ids
-        recent_commit = recent_results[0].commit
-        for result in recent_results:
-            # consider all results with the same recent commit; this is to make sure
-            # we will include different job flavors of the same test
-            if result.commit != recent_commit:
-                continue
-            step_id = result.rayci_step_id
-            if not step_id:
-                continue
-            step_ids.append(step_id)
-
-        return step_ids
-
     @classmethod
     def gen_microcheck_step_ids(cls, prefix: str, bazel_workspace_dir: str) -> Set[str]:
+        """
+        This function is used to get the buildkite step ids of the microcheck tests
+        with the given test prefix. This is used to determine the buildkite steps in
+        the microcheck pipeline.
+        """
         step_ids = set()
         test_targets = cls.gen_microcheck_tests(prefix, bazel_workspace_dir)
         for test_target in test_targets:
             test = cls.gen_from_name(f"{prefix}{test_target}")
             if not test:
                 continue
-            step_ids.update(test._get_step_ids())
+            recent_results = test.get_test_results()
+            if not recent_results:
+                continue
+            test_step_ids = {
+                result.rayci_step_id
+                for result in recent_results
+                if result.commit == recent_results[0].commit and result.rayci_step_id
+            }
+            if test_step_ids and not step_ids.intersection(test_step_ids):
+                step_ids.add(test_step_ids.pop())
 
         return step_ids
 
@@ -237,6 +230,19 @@ def gen_microcheck_tests(
         """
         Obtain all microcheck tests with the given prefix
         """
+        high_impact_tests = Test._gen_high_impact_tests(prefix, team)
+        changed_tests = Test._get_changed_tests(bazel_workspace_dir)
+        human_specified_tests = Test._get_human_specified_tests(bazel_workspace_dir)
+
+        return high_impact_tests.union(changed_tests, human_specified_tests)
+
+    @classmethod
+    def _gen_high_impact_tests(
+        cls, prefix: str, team: Optional[str] = None
+    ) -> Set[str]:
+        """
+        Obtain all high impact tests with the given prefix
+        """
         high_impact_tests = [
             test for test in cls.gen_from_s3(prefix) if test.is_high_impact()
         ]
@@ -245,19 +251,10 @@ def gen_microcheck_tests(
                 test for test in high_impact_tests if test.get_oncall() == team
             ]
 
-        high_impact_test_targets = {test.get_target() for test in high_impact_tests}
-        new_test_targets = Test.get_new_tests(prefix, bazel_workspace_dir)
-        changed_test_targets = Test.get_changed_tests(bazel_workspace_dir)
-        human_specified_test_targets = Test.get_human_specified_tests(
-            bazel_workspace_dir
-        )
-
-        return high_impact_test_targets.union(
-            new_test_targets, changed_test_targets, human_specified_test_targets
-        )
+        return {test.get_target() for test in high_impact_tests}
 
     @classmethod
-    def get_human_specified_tests(cls, bazel_workspace_dir: str) -> Set[str]:
+    def _get_human_specified_tests(cls, bazel_workspace_dir: str) -> Set[str]:
         """
         Get all test targets that are specified by humans
         """
@@ -280,7 +277,7 @@ def get_human_specified_tests(cls, bazel_workspace_dir: str) -> Set[str]:
         return tests
 
     @classmethod
-    def get_changed_tests(cls, bazel_workspace_dir: str) -> Set[str]:
+    def _get_changed_tests(cls, bazel_workspace_dir: str) -> Set[str]:
         """
         Get all changed tests in the current PR
         """