refactor(grpc): expose public blocking client, move internal-only tests to mock suite#1570
refactor(grpc): expose public blocking client, move internal-only tests to mock suite#1570ramakrishnap-nv wants to merge 2 commits into
Conversation
ramakrishnap-nv
added
non-breaking
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe gRPC integration tests now use public blocking remote solve APIs, environment-based host/port and TLS configuration, direct channel readiness checks, and restart/error assertions. Low-level client lifecycle tests are removed, while raw chunk-validation coverage remains. gRPC integration tests
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cpp/include/cuopt/grpc/grpc_cpp_client.hpp (2)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
<cstdint>directly.
int64_t/int32_tare used directly in this header's public API (stream_logs,get_incumbents) but<cstdint>isn't included here — it's only available transitively viagrpc_client_types.hpp.As per coding guidelines, C++ headers should "follow Include What You Use."♻️ Proposed fix
`#include` <functional> `#include` <memory> `#include` <string> +#include <cstdint>Also applies to: 102-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cuopt/grpc/grpc_cpp_client.hpp` around lines 14 - 16, Add a direct <cstdint> include to grpc_cpp_client.hpp, which publicly uses int64_t and int32_t in stream_logs and get_incumbents. Do not rely on the transitive inclusion from grpc_client_types.hpp.Source: Coding guidelines
43-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd per-method Doxygen docs to match the internal client's documentation level.
The class has a good class-level doc block, but individual public methods (
connect,solve_lp,submit_lp,get_incumbents, etc.) have no@brief/@param/@returntags, unlikegrpc_client_tingrpc_client.hpp, which documents every method this class forwards to.As per path instructions, "New public functions/classes need Doxygen-style documentation" for
cpp/include/cuopt/**/*.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cuopt/grpc/grpc_cpp_client.hpp` around lines 43 - 113, Add Doxygen-style documentation to every public method in grpc_cpp_client_t, including constructors, connection, solve, job lifecycle, result retrieval, and logging/incumbent APIs. Document each method’s purpose with `@brief`, all parameters with `@param`, and return semantics with `@return`, matching the corresponding grpc_client_t documentation and terminology. Keep the existing API signatures and behavior unchanged.Source: Path instructions
cpp/include/cuopt/grpc/grpc_client_types.hpp (1)
32-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument callback thread-safety and the
chunked_array_threshold_bytessentinel.
log_callback/incumbent_callback/debug_log_callbackare invoked from a background streaming thread in the internal client (pergrpc_client.hpp's "Background log streaming" comment), but that contract isn't documented here where callers configure them. Also,chunked_array_threshold_bytes = -1has no comment explaining that-1means "auto-compute frommax_message_bytes" (confirmed by test usage forcing0/large values to select unary vs. chunked paths).As per path instructions, "Suggest documenting thread-safety, GPU requirements, numerical behavior" for public headers under
cpp/include/cuopt/**/*.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cuopt/grpc/grpc_client_types.hpp` around lines 32 - 57, Document in grpc_client_config_t that log_callback, incumbent_callback, and debug_log_callback execute on the internal background streaming thread and must be thread-safe, including any relevant callback constraints. Add a field comment for chunked_array_threshold_bytes stating that -1 enables automatic computation from max_message_bytes, while nonnegative values explicitly control the threshold.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/grpc/client/grpc_cpp_client.cpp`:
- Around line 84-94: Remove the duplicate explicit instantiations for <int,
double> and <int32_t, double> associated with grpc_cpp_client_t::get_lp_result
and get_mip_result, retaining only one specialization per result type. If int is
the sole supported index type, also make these wrapper methods non-templated
consistently with solve_lp, solve_mip, submit_lp, and submit_mip.
---
Nitpick comments:
In `@cpp/include/cuopt/grpc/grpc_client_types.hpp`:
- Around line 32-57: Document in grpc_client_config_t that log_callback,
incumbent_callback, and debug_log_callback execute on the internal background
streaming thread and must be thread-safe, including any relevant callback
constraints. Add a field comment for chunked_array_threshold_bytes stating that
-1 enables automatic computation from max_message_bytes, while nonnegative
values explicitly control the threshold.
In `@cpp/include/cuopt/grpc/grpc_cpp_client.hpp`:
- Around line 14-16: Add a direct <cstdint> include to grpc_cpp_client.hpp,
which publicly uses int64_t and int32_t in stream_logs and get_incumbents. Do
not rely on the transitive inclusion from grpc_client_types.hpp.
- Around line 43-113: Add Doxygen-style documentation to every public method in
grpc_cpp_client_t, including constructors, connection, solve, job lifecycle,
result retrieval, and logging/incumbent APIs. Document each method’s purpose
with `@brief`, all parameters with `@param`, and return semantics with `@return`,
matching the corresponding grpc_client_t documentation and terminology. Keep the
existing API signatures and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0b570ce5-b118-4a9a-aa34-08cb0665a5dc
📒 Files selected for processing (8)
cpp/CMakeLists.txtcpp/include/cuopt/grpc/grpc_client_env.hppcpp/include/cuopt/grpc/grpc_client_types.hppcpp/include/cuopt/grpc/grpc_cpp_client.hppcpp/src/grpc/client/grpc_client.hppcpp/src/grpc/client/grpc_cpp_client.cppcpp/tests/linear_programming/grpc/CMakeLists.txtcpp/tests/linear_programming/grpc/grpc_integration_test.cpp
💤 Files with no reviewable changes (1)
- cpp/tests/linear_programming/grpc/CMakeLists.txt
| template <typename i_t, typename f_t> | ||
| remote_lp_result_t<i_t, f_t> grpc_cpp_client_t::get_lp_result(const std::string& job_id) | ||
| { | ||
| return impl_->client.get_lp_result<i_t, f_t>(job_id); | ||
| } | ||
|
|
||
| template <typename i_t, typename f_t> | ||
| remote_mip_result_t<i_t, f_t> grpc_cpp_client_t::get_mip_result(const std::string& job_id) | ||
| { | ||
| return impl_->client.get_mip_result<i_t, f_t>(job_id); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'int32_t.*=.*int;|typedef int int32_t' /usr/include 2>/dev/null | head -20
rg -n 'get_(lp|mip)_result<int32_t|get_(lp|mip)_result<int,' cpp/src/grpc/client/*.cpp cpp/src/grpc/client/*.hppRepository: NVIDIA/cuopt
Length of output: 929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- grpc_cpp_client.cpp (instantiation region) ---'
sed -n '100,125p' cpp/src/grpc/client/grpc_cpp_client.cpp
echo
echo '--- int32_t / int type relationship probe ---'
cat >/tmp/int32_probe.cpp <<'CPP'
`#include` <cstdint>
`#include` <type_traits>
static_assert(std::is_same_v<int32_t, int>, "int32_t is not int");
int main() {}
CPP
if command -v g++ >/dev/null 2>&1; then
g++ -std=c++17 -c /tmp/int32_probe.cpp -o /tmp/int32_probe.o && echo 'g++: int32_t == int'
else
echo 'g++ not available'
fi
echo
echo '--- duplicate explicit instantiation probe ---'
cat >/tmp/dup_inst_probe.cpp <<'CPP'
template <typename T>
T f();
template int f<int>();
template int f<std::int32_t>();
int main() {}
CPP
if command -v g++ >/dev/null 2>&1; then
g++ -std=c++17 -c /tmp/dup_inst_probe.cpp -o /tmp/dup_inst_probe.o
else
echo 'g++ not available'
fiRepository: NVIDIA/cuopt
Length of output: 1280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- grpc_cpp_client.cpp (instantiation region) ---'
sed -n '100,125p' cpp/src/grpc/client/grpc_cpp_client.cpp
echo
echo '--- int32_t / int type relationship probe ---'
cat >/tmp/int32_probe.cpp <<'CPP'
`#include` <cstdint>
`#include` <type_traits>
static_assert(std::is_same_v<int32_t, int>, "int32_t is not int");
int main() {}
CPP
if command -v g++ >/dev/null 2>&1; then
g++ -std=c++17 -c /tmp/int32_probe.cpp -o /tmp/int32_probe.o && echo 'g++: int32_t == int'
else
echo 'g++ not available'
fi
echo
echo '--- duplicate explicit instantiation probe ---'
cat >/tmp/dup_inst_probe.cpp <<'CPP'
`#include` <cstdint>
template <typename T>
T f();
template int f<int>();
template int f<std::int32_t>();
int main() {}
CPP
if command -v g++ >/dev/null 2>&1; then
g++ -std=c++17 -c /tmp/dup_inst_probe.cpp -o /tmp/dup_inst_probe.o
else
echo 'g++ not available'
fiRepository: NVIDIA/cuopt
Length of output: 1280
🌐 Web query:
C++ duplicate explicit instantiation same specialization ill-formed explicit instantiation definition once per program int32_t int alias
💡 Result:
In C++, an explicit instantiation definition for a given template specialization must appear at most once in a program [1][2][3]. If multiple explicit instantiation definitions for the same specialization are provided across different translation units, the program is ill-formed [2][3]. While implementations are not required to diagnose this violation (making it a "no diagnostic required" or NDR rule), it violates the one-definition rule (ODR) and can lead to unpredictable behavior, such as linker errors or silent bugs [1][4][3]. Regarding the use of type aliases like int32_t versus int, the C++ standard does not treat them as identical types even if they resolve to the same underlying primitive type on a specific implementation [5][6]. Explicit instantiation relies on the template arguments matching exactly [2][3]. If you explicitly instantiate a template using int for one translation unit and int32_t for another, they are considered different specializations [2][6]. Consequently, you are effectively creating two distinct instantiations rather than duplicating the same one. However, if the compiler or linker eventually determines these are the same entity, it may cause conflicts, but they are technically separate template arguments [2][5]. To summarize: 1. Explicit instantiation definition: Must appear at most once per program for a specific set of template arguments [1][2]. 2. Multiple definitions: Providing the same explicit instantiation definition in multiple translation units makes the program ill-formed (NDR) [4][3]. 3. Type aliases (int32_t vs int): These are distinct types [6]. Using them as template arguments creates different specializations [2]. Whether the resulting program is valid depends on whether those distinct specializations are intended and whether they satisfy the ODR if they are eventually resolved to the same underlying entity [5][6].
Citations:
- 1: https://eel.is/c++draft/temp.spec
- 2: https://timsong-cpp.github.io/cppwp/n4950/temp.spec.general
- 3: https://stackoverflow.com/questions/64910370/explicitly-instantiate-template-method-of-explicitly-instantiated-template-class
- 4: https://stackoverflow.com/questions/65613231/compiler-segfault-during-explicit-template-instantiation-on-clang-11
- 5: https://stackoverflow.com/questions/36875583/how-legal-is-this-definition-of-main
- 6: https://stackoverflow.com/questions/32822153/when-int-isnt-an-int-intx-t
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in /usr/include/stdint.h /usr/include/x86_64-linux-gnu/bits/stdint-intn.h /usr/include/c++/*/cstdint; do
[ -e "$f" ] || continue
echo "--- $f ---"
rg -n 'typedef.*int32_t|using.*int32_t|int32_t' "$f" || true
doneRepository: NVIDIA/cuopt
Length of output: 150
Remove the duplicate explicit instantiations in cpp/src/grpc/client/grpc_cpp_client.cpp:111-118.
<int, double> and <int32_t, double> resolve to the same specialization here, so only one explicit instantiation should remain per result type. If int is the only supported index type, make these wrappers non-templated to match solve_lp/solve_mip/submit_lp/submit_mip.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/grpc/client/grpc_cpp_client.cpp` around lines 84 - 94, Remove the
duplicate explicit instantiations for <int, double> and <int32_t, double>
associated with grpc_cpp_client_t::get_lp_result and get_mip_result, retaining
only one specialization per result type. If int is the sole supported index
type, also make these wrapper methods non-templated consistently with solve_lp,
solve_mip, submit_lp, and submit_mip.
CI Test Summary✅ All 13 test job(s) passed. |
Replace the internal grpc_client_t (and the interim grpc_cpp_client_t
wrapper) with the already-public solve_lp_remote() / solve_mip_remote()
API in GRPC_INTEGRATION_TEST. Each fixture sets CUOPT_REMOTE_HOST /
CUOPT_REMOTE_PORT via setenv() so the public functions can reach the
per-test server without any internal client class.
Changes:
- Delete grpc_cpp_client_t (grpc_cpp_client.hpp, grpc_client_types.hpp,
grpc_cpp_client.cpp) — unnecessary abstraction
- Revert grpc_client.hpp and grpc_client_env.hpp to their pre-PR state
- Remove src/grpc/client from GRPC_INTEGRATION_TEST include paths
- Rewrite grpc_integration_test.cpp to use only the public API:
• ServerProcess::wait_for_ready uses raw gRPC channel connectivity
• GrpcIntegrationTestBase sets/clears env vars in SetUp/TearDown
• TLS / mTLS tests use CUOPT_TLS_* env vars (cert file paths)
• ChunkedUploadTests sets CUOPT_MAX_MESSAGE_BYTES=4096 to trigger
the chunked transport path for medium-sized problems
• QCQP end-to-end tests kept (functional value independent of path)
• ChunkValidationTests kept unchanged (raw gRPC stub)
- Remove tests that require the internal async lifecycle API:
submit_lp/mip, check_status, cancel_job, delete_job, stream_logs,
get_result, debug_log_callback, log_callback, incumbent_callback
This eliminates any static-library linkage risk and makes the test
binary depend only on the public cuopt shared library.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ramakrishnap-nv
force-pushed
the
fix/grpc-tests-use-public-api
branch
from
4e9cba7 to
660a01f
Compare
ramakrishnap-nv
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/tests/linear_programming/grpc/grpc_integration_test.cpp (1)
741-754: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWeak assertion: only checks non-null, not solve outcome.
QuadraticConstraintsNonZeroRhsassertssolution != nullptronly, while the adjacentQuadraticConstraintsEndToEndSocp(Lines 759-786) fully validates termination status and closed-form values. See consolidated comment below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp` around lines 741 - 754, Strengthen QuadraticConstraintsNonZeroRhs by validating the returned solution’s termination status and expected closed-form values, matching the comprehensive assertions used by QuadraticConstraintsEndToEndSocp. Retain the existing problem-loading and non-null check, and use the solution fields and expected results appropriate for QC_Test_1.Source: Path instructions
🧹 Nitpick comments (5)
cpp/tests/linear_programming/grpc/grpc_integration_test.cpp (5)
979-986: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
read_file_contents()helper — see consolidated comment.Identical to the copy in
TlsServerTests(Lines 886-893).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp` around lines 979 - 986, The read_file_contents helper is duplicated in the surrounding test code. Remove the duplicate definition and reuse the existing read_file_contents implementation from TlsServerTests, updating callers only as needed to reference that shared helper.
276-296: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse one
grpc::Channelacross the polling loop instead of recreating it each iteration.Each
wait_for_ready()iteration (up to ~75 times over the 15s startup timeout) constructs a brand-newgrpc::Channeland, for TLS, re-parsesSslCredentialsOptionsfrom scratch. Creating the channel once outside the loop and repeatedly callingWaitForConnected()with a fresh deadline on the same channel is more idiomatic gRPC usage (it also lets gRPC's own connection/backoff state persist across polls) and avoids the repeat cert-parsing overhead.♻️ Proposed refactor
bool wait_for_ready(int timeout_ms) { auto start = std::chrono::steady_clock::now(); + + std::shared_ptr<grpc::ChannelCredentials> creds; + if (!tls_root_certs_.empty()) { + grpc::SslCredentialsOptions ssl_opts; + ssl_opts.pem_root_certs = tls_root_certs_; + ssl_opts.pem_cert_chain = tls_client_cert_; + ssl_opts.pem_private_key = tls_client_key_; + creds = grpc::SslCredentials(ssl_opts); + } else { + creds = grpc::InsecureChannelCredentials(); + } + auto channel = grpc::CreateChannel("localhost:" + std::to_string(port_), std::move(creds)); while (true) { auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>( std::chrono::steady_clock::now() - start); if (elapsed.count() >= timeout_ms) { return false; } - std::shared_ptr<grpc::ChannelCredentials> creds; - if (!tls_root_certs_.empty()) { - grpc::SslCredentialsOptions ssl_opts; - ssl_opts.pem_root_certs = tls_root_certs_; - ssl_opts.pem_cert_chain = tls_client_cert_; - ssl_opts.pem_private_key = tls_client_key_; - creds = grpc::SslCredentials(ssl_opts); - } else { - creds = grpc::InsecureChannelCredentials(); - } - - auto channel = - grpc::CreateChannel("localhost:" + std::to_string(port_), std::move(creds)); auto deadline = std::chrono::system_clock::now() + std::chrono::milliseconds(200); if (channel->WaitForConnected(deadline)) { return true; } int status = 0; if (waitpid(pid_, &status, WNOHANG) == pid_) { std::cerr << "Server process died during startup\n"; return false; } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp` around lines 276 - 296, Refactor wait_for_ready() so the grpc::Channel and its credentials are created once before the polling loop, preserving the existing TLS and insecure credential selection. In each iteration, compute a fresh deadline and call WaitForConnected() on the shared channel, while retaining the existing process-death checks and timeout behavior.
709-737: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate concurrent-solve helper logic — see consolidated comment.
Same 3-thread solve/atomic-count pattern as
DefaultServerTests::ConcurrentSolves(Lines 583-611), only the server fixture differs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp` around lines 709 - 737, Remove the duplicated 3-thread solve and atomic success-count logic from ChunkedUploadTests::ConcurrentSolves and reuse the existing concurrent-solve helper established by DefaultServerTests::ConcurrentSolves. Preserve the chunked-upload fixture/server setup and the expectation that all three solves succeed.
583-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate concurrent-solve helper logic — see consolidated comment.
This 3-thread solve/atomic-count pattern is duplicated almost verbatim in
ChunkedUploadTests::ConcurrentSolves(Lines 709-737). Flagged in the consolidated comment below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp` around lines 583 - 611, Extract the shared 3-thread remote-solve and success-count logic from DefaultServerTests::ConcurrentSolves and ChunkedUploadTests::ConcurrentSolves into a reusable test helper. Update both tests to call the helper while preserving the existing problem, settings, objective validation, exception handling, and expected success count.
886-893: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
read_file_contents()helper — see consolidated comment.Identical to the copy in
MtlsServerTests(Lines 979-986).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp` around lines 886 - 893, The read_file_contents helper is duplicated in this test file. Remove the duplicate near the current location and reuse the existing read_file_contents definition from MtlsServerTests, updating callers only if needed to reference that shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp`:
- Around line 698-707: Strengthen the ChunkedUploadTests.ChunkedUploadMIP
assertion by checking solution->get_termination_status() against the expected
successful solve status, while retaining the non-null assertion before
dereferencing solution. Match the termination-status validation used by sibling
MIP integration tests.
- Around line 809-826: Strengthen SolveMIPAfterServerRestart by validating that
both returned solutions represent successful, feasible solves, not merely
non-null pointers. After each solve_mip_remote call, assert the solution’s
termination status and feasibility using the existing solution fields or helper
assertions used elsewhere in the tests, while preserving the restart sequence
and failure messages.
---
Outside diff comments:
In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp`:
- Around line 741-754: Strengthen QuadraticConstraintsNonZeroRhs by validating
the returned solution’s termination status and expected closed-form values,
matching the comprehensive assertions used by QuadraticConstraintsEndToEndSocp.
Retain the existing problem-loading and non-null check, and use the solution
fields and expected results appropriate for QC_Test_1.
---
Nitpick comments:
In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp`:
- Around line 979-986: The read_file_contents helper is duplicated in the
surrounding test code. Remove the duplicate definition and reuse the existing
read_file_contents implementation from TlsServerTests, updating callers only as
needed to reference that shared helper.
- Around line 276-296: Refactor wait_for_ready() so the grpc::Channel and its
credentials are created once before the polling loop, preserving the existing
TLS and insecure credential selection. In each iteration, compute a fresh
deadline and call WaitForConnected() on the shared channel, while retaining the
existing process-death checks and timeout behavior.
- Around line 709-737: Remove the duplicated 3-thread solve and atomic
success-count logic from ChunkedUploadTests::ConcurrentSolves and reuse the
existing concurrent-solve helper established by
DefaultServerTests::ConcurrentSolves. Preserve the chunked-upload fixture/server
setup and the expectation that all three solves succeed.
- Around line 583-611: Extract the shared 3-thread remote-solve and
success-count logic from DefaultServerTests::ConcurrentSolves and
ChunkedUploadTests::ConcurrentSolves into a reusable test helper. Update both
tests to call the helper while preserving the existing problem, settings,
objective validation, exception handling, and expected success count.
- Around line 886-893: The read_file_contents helper is duplicated in this test
file. Remove the duplicate near the current location and reuse the existing
read_file_contents definition from MtlsServerTests, updating callers only if
needed to reference that shared helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b9b10330-45af-4f23-8480-9bc30abcf288
📒 Files selected for processing (2)
cpp/tests/linear_programming/grpc/CMakeLists.txtcpp/tests/linear_programming/grpc/grpc_integration_test.cpp
💤 Files with no reviewable changes (1)
- cpp/tests/linear_programming/grpc/CMakeLists.txt
ramakrishnap-nv
changed the title
- Move gRPC channel/credentials creation outside the wait_for_ready() polling loop (created once; WaitForConnected called per iteration) - Extract duplicate read_file_contents() from TlsServerTests and MtlsServerTests into a shared anonymous-namespace free function - Strengthen ChunkedUploadMIP: check termination_status == Optimal - Strengthen SolveMIPAfterServerRestart: use ASSERT_NE (prevents null deref) and check termination_status == Optimal on both solves Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
Do we have any code coverage utilities to verify that we're not losing coverage from this change? |
We do run static analysis, but it catches mostly security threats and not anything else, may be we need to add one. |
2 participants
Add
grpc_client_config_tas a public header and a slim blocking clientgrpc_cpp_client_t(pimpl over the internalgrpc_client_t) so thatGRPC_INTEGRATION_TESTno longer needs internal headers. Tests exercising the async job lifecycle (submit/check_status/cancel/delete) are removed from the integration test — they are already covered byGRPC_CLIENT_TESTmock tests.StreamLogs_NotFoundis added toGRPC_CLIENT_TEST. Fixes the pre-existing bad relative include ingrpc_client_env.hpp.