Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ci][microcheck] manually add tests to microcheck #45400

Merged
merged 2 commits into from
May 17, 2024
Merged

[ci][microcheck] manually add tests to microcheck #45400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96744a2
[ci] manually added tests to microcheck
can-anyscale May 17, 2024
fa54f04
- remove git fetch
can-anyscale May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion ci/ray_ci/test_tester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
_get_tag_matcher,
_get_changed_files,
_get_changed_tests,
_get_human_specified_tests,
)
from ray_release.test import Test, TestState

Expand Down Expand Up @@ -214,6 +215,7 @@ def test_get_high_impact_test_targets() -> None:
{
"input": [],
"new_tests": set(),
"human_tests": set(),
"output": set(),
},
{
Expand All @@ -232,9 +234,11 @@ def test_get_high_impact_test_targets() -> None:
),
],
"new_tests": {"//core_new"},
"human_tests": {"//human_test"},
"output": {
"//core_good",
"//core_new",
"//human_test",
},
},
]
Expand All @@ -245,6 +249,9 @@ def test_get_high_impact_test_targets() -> None:
), mock.patch(
"ci.ray_ci.tester._get_changed_tests",
return_value=test["new_tests"],
), mock.patch(
"ci.ray_ci.tester._get_human_specified_tests",
return_value=test["human_tests"],
):
assert (
_get_high_impact_test_targets(
Expand All @@ -256,7 +263,10 @@ def test_get_high_impact_test_targets() -> None:
)


@mock.patch.dict(os.environ, {"BUILDKITE_PULL_REQUEST_BASE_BRANCH": "base"})
@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:
Expand All @@ -277,6 +287,17 @@ def test_get_changed_tests(
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"}


def test_get_flaky_test_targets() -> None:
test_harness = [
{
Expand Down
28 changes: 26 additions & 2 deletions ci/ray_ci/tester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
""" # noqa: E501

DEFAULT_EXCEPT_TAGS = {"manual"}
MICROCHECK_COMMAND = "@microcheck"

# Gets the path of product/tools/docker (i.e. the parent of 'common')
bazel_workspace_dir = os.environ.get("BUILD_WORKSPACE_DIRECTORY", "")
Expand Down Expand Up @@ -416,8 +417,32 @@ def _get_high_impact_test_targets(
if test.get_oncall() == team
}
changed_tests = _get_changed_tests()
human_specified_tests = _get_human_specified_tests()

return high_impact_tests.union(changed_tests)
return high_impact_tests.union(changed_tests).union(human_specified_tests)


def _get_human_specified_tests() -> Set[str]:
"""
Get all test targets that are specified by humans
"""
base = os.environ.get("BUILDKITE_PULL_REQUEST_BASE_BRANCH")
head = os.environ.get("BUILDKITE_COMMIT")
if not base or not head:
# if not in a PR, return an empty set
return set()

tests = set()
messages = subprocess.check_output(
["git", "rev-list", "--format=%b", f"origin/{base}...{head}"],
cwd=bazel_workspace_dir,
)
for message in messages.decode().splitlines():
if message.startswith(MICROCHECK_COMMAND):
tests = tests.union(message[len(MICROCHECK_COMMAND) :].strip().split(" "))
Comment on lines +440 to +442
Copy link
Collaborator

@aslonnie aslonnie May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is decoding all messages? why not just the last one? I think we just need the last one?

is it possible to just check the git body, but not the title? then there is a lot less risks of having @microcheck polluting the title, and much easier to meet the 0.3% goal.

and I am much more tolerant to having unrelated info in the commit message body.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

intentionally all messages yes, so that folks don't need to re-add tests again on every commit update

let me check how to get the body vs title

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

use format=%b to get the git body only

logger.info(f"Human specified tests: {tests}")

return tests


def _get_changed_tests() -> Set[str]:
Expand Down Expand Up @@ -472,7 +497,6 @@ def _get_changed_files() -> Set[str]:
# if not in a PR, return an empty set
return set()

subprocess.check_call(["git", "fetch", "origin", base], cwd=bazel_workspace_dir)
changes = subprocess.check_output(
["git", "diff", "--name-only", f"origin/{base}...{head}"],
cwd=bazel_workspace_dir,
Expand Down
Loading