Repository navigation
test(obj): Increase unit test coverage for Dell ObjectScale accelerated engine - #1630
Merged
Merged
Conversation
|
👋 Hi jgoldsch12! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shared OBJ test infrastructure and mocks, a conditional meson build entry, and a new cuobjclient (Dell/RDMA) test source exercising RDMA flows, memory registration, input validation, edge cases, and async behaviors. ChangesOBJ Accelerated Engine Tests
Sequence Diagram(s)sequenceDiagram
participant Test as Obj Tests
participant Engine as nixlObjEngine
participant S3Mock as mockDellS3Client
participant Executor as Aws::Executor
Test->>Engine: setupEngine(params)
Engine->>S3Mock: putObjectRdmaAsync / getObjectRdmaAsync / checkObjectExistsAsync
S3Mock->>Executor: queue callback (optional per-key delay)
Executor-->>Engine: execute queued callback (success/error)
Engine-->>Test: prepXfer/checkXfer results (NIXL_IN_PROG / NIXL_SUCCESS / errors)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/gtest/unit/obj/obj_test_base.h`:
- Around line 17-18: The header guard NIXL_TEST_OBJ_TEST_BASE_H must be replaced
with the full repository-relative uppercase macro
NIXL_TEST_GTEST_UNIT_OBJ_OBJ_TEST_BASE_H (update the `#ifndef` and `#define`) and
update the trailing `#endif` comment to match the new macro; also search and
update any other occurrences (e.g., the duplicate at 531) to keep them
consistent.
- Around line 163-170: The execAsync() helper currently dereferences executor_
without checking for null; update execAsync() so that if executor_ is non-null
it submits each pendingCallbacks_ to executor_->Submit(...) and then calls
executor_->waitUntilIdle(), but if executor_ is null it simply runs each
callback inline (e.g., invoke callback() directly) instead of submitting, then
clear pendingCallbacks_; ensure you reference the existing symbols execAsync,
executor_, pendingCallbacks_, Submit and waitUntilIdle in the change.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 20138775-972f-47b2-b4af-e8717ad5362a
📒 Files selected for processing (3)
test/gtest/unit/obj/meson.buildtest/gtest/unit/obj/obj_cuobj.cpptest/gtest/unit/obj/obj_test_base.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/gtest/unit/obj/obj_test_base.h`:
- Around line 463-514: The testTransferFailure helper should assert that a
failed read does not mutate the destination buffer: before calling
prepXfer/postXfer save a copy of test_buffer (or its contents) and after
mockS3Client_->execAsync() and the failing checkXfer verify (only when operation
== NIXL_READ) that test_buffer still equals the saved copy (all 'Z'); update
testTransferFailure to perform that comparison after EXPECT_NE(status,
NIXL_SUCCESS) and before releasing/deregistering resources.
- Around line 330-342: The nixlBlobDesc instances (e.g., local_desc and
remote_desc in the obj_test_base.h test setup) are partially uninitialized;
explicitly value-initialize the structs (or set addr and len to 0 and other
fields as appropriate) before assigning specific fields like devId and metaInfo
to avoid leaving addr/len uninitialized. Locate the nixlBlobDesc declarations
(local_desc, remote_desc and the other nixlBlobDesc instances elsewhere in the
file) and either use aggregate/value-initialization ({}) or explicitly set addr
= 0 and len = 0 (and clear metaInfo if needed) prior to further assignments and
calls such as objEngine_->registerMem to ensure all members are initialized.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c133fece-ab9e-4dec-88f3-ae867fe1ac37
📒 Files selected for processing (1)
test/gtest/unit/obj/obj_test_base.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/gtest/unit/obj/obj_test_base.h`:
- Around line 511-514: The current failed-read check only inspects
test_buffer[0], risking missed partial mutations; when operation == NIXL_READ,
verify the entire destination buffer is still filled with 'Z' by comparing all
buffer_size bytes (use test_buffer and buffer_size) rather than just index 0 so
any partial writes are caught.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1358e84d-6b85-45af-94f8-54c9de9b8803
📒 Files selected for processing (1)
test/gtest/unit/obj/obj_test_base.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@test/gtest/unit/obj/obj_test_base.h`:
- Around line 511-514: The assertion block checking that a failed read didn't
mutate the destination buffer is not formatted to match the project's
clang-format rules; run the code formatter over the block containing
EXPECT_TRUE(std::all_of(...)) that references operation, NIXL_READ, and
test_buffer so the lambda and line breaks match the repo style (i.e., reflow the
EXPECT_TRUE/std::all_of assertion and its lambda) and commit the formatted
changes.
- Around line 82-93: The mock exposes per-key controls (setKeyOutcome,
setKeyDelay, setKeyError) but putObjectAsync/getObjectAsync currently ignore the
key and only use simulateSuccess_; update putObjectAsync and getObjectAsync to
consult keyOutcomes_, keyDelays_, and keyErrors_ for the provided key (use
keyErrors_.count(key) to force failure, keyDelays_[key] to delay, and
keyOutcomes_[key] to decide success), falling back to simulateSuccess_ when no
per-key entry exists; alternatively, if you prefer to keep the mock simple,
remove the per-key setters and their storage (keyOutcomes_, keyDelays_,
keyErrors_) so the API surface matches simulateSuccess_ behavior.
- Around line 507-509: After calling mockS3Client_->execAsync(), don't just
assert status != NIXL_SUCCESS because checkXfer(handle) may still return
NIXL_IN_PROG; instead, poll or wait until objEngine_->checkXfer(handle) returns
a terminal status (i.e., not NIXL_IN_PROG) and then assert that the terminal
status is a failure (not NIXL_SUCCESS). In other words, after
mockS3Client_->execAsync() repeatedly call objEngine_->checkXfer(handle) until
the returned status != NIXL_IN_PROG, then use EXPECT_NE(status, NIXL_SUCCESS)
(or an explicit EXPECT_EQ to the expected failure code) to ensure a terminal
failure was observed.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 12ce0668-e3f0-41e5-8736-cfec190efe0a
📒 Files selected for processing (1)
test/gtest/unit/obj/obj_test_base.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/gtest/unit/obj/obj_test_base.h (2)
76-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor the full per-key transfer controls in the async transfer mocks.
setKeyDelay(),setKeyError(), andsetSimulateError()still don't affectputObjectAsync()/getObjectAsync(). Right now onlykeyOutcomes_is consulted, so tests cannot model delayed transfer completion or per-key transfer errors even though the mock API advertises both. That leaves the transfer-path coverage weaker than the helper surface suggests.Also applies to: 81-93, 102-137
🤖 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 `@test/gtest/unit/obj/obj_test_base.h` around lines 76 - 79, The async transfer mocks currently ignore per-key controls (setKeyDelay, setKeyError, setSimulateError) and only consult keyOutcomes_, so putObjectAsync() and getObjectAsync() never simulate delays or per-key errors; update the async scheduling and completion code in putObjectAsync/getObjectAsync to consult the per-key maps (keyDelay_, keyError_, simulateError_) before using keyOutcomes_, apply the delay from setKeyDelay(), inject the configured error from setKeyError() or setSimulateError() into the async completion callback/path, and ensure any existing fallback to keyOutcomes_ remains if no per-key control is present; reference keyOutcomes_, setKeyDelay(), setKeyError(), setSimulateError(), putObjectAsync(), getObjectAsync() when making the changes.
384-386:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the full read buffers, not just byte 0.
These success helpers still only inspect the first byte after a read. That misses partial-write regressions and will read out of bounds when a zero-length buffer is passed through these size-based helpers.
Also applies to: 463-466
🤖 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 `@test/gtest/unit/obj/obj_test_base.h` around lines 384 - 386, The helper currently only checks test_buffer[0] for NIXL_READ, which misses partial-write regressions and can dereference on zero-length buffers; update the read-success checks (referencing test_buffer and the operation == NIXL_READ branch) to validate the entire buffer length (use the provided length/size parameter and compare all bytes, e.g. a loop or memcmp/std::equal against the expected pattern like 'A') and first guard against zero length before indexing; make the same change for the duplicate check around lines showing the same pattern (the other EXPECT_EQ usage referenced).
🤖 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 `@test/gtest/unit/obj/obj_test_base.h`:
- Around line 519-523: The polling loop using mockS3Client_->execAsync() and
repeatedly calling objEngine_->checkXfer(handle) can hang if checkXfer keeps
returning NIXL_IN_PROG; add a bounded retry/deadline (e.g., max iterations or a
timeout) around the do/while and break when the cap is reached, then assert that
the final status is a terminal state (not NIXL_IN_PROG) before checking
EXPECT_NE(status, NIXL_SUCCESS); ensure you still verify terminal status !=
NIXL_SUCCESS and fail the test with a clear assertion if the deadline/retry cap
was hit while still NIXL_IN_PROG.
---
Duplicate comments:
In `@test/gtest/unit/obj/obj_test_base.h`:
- Around line 76-79: The async transfer mocks currently ignore per-key controls
(setKeyDelay, setKeyError, setSimulateError) and only consult keyOutcomes_, so
putObjectAsync() and getObjectAsync() never simulate delays or per-key errors;
update the async scheduling and completion code in putObjectAsync/getObjectAsync
to consult the per-key maps (keyDelay_, keyError_, simulateError_) before using
keyOutcomes_, apply the delay from setKeyDelay(), inject the configured error
from setKeyError() or setSimulateError() into the async completion
callback/path, and ensure any existing fallback to keyOutcomes_ remains if no
per-key control is present; reference keyOutcomes_, setKeyDelay(),
setKeyError(), setSimulateError(), putObjectAsync(), getObjectAsync() when
making the changes.
- Around line 384-386: The helper currently only checks test_buffer[0] for
NIXL_READ, which misses partial-write regressions and can dereference on
zero-length buffers; update the read-success checks (referencing test_buffer and
the operation == NIXL_READ branch) to validate the entire buffer length (use the
provided length/size parameter and compare all bytes, e.g. a loop or
memcmp/std::equal against the expected pattern like 'A') and first guard against
zero length before indexing; make the same change for the duplicate check around
lines showing the same pattern (the other EXPECT_EQ usage referenced).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2045766e-9b7a-419f-8ce1-c9daf961e993
📒 Files selected for processing (1)
test/gtest/unit/obj/obj_test_base.h
…accelerated engine - Add obj_test_base.h with shared test fixtures and mocks - Add obj_cuobj.cpp with Dell-specific accelerated engine tests - Update meson.build to conditionally build obj_cuobj.cpp when cuobjclient is available These tests require the cuobjclient library provided by CUDA 13.1 and a cufile.json configuration. Signed-off-by: Jason Goldschmidt <jason.goldschmidt@dell.com> test(obj): Fix header guard naming convention and execAsync null check - Update header guard from NIXL_TEST_OBJ_TEST_BASE_H to NIXL_TEST_GTEST_UNIT_OBJ_OBJ_TEST_BASE_H to follow repo convention - Fix execAsync() to check for null executor_ before dereferencing - When executor_ is null, callbacks are invoked inline instead of submitted Signed-off-by: Jason Goldschmidt <jason.goldschmidt@dell.com> test(obj): Fix buffer mutation check and struct initialization - Add buffer mutation verification in testTransferFailure for READ operations - Value-initialize all nixlBlobDesc instances to avoid uninitialized members - Specifically fix remote_desc instances in testTransferWithSize, testMultiDescriptorWithSizes, and testTransferFailure Signed-off-by: Jason Goldschmidt <jason.goldschmidt@dell.com> test(obj): Verify entire destination buffer in failed-read check Changed buffer mutation check from inspecting only test_buffer[0] to verifying all bytes with std::all_of, catching partial writes that could be missed by single-byte check. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> test(obj): Fix mock per-key controls and polling in testTransferFailure - Run clang-format on buffer mutation check assertion to match project style - Update putObjectAsync/getObjectAsync to use per-key outcome controls (keyOutcomes_) matching the pattern in checkObjectExistsAsync - Add polling loop after execAsync() to wait for terminal status before asserting failure, avoiding false positives when checkXfer returns NIXL_IN_PROG Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> test(obj): Add polling timeout, per-key controls, and full buffer validation - Add bounded polling loop (max 100 iterations) in testTransferFailure to prevent hanging if checkXfer keeps returning NIXL_IN_PROG - Update putObjectAsync/getObjectAsync to use per-key delay and error controls (keyDelays_, keyErrors_) matching the pattern in checkObjectExistsAsync - Fix testTransferWithSize read validation to check entire buffer instead of only test_buffer[0], guarding against zero-length buffers Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
jgoldsch12
force-pushed
the
dell_test_coverage_pr
branch
from
May 11, 2026 16:24
bdddb17 to
0ba4752
Compare
Contributor
Author
|
@aranadive I opened this pull request to replace the one which was closed. This is ready to merge. |
Contributor
|
/ok to test be91244 |
aranadive
approved these changes
May 14, 2026
Contributor
|
/build |
2 similar comments
Contributor
|
/build |
Contributor
|
/build |
Contributor
|
/ok to test 7b3d4db |
Contributor
|
/build |
Contributor
|
/build |
Contributor
|
/ok to test 48abb1c |
Contributor
|
/build |
Contributor
|
/ok to test 8150372 |
Contributor
|
/build |
realAaronWu
added a commit
to realAaronWu/nixl
that referenced
this pull request
May 21, 2026
… fixes Refactor OBJ unit tests to support both standard S3 and Dell/Accel engine configurations, integrating tests from PR ai-dynamo#1630 while fixing compilation and correctness issues for the Pattern B refactored codebase. Key changes: - Extract shared test infrastructure into obj_test_base.h: - ObjTestConfig with Standard, CRT, Accel, and Dell configurations - Enhanced mockS3Client with per-key outcomes, delays, and error injection - mockDellS3Client using iS3Client (not deleted iDellS3RdmaClient) - objTestBase, objParamTestFixture, and objTestFixture base classes - Add obj_cuobj.cpp for Dell/Accel-specific tests (conditionally compiled): - 18 objDellTestFixture tests covering RDMA read/write, memory registration, transfer failures, descriptor validation, and object segment management - 14 CuObjTokenManagerTest tests for token generation edge cases - Parameterized ObjAccelClientTests suite (Accel + Dell configs) - Simplify obj.cpp by removing duplicated infrastructure and all HAVE_CUOBJ_CLIENT guarded code, now handled in obj_cuobj.cpp - Update meson.build to conditionally compile obj_cuobj.cpp when cuobj_dep is found Fixes from PR ai-dynamo#1630 original tests: - Remove rdma_interface.h include (deleted in Pattern B) - Fix mock client inheritance (iS3Client, not iDellS3RdmaClient) - Fix registerMem expectations: mock path (tokenMgr_=null) returns NIXL_SUCCESS with nullptr metadata, not backend errors - Fix prepXfer vs postXfer validation: devId-to-key mapping is checked in postXfer, not prepXfer - Remove 3 tests with untestable expectations (descriptor count mismatch, unregistered cuObject address, large memory offset)
Contributor
|
/ok to test 7240a85 |
Contributor
|
/build |
realAaronWu
added a commit
to realAaronWu/nixl
that referenced
this pull request
May 27, 2026
…place-callback-only Resolve conflicts in test/gtest/unit/obj/obj_cuobj.cpp and test/gtest/unit/obj/obj_test_base.h caused by PR ai-dynamo#1630 landing on main. Resolution strategy: - Keep HEAD's CuObjTokenManager architecture (no iDellS3RdmaClient, no mockDellS3Client, no rdma_interface.h) - Keep HEAD's mock behavior: single mockS3Client for all engine types, DRAM/VRAM registerMem returns nullptr metadata when tokenMgr_ is null - Keep HEAD's validation semantics: devId registration checked in postXfer, not prepXfer - Keep HEAD's exceptionThrowingMockS3Client and CuObjTokenManager integration tests - Adopt origin/main's value-initialization (= {}) for nixlBlobDesc - Adopt origin/main's polling loop and buffer verification in testTransferFailure - Adopt origin/main's DellEngineMismatchedDescriptorCounts test (parent-class prepXfer validation, independent of mock design)
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These tests require the cuobjclient library provided by CUDA 13.1 and a cufile.json configuration.
What?
Adds unit test cases
Why?
To increase unit test coverage and protect against regression
How?
Filled gaps in unit test coverage using mocks where necessary
Summary by CodeRabbit