14 write comm tests #17
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit tests for the communication layer (fixes #14). The changes introduce tests for both MPI and VT communication backends, verify basic communication operations (send/poll), and test reduce operations with various data types.
- Added new test file with typed tests covering send/receive and reduction operations
- Fixed macro in test helpers to work with typed test fixtures
- Corrected memory access pattern in proxy wrapper reduce callback
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/unit/test_helpers.h | Updated SET_MIN_NUM_NODES_CONSTRAINT macro to use this->comm for compatibility with typed test fixtures |
| tests/unit/comm/test_comm.cc | New test file implementing unit tests for communication primitives including send/poll dispatch and various reduce operations (int, double, float arrays) |
| src/vt-lb/comm/vt/proxy_wrapper.impl.h | Fixed reduce callback to correctly access container data using .at(0) instead of taking address of container object itself |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //@HEADER | ||
| // ***************************************************************************** | ||
| // | ||
| // test_comm.h |
There was a problem hiding this comment.
The header comment indicates this file is named "test_comm.h", but the actual filename is "test_comm.cc". The comment should be corrected to match the actual filename.
| // test_comm.h | |
| // test_comm.cc |
| using ValT = typename T::value_type; | ||
| static_assert(std::is_trivially_copyable_v<ValT> || std::is_arithmetic_v<ValT>, "Reduce value must be trivially copyable"); | ||
| std::memcpy(ctx->out_ptr, std::addressof(val), sizeof(ValT) * std::max<std::size_t>(1, ctx->count)); | ||
| std::memcpy(ctx->out_ptr, std::addressof(val.at(0)), sizeof(ValT) * std::max<std::size_t>(1, ctx->count)); |
There was a problem hiding this comment.
[nitpick] While std::addressof(val.at(0)) is functionally correct, using val.data() would be more idiomatic for obtaining a pointer to the container's underlying data. For example: std::memcpy(ctx->out_ptr, val.data(), sizeof(ValT) * std::max<std::size_t>(1, ctx->count));
| std::memcpy(ctx->out_ptr, std::addressof(val.at(0)), sizeof(ValT) * std::max<std::size_t>(1, ctx->count)); | |
| std::memcpy(ctx->out_ptr, val.data(), sizeof(ValT) * std::max<std::size_t>(1, ctx->count)); |
| #define SET_MIN_NUM_NODES_CONSTRAINT(min_req_num_nodes) \ | ||
| { \ | ||
| auto const num_nodes = comm.numRanks(); \ | ||
| auto const num_nodes = this->comm.numRanks(); \ |
There was a problem hiding this comment.
The SET_MAX_NUM_NODES_CONSTRAINT macro on line 118 uses comm.numRanks() without the this-> prefix, while SET_MIN_NUM_NODES_CONSTRAINT was updated to use this->comm.numRanks(). For consistency, this macro should also be updated to use this->comm.numRanks().
Fixes #14