Skip to content

testing: Add test scenarios for OBJ plugin gtest #666

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

testing: Add test scenarios for OBJ plugin gtest #666

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions test/gtest/plugins/obj_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,67 @@ TEST_P(setupObjTestFixture, queryMemTest) {
EXPECT_EQ(resp[2].has_value(), false);
}

TEST_P(setupObjTestFixture, writeWithOffsetsTest) {
transferMemConfig mem_cfg{.numEntries_ = 2};
Copy link
Contributor

@etoledano etoledano Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 2 a must? if so, please use a named constant, otherwise a random value.

transferHandler<DRAM_SEG, OBJ_SEG> transfer(
localBackendEngine_, localBackendEngine_, local_agent_name, local_agent_name, mem_cfg);
transfer.setupMems();

nixl_xfer_op_t op = NIXL_WRITE;
Copy link
Contributor

@etoledano etoledano Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why a stack variable assist here.

ASSERT_EQ(transfer.prepareTransfer(op), NIXL_SUCCESS);
ASSERT_EQ(transfer.postTransfer(op), NIXL_SUCCESS);
ASSERT_EQ(transfer.waitForTransfer(), NIXL_ERR_BACKEND);
}

TEST_P(setupObjTestFixture, readWithOffsetsTest) {
uint8_t src_byte = getRandomInt(0, 255);
Copy link
Contributor

@etoledano etoledano Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To have less hard coded numbers, I would suggest:

uint8_t src_byte = getRandomInt(0, std::numeric_limits<uint8_t>::max());


// First, write entire buffer to OBJ in a single chunk (no offsets)
transferMemConfig mem_cfg_write{.numEntries_ = 1, .entrySize_ = 128, .srcBufByte_ = src_byte};
Copy link
Contributor

@etoledano etoledano Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same note regarding 1, 128

transferHandler<DRAM_SEG, OBJ_SEG> transferWrite(localBackendEngine_,
localBackendEngine_,
local_agent_name,
local_agent_name,
mem_cfg_write);
transferWrite.setupMems();
transferWrite.setSrcMem();
transferWrite.testTransfer(NIXL_WRITE);

// Then, read buffer from OBJ in smaller offset-based chunks
transferMemConfig mem_cfg_read{.numEntries_ = 2, .entrySize_ = 64, .srcBufByte_ = src_byte};
transferHandler<DRAM_SEG, OBJ_SEG> transferRead(
localBackendEngine_, localBackendEngine_, local_agent_name, local_agent_name, mem_cfg_read);
transferRead.setupMems();
transferRead.resetSrcMem();
transferRead.testTransfer(NIXL_READ);
transferRead.checkSrcMem();
}

TEST_P(setupObjTestFixture, xferUnregisteredMemTest) {
transferHandler<DRAM_SEG, OBJ_SEG> transfer(
localBackendEngine_, localBackendEngine_, local_agent_name, local_agent_name);

nixlMetaDesc meta_desc;
meta_desc.devId = 1;
Copy link
Contributor

@etoledano etoledano Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 1 a must? if so please use named constant, otherwise a random value

transfer.addSrcDesc(meta_desc);
transfer.addDstDesc(meta_desc);

nixl_xfer_op_t op = NIXL_WRITE;
ASSERT_EQ(transfer.prepareTransfer(op), NIXL_SUCCESS);
ASSERT_EQ(transfer.postTransfer(op), NIXL_ERR_BACKEND);
}

TEST_P(setupObjTestFixture, objAsSrcMemTest) {
transferHandler<OBJ_SEG, OBJ_SEG> transfer(
localBackendEngine_, localBackendEngine_, local_agent_name, local_agent_name);
ASSERT_EQ(transfer.prepareTransfer(NIXL_WRITE), NIXL_ERR_INVALID_PARAM);
}

TEST_P(setupObjTestFixture, dramAsDstMemTest) {
transferHandler<DRAM_SEG, DRAM_SEG> transfer(
localBackendEngine_, localBackendEngine_, local_agent_name, local_agent_name);
ASSERT_EQ(transfer.prepareTransfer(NIXL_WRITE), NIXL_ERR_INVALID_PARAM);
}
Comment on lines +147 to +151
Copy link
Contributor

@etoledano etoledano Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(All around)

When you have a single place to use a class you can do that creating a temporary instance with shorter life span (gone after function call), i.e.:

TEST_P(setupObjTestFixture, dramAsDstMemTest) {
    ASSERT_EQ(transferHandler<DRAM_SEG, DRAM_SEG>(localBackendEngine_, localBackendEngine_, local_agent_name, local_agent_name).prepareTransfer(NIXL_WRITE), NIXL_ERR_INVALID_PARAM);
}


INSTANTIATE_TEST_SUITE_P(ObjTests, setupObjTestFixture, testing::Values(obj_test_params));

Expand Down
Loading