turn off cpu only tests to unblock CI during investigation#959
turn off cpu only tests to unblock CI during investigation#959rapids-bot[bot] merged 1 commit intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughCPU-only tests are being disabled across two test suites. In the C++ test file, two test cases are renamed with the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.py (1)
171-172: Class-level skip marker correctly applied.Using
pytestmarkat class level is the idiomatic pytest approach for skipping all tests in a class. The reason string provides context about the temporary nature.Same suggestion as the C++ file: consider adding an issue reference to track re-enabling these tests:
- pytestmark = pytest.mark.skip(reason="CPU-only tests temporarily disabled") + pytestmark = pytest.mark.skip(reason="CPU-only tests temporarily disabled - see `#XXX`")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.py` around lines 171 - 172, The class-level pytest skip is fine but please add an issue tracker reference to the skip reason so future maintainers know where to follow up; update the pytestmark assignment (pytestmark = pytest.mark.skip(...)) in test_cpu_only_execution.py to include an issue ID or URL in the reason string (e.g., "CPU-only tests temporarily disabled – see ISSUE-1234") so it’s easy to find the re-enable task.cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp (1)
385-400: Tests correctly disabled using GoogleTest convention.The
DISABLED_prefix is the proper GoogleTest mechanism for temporarily skipping tests. The test bodies are preserved, which is appropriate for a temporary measure.Consider adding a tracking reference (e.g., GitHub issue number) to ensure these tests are re-enabled once the CI environment issue is resolved:
-// TODO: Add numerical assertions once gRPC remote solver replaces the stub implementation. -// Currently validates that the CPU-only C API path completes without errors. +// TODO: Add numerical assertions once gRPC remote solver replaces the stub implementation. +// Currently validates that the CPU-only C API path completes without errors. +// TODO(`#XXX`): Re-enable once CI environment issue is resolved. TEST(c_api_cpu_only, DISABLED_lp_solve)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp` around lines 385 - 400, The tests c_api_cpu_only::DISABLED_lp_solve and c_api_cpu_only::DISABLED_mip_solve are intentionally disabled with the DISABLED_ prefix but lack a tracking reference; add a brief TODO comment above each test (or append a short marker to the test body) that includes a tracking identifier (e.g., "TODO: re-enable when CI fixed - GH#<issue-number>") and, if your project prefers, include the issue number in the test name (e.g., DISABLED_lp_solve_GH1234) so it's clear when and why these tests should be re-enabled; keep the existing test bodies and ensure the comment references the functions test_cpu_only_execution and test_cpu_only_mip_execution for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp`:
- Around line 385-400: The tests c_api_cpu_only::DISABLED_lp_solve and
c_api_cpu_only::DISABLED_mip_solve are intentionally disabled with the DISABLED_
prefix but lack a tracking reference; add a brief TODO comment above each test
(or append a short marker to the test body) that includes a tracking identifier
(e.g., "TODO: re-enable when CI fixed - GH#<issue-number>") and, if your project
prefers, include the issue number in the test name (e.g.,
DISABLED_lp_solve_GH1234) so it's clear when and why these tests should be
re-enabled; keep the existing test bodies and ensure the comment references the
functions test_cpu_only_execution and test_cpu_only_mip_execution for context.
In `@python/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.py`:
- Around line 171-172: The class-level pytest skip is fine but please add an
issue tracker reference to the skip reason so future maintainers know where to
follow up; update the pytestmark assignment (pytestmark = pytest.mark.skip(...))
in test_cpu_only_execution.py to include an issue ID or URL in the reason string
(e.g., "CPU-only tests temporarily disabled – see ISSUE-1234") so it’s easy to
find the re-enable task.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d14f5ae-aaeb-42f3-bc61-9dc8ccc0138d
📒 Files selected for processing (2)
cpp/tests/linear_programming/c_api_tests/c_api_tests.cpppython/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.py
|
/merge |
something has changed in the CI environment to cause cpu-only tests related to remote execution to fail, disabling these tests while we investigate root cause for the change Authors: - Trevor McKay (https://github.com/tmckayus) Approvers: - Ramakrishnap (https://github.com/rgsl888prabhu) - Rajesh Gandham (https://github.com/rg20) URL: NVIDIA#959
something has changed in the CI environment to cause cpu-only tests related to remote execution to fail, disabling these tests while we investigate root cause for the change