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

[core] [easy] [no-op] No gtest code in production code #49359

Closed
wants to merge 3 commits into from

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Dec 19, 2024

As titled, shouldn't have gtest assertion code in production; also some minor updates.

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Dec 19, 2024
Signed-off-by: dentiny <dentinyhao@gmail.com>
@dentiny dentiny force-pushed the hjiang/no-test-in-prod branch from b7148ae to a994c82 Compare December 19, 2024 10:12
@jjyao
Copy link
Collaborator

jjyao commented Dec 19, 2024

Test failures.

@dentiny
Copy link
Contributor Author

dentiny commented Dec 19, 2024

Test failures.

For my change, compilation is enough

sched_cls_cap_enabled_(RayConfig::instance().worker_cap_enabled()),
sched_cls_cap_interval_ms_(sched_cls_cap_interval_ms),
sched_cls_cap_max_ms_(RayConfig::instance().worker_cap_max_backoff_delay_ms()) {}

void LocalTaskManager::QueueAndScheduleTask(std::shared_ptr<internal::Work> work) {
// If the local node is draining, the cluster task manager will
// guarantee that the local node is not selected for scheduling.
ASSERT_FALSE(
Copy link
Contributor

Choose a reason for hiding this comment

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

which #import did this macro get in from? Shall we also remove that macro, and maybe in bazel target deps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For our build file organization, it's not easy to check by bazel target dependency, since we place all files into one giant target.

ray/BUILD.bazel

Lines 675 to 695 in 2dd6061

ray_cc_library(
name = "raylet_lib",
srcs = glob(
[
"src/ray/raylet/**/*.cc",
],
exclude = [
"src/ray/raylet/**/*_test.cc",
"src/ray/raylet/scheduling/**/*.cc",
"src/ray/raylet/main.cc",
],
),
hdrs = glob(
[
"src/ray/raylet/**/*.h",
],
exclude = [
"src/ray/raylet/scheduling/**/*.h",
"src/ray/raylet/main.cc",
],
),

Copy link
Contributor

Choose a reason for hiding this comment

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

sad news

@dentiny dentiny requested a review from rynewang December 19, 2024 21:42
@rynewang
Copy link
Contributor

raylet crash in ci

@dentiny
Copy link
Contributor Author

dentiny commented Dec 24, 2024

Close it temporarily, reopen when I got more b/w

@dentiny dentiny closed this Dec 24, 2024
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.

4 participants