Add large 1mb buffer test for crc32 hashing.#1913
Conversation
|
""" WalkthroughA new test fixture class, Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant crc32_large_buf
participant CRC32Function
TestRunner->>crc32_large_buf: SetUpTestSuite()
crc32_large_buf->>crc32_large_buf: Allocate 512KB buffer, fill with 0x55
TestRunner->>crc32_large_buf: Run hash(CRC32Function)
crc32_large_buf->>CRC32Function: Compute CRC32 on buffer
CRC32Function-->>crc32_large_buf: Return CRC32 hash
crc32_large_buf->>TestRunner: Assert hash == expected
TestRunner->>crc32_large_buf: TearDownTestSuite()
crc32_large_buf->>crc32_large_buf: Free buffer
""" Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
4362d1a to
9b3452b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1913 +/- ##
============================================
- Coverage 83.42% 39.40% -44.03%
============================================
Files 144 74 -70
Lines 12948 7883 -5065
Branches 2857 1303 -1554
============================================
- Hits 10802 3106 -7696
- Misses 1202 4538 +3336
+ Partials 944 239 -705 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/test_crc32.cc (1)
237-244: Consider adding allocation failure handling to the test setup.While the memory allocation and filling are correctly implemented, there's no check for allocation failure in
SetUpTestSuite().Add error handling to prevent segmentation faults if allocation fails:
static void SetUpTestSuite() { buffer = (uint8_t*)zng_alloc(buffer_size); + ASSERT_NE(buffer, nullptr) << "Failed to allocate large buffer for CRC32 tests"; memset(buffer, 0x55, buffer_size); }
🛑 Comments failed to post (1)
test/test_crc32.cc (1)
230-251:
⚠️ Potential issueGood implementation of the test fixture class for large buffer testing.
The
crc32_large_buftest fixture is well structured with static setup and teardown methods that properly allocate and free memory only once for all test cases. The buffer size of 1MB is appropriate for testing CRC32 performance with large inputs.One minor issue: the static constant
expected_hashneeds an external definition to prevent linker errors.The pipeline failure indicates an undefined reference to
crc32_large_buf::expected_hash. Static const members with in-class initializers still need definitions outside the class. Add this definition:uint8_t *crc32_large_buf::buffer = nullptr; +const uint32_t crc32_large_buf::expected_hash;Or alternatively, make it a constexpr:
- static const uint32_t expected_hash = 0x0026D5FB; + static constexpr uint32_t expected_hash = 0x0026D5FB;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./* Test large 1MB buffer with known CRC32 */ class crc32_large_buf : public ::testing::Test { protected: static uint8_t *buffer; static const size_t buffer_size = 1024 * 1024; - static const uint32_t expected_hash = 0x0026D5FB; + static constexpr uint32_t expected_hash = 0x0026D5FB; static void SetUpTestSuite() { buffer = (uint8_t*)zng_alloc(buffer_size); memset(buffer, 0x55, buffer_size); } static void TearDownTestSuite() { zng_free(buffer); } public: void hash(crc32_func crc32) { const uint32_t actual_hash = crc32(0, buffer, buffer_size); EXPECT_EQ(actual_hash, expected_hash); } };🤖 Prompt for AI Agents (early access)
In test/test_crc32.cc around lines 230 to 251, the static const member expected_hash is initialized inside the class but lacks an external definition, causing linker errors. Fix this by adding an out-of-class definition for expected_hash after the class definition or change its declaration to constexpr to allow inline initialization without needing an external definition.
4c664b3 to
0803b07
Compare
test/test_crc32.cc
Outdated
| const uint32_t crc32_large_buf::expected_hash; | ||
| uint8_t *crc32_large_buf::buffer = nullptr; |
There was a problem hiding this comment.
I don't see why these shouldn't be declared and initialized inside the class like buffer_size is. Also, crc32_large_buf::expected_hash is forward declaration that happens after the real declaration, which makes it redundant. The way buffer is initialized here can result in memory leak if SetupTestSuite is run more than once.
There was a problem hiding this comment.
I’m not really a C++ programmer so..
There was a problem hiding this comment.
The way buffer is initialized here can result in memory leak if SetupTestSuite is run more than once.
That would be a bug in Google test framework if it happened.
There was a problem hiding this comment.
Regarding forward declaring, the linker just complained and I did what it said so idk. 🤷♂️
There was a problem hiding this comment.
Maybe I need constexpr but I didn't want to be the first to introduce it.
There was a problem hiding this comment.
I’m not really a C++ programmer so..
Neither am I... I started with Z80 machine code and moved on to Pascal... I'm just saying that if I can't understand the logic, then most likely new contributors can't either... Without knowing the exact error messages, it will be hard to know if there is better solution... It's my personal opinion, but I really don't think we should introduce code that gets reverted or replaced right after it is committed... My professional opinion is that since this fails Emscripten WASM32 CI in a way that is unexpected, it should be rewritten...
According to clang 19, the construct is valid for C++17, but we set C++11 as minimum...
There was a problem hiding this comment.
diff --git a/test/test_crc32.cc b/test/test_crc32.cc
index 00abd5f6..dc8c497e 100644
--- a/test/test_crc32.cc
+++ b/test/test_crc32.cc
@@ -232,7 +232,7 @@ class crc32_large_buf : public ::testing::Test {
protected:
static uint8_t *buffer;
static const size_t buffer_size = 1024 * 1024;
- static const uint32_t expected_hash = 0x0026D5FB;
+ static const uint32_t expected_hash;
static void SetUpTestSuite() {
buffer = (uint8_t*)zng_alloc(buffer_size);
@@ -250,7 +250,8 @@ public:
}
};
-const uint32_t crc32_large_buf::expected_hash;
+// inline static declaration requires C++17
+const uint32_t crc32_large_buf::expected_hash = 0x0026D5FB;
uint8_t *crc32_large_buf::buffer = nullptr;
INSTANTIATE_TEST_SUITE_P(crc32, crc32_variant, testing::ValuesIn(tests));There was a problem hiding this comment.
Still unsure why buffer_size doesn't trigger warning or error...
There was a problem hiding this comment.
Grok told me it may be that EXPECT_EQ macro is referencing the expected_hash variable in such a way that it takes the address of it. So that is possibly why it wants it for expected_hash but not buffer_size. I don't care enough to find out for sure, but it seems plausable.
There was a problem hiding this comment.
I'm not really familiar with static const class members as I haven't really used C++17 features much... I did briefly read the relevant section of the standard, but that's quite confusing...
In my own code, I usually just use global non-member variables when static class members need to access same variables as non-static class members and there is no instance of the class that could be passed to the static member function.
0803b07 to
ee6d71a
Compare
f542646 to
77a79fe
Compare
|
@Dead2 this is ready to merge. |
Summary by CodeRabbit