testing: Refactor plugins gtest infra to support additional test cases #665
Conversation
|
👋 Hi mamar123! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/ok to test 79dc9d2 |
|
/build |
Signed-off-by: Menachem <[email protected]>
mamar123
force-pushed
the
plugins_gtest_enhance_pr
branch
from
79dc9d2 to
e25d9ed
Compare
|
/build |
|
/ok to test e25d9ed |
|
/build |
| } | ||
|
|
||
| TEST_P(setupObjTestFixture, XferMultiBufsTest) { | ||
| transferMemConfig mem_cfg{.numBufs_ = 3}; |
There was a problem hiding this comment.
Is 3 a must? if yes, please use a constant, otherwise a random value to make it clear
| const size_t numEntries_ = 1; | ||
| const size_t entrySize_ = 64; | ||
| const size_t numBufs_ = 1; | ||
| const uint8_t srcBufByte_ = getRandomInt(0, 255); | ||
| const uint8_t dstBufByte_ = getRandomInt(0, 255); |
There was a problem hiding this comment.
On second thought, not sure if const is the right choice when applied for everything.
This has several drawbacks e.g. impossible to assign.
If this configuration is local and we don't keep it - its fine to have the user decide.
| srcDevId_(0), | ||
| dstDevId_(isRemoteXfer_ ? 1 : 0) { |
There was a problem hiding this comment.
What does 0/1 means? should they be constants?
|
|
||
| void | ||
| setLocalMem() { | ||
| addSrcDesc(nixlMetaDesc &meta_desc) { |
| } | ||
|
|
||
| void | ||
| addDstDesc(nixlMetaDesc &meta_desc) { |
| nixl_status_t | ||
| waitForTransfer() { | ||
| nixl_status_t ret = NIXL_IN_PROG; | ||
| auto end_time = absl::Now() + absl::Seconds(3); |
There was a problem hiding this comment.
Why 3? maybe a constant?
| int entry_size = split_buf ? ENTRY_SIZE : BUF_SIZE; | ||
| for (size_t i = 0; i < srcMem_.size(); i++) { | ||
| for (int entry_i = 0; entry_i < num_entries; entry_i++) { | ||
| for (size_t entry_i = 0; entry_i < memConfig_.numEntries_; entry_i++) { |
There was a problem hiding this comment.
auto is easier to maintain
| while (ret == NIXL_IN_PROG && absl::Now() < end_time) { | ||
| ret = srcBackendEngine_->checkXfer(xferHandle_); | ||
| if (ret != NIXL_SUCCESS && ret != NIXL_IN_PROG) return ret; | ||
|
|
||
| if (dstBackendEngine_->supportsProgTh()) dstBackendEngine_->progress(); | ||
| } |
There was a problem hiding this comment.
ret == NIXL_IN_PROG in the condition isn't needed since you're only waiting for NIXL_SUCCESS or timeout
This loop can be rewritten as follows (in a more declarative way):
auto progress = { if (dstBackendEngine_->supportsProgTh()) dstBackendEngine_->progress(); };
do {
auto ret = srcBackendEngine_->checkXfer(xferHandle_);
if (ret != NIXL_SUCCESS && ret != NIXL_IN_PROG) return ret;
progress();
} while (absl::Now() < end_time);
4 participants
What?
Refactored plugins GTest infrastructure to support more flexible test cases and flows.
Why?
To make it easier to add and manage test variations (e.g., sizes, random data, edge cases) and reduce duplication.
How?
Used transferMemConfig for configuration, added support for random buffers, and simplified test setup logic.