Skip to content

Commit a10aff5

Browse files
Copilotchhwang
andcommitted
Address code review feedback
- Move PerfTestResult struct definition outside vector declaration - Move getCurrentTimestamp to anonymous namespace - Add documentation for GTEST_SKIP macro explaining RAII pattern Co-authored-by: chhwang <8018170+chhwang@users.noreply.github.com>
1 parent 3d8a2e7 commit a10aff5

2 files changed

Lines changed: 16 additions & 4 deletions

File tree

test/framework.hpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,11 @@ void reportSuccess();
373373
} while (0)
374374

375375
// Helper class for GTEST_SKIP functionality
376+
// This class uses RAII (Resource Acquisition Is Initialization) pattern:
377+
// - The constructor records file and line information
378+
// - The stream operator (<<) allows appending a skip message
379+
// - The destructor throws an exception to skip the test
380+
// This enables usage like: GTEST_SKIP() << "Reason for skipping";
376381
class SkipHelper {
377382
public:
378383
explicit SkipHelper(const char* file, int line) : file_(file), line_(line) {}
@@ -397,6 +402,8 @@ class SkipHelper {
397402
std::ostringstream message_;
398403
};
399404

405+
// Test skip macro - throws exception to skip test execution
406+
// Usage: GTEST_SKIP() << "Optional skip message";
400407
#define GTEST_SKIP() ::mscclpp::test::SkipHelper(__FILE__, __LINE__)
401408

402409
// Create a namespace alias for compatibility with GTest code

test/perf/framework.cc

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,24 +11,29 @@
1111
namespace mscclpp {
1212
namespace test {
1313

14-
// Global state for performance test results
15-
static std::vector < struct PerfTestResult {
14+
// Performance test result structure
15+
struct PerfTestResult {
1616
std::string test_name;
1717
std::string test_category;
1818
std::map<std::string, std::string> test_params;
1919
nlohmann::ordered_json metrics;
2020
int num_processes;
2121
int process_rank;
2222
std::string timestamp;
23-
} > g_perf_results;
23+
};
24+
25+
// Global state for performance test results
26+
static std::vector<PerfTestResult> g_perf_results;
2427

25-
static std::string getCurrentTimestamp() {
28+
namespace {
29+
std::string getCurrentTimestamp() {
2630
auto now = std::chrono::system_clock::now();
2731
auto time_t = std::chrono::system_clock::to_time_t(now);
2832
std::stringstream ss;
2933
ss << std::put_time(std::gmtime(&time_t), "%Y-%m-%dT%H:%M:%S");
3034
return ss.str();
3135
}
36+
} // namespace
3237

3338
void recordResult(const std::string& test_name, const std::string& test_category, const nlohmann::ordered_json& metrics,
3439
const std::map<std::string, std::string>& test_params) {

0 commit comments

Comments
 (0)