Skip to content

test(mooncake): fix -Werror violations exposed by release test builds#1815

Open
nv-nmailhot wants to merge 1 commit into
mainfrom
nmailhot/fix-mooncake-werror
Open

test(mooncake): fix -Werror violations exposed by release test builds#1815
nv-nmailhot wants to merge 1 commit into
mainfrom
nmailhot/fix-mooncake-werror

Conversation

@nv-nmailhot

@nv-nmailhot nv-nmailhot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

CI now compiles tests in release (-O3 -DNDEBUG), where assert() is a no-op, exposing pre-existing -Werror issues in mooncake_backend_test.cpp:

  • memType2Str / getValidationPtr fell off the end of non-void functions in the default case (assert(0) compiles out) -> add explicit returns.
  • desc.len used uninitialized in allocateWrongGPUTest -> initialize it.
  • ret/ret1/ret3_gen became unused (only read by compiled-out asserts) -> mark [[maybe_unused]].

Unblocks the AWS EFA validation, which builds + runs the C++ tests in release (.gitlab/build.sh + .gitlab/test_cpp.sh).

What?

Describe what this PR is doing.

Why?

Justification for the PR. If there is an existing issue/bug, please reference it. For
bug fixes, the 'Why?' and 'What?' can be merged into a single item.

How?

It is optional, but for complex PRs, please provide information about the design,
architecture, approach, etc.

Summary by CodeRabbit

  • Tests
    • Improved test reliability by ensuring functions return valid default values and reducing compiler warnings in test code.

CI now compiles tests in release (-O3 -DNDEBUG), where assert() is a no-op,
exposing pre-existing -Werror issues in mooncake_backend_test.cpp:
- memType2Str / getValidationPtr fell off the end of non-void functions in the
  default case (assert(0) compiles out) -> add explicit returns.
- desc.len used uninitialized in allocateWrongGPUTest -> initialize it.
- ret/ret1/ret3_gen became unused (only read by compiled-out asserts) -> mark
  [[maybe_unused]].

Unblocks the AWS EFA validation, which builds + runs the C++ tests in release
(.gitlab/build.sh + .gitlab/test_cpp.sh).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nv-nmailhot nv-nmailhot requested a review from a team as a code owner June 23, 2026 16:32
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 98b75b3c-528f-4194-8d25-faf9636f08b8

📥 Commits

Reviewing files that changed from the base of the PR and between b775042 and b4a0c88.

📒 Files selected for processing (1)
  • test/unit/plugins/mooncake/mooncake_backend_test.cpp

📝 Walkthrough

Walkthrough

In mooncake_backend_test.cpp, the memType2Str and getValidationPtr helper functions gain sentinel return values ("UNKNOWN" and NULL) in their default branches. Six status variables across five test functions are annotated with [[maybe_unused]] to suppress compiler warnings.

Changes

Mooncake backend test warning fixes

Layer / File(s) Summary
Sentinel return values in default branches
test/unit/plugins/mooncake/mooncake_backend_test.cpp
memType2Str default branch returns "UNKNOWN" and getValidationPtr default branch returns NULL, providing valid return values when assertions are compiled out.
[[maybe_unused]] annotations on status variables
test/unit/plugins/mooncake/mooncake_backend_test.cpp
ret, ret1, and ret3_gen status variables in allocateWrongGPUTest, allocateAndRegister, loadRemote, test_intra_agent_transfer, and test_inter_agent_transfer are annotated with [[maybe_unused]].

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A bunny hopped through warnings grim,
And stamped [[maybe_unused]] on a whim.
Two branches with no path to end
Now return their values, round the bend.
"UNKNOWN" and NULL — no fright,
The compiler hops along just right! 🐇✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description provides context on why the changes were needed and details what was fixed, but does not follow the template structure with explicit What/Why/How sections. Reorganize the description to clearly follow the template with distinct What, Why, and How sections for better clarity and consistency.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing -Werror violations in mooncake tests for release builds.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nmailhot/fix-mooncake-werror

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants