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.

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

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

Merged
merged 2 commits into from
May 17, 2024
Merged

Conversation

can-anyscale
Copy link
Collaborator

@can-anyscale can-anyscale commented May 17, 2024

Allow PR owners to add tests to microcheck manually by adding a line with syntax

@microcheck //test_target_01 //test_target_02

to the git commit message. Thanks @stephanie-wang for the great idea.

Note that it's ok to provide an invalid target because tester will re-validate them with bazel query.

Test:

@MicroCheck //release:test_config //release:invalid_test

Signed-off-by: can <can@anyscale.com>
@can-anyscale can-anyscale marked this pull request as ready for review May 17, 2024 13:24
@can-anyscale can-anyscale requested a review from a team as a code owner May 17, 2024 13:24
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.

I think the idea that allowing users to pick which test to run is great. I am slightly against using git commit message though, as it often leads to weird-looking git commit messages merged into master.

can we use some other means? for example, asking users to manually trigger the pipeline with addtional env var? or maybe this is the time for us to build a GitHub app?

@can-anyscale
Copy link
Collaborator Author

weird-looking git commit messages merged into master

hmm can you elaborate more, when does this happen?

asking users to manually trigger the pipeline with addtional env var

an option yes but it adds more frictions into the people workflow; i'm choosing the git commit message since that's already on the part of people existing workflow so it creates minimal overhead for folks

@can-anyscale
Copy link
Collaborator Author

i think users will prefer git commit messages way more than manually creating builds (i don't understand the github app enough to have an opinion); might be an overkill but we can ask a few OSS engineers if needed for them to choose

the merge message is the github PR description so git commit message is a non-issue as well (understand that some repos merge message is git commit message but it should be a no issue for ray; also for those repos, people often have to self-massage the merge message since the git commit message format is inherently messy anyway)

@can-anyscale
Copy link
Collaborator Author

can-anyscale commented May 17, 2024

discussed offline, we agree to go with this solution and redo with another option if we see more than 3 mailformed git commit title caused by this from now until Sep 30th.

ci/ray_ci/tester.py Outdated Show resolved Hide resolved
Comment on lines +441 to +442
for message in messages.decode().splitlines():
if message.startswith(MICROCHECK_COMMAND):
tests = tests.union(message[len(MICROCHECK_COMMAND) :].strip().split(" "))
Copy link
Collaborator

@aslonnie aslonnie May 17, 2024

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

- only check the git body

Signed-off-by: can <can@anyscale.com>
@can-anyscale can-anyscale enabled auto-merge (squash) May 17, 2024 20:33
@github-actions github-actions bot added the go Trigger full test run on premerge label May 17, 2024
@can-anyscale can-anyscale merged commit 566dd5e into master May 17, 2024
8 checks passed
@can-anyscale can-anyscale deleted the can-mcx09 branch May 17, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Trigger full test run on premerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants