Add per-fixture isolation in GTEST for MsQuicLib() and some much needed de-duplications.#6149
Add per-fixture isolation in GTEST for MsQuicLib() and some much needed de-duplications.#6149ProjectsByJackHe wants to merge 4 commits into
Conversation
ProjectsByJackHe
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6149 +/- ##
==========================================
+ Coverage 84.49% 84.76% +0.27%
==========================================
Files 60 60
Lines 18973 18974 +1
==========================================
+ Hits 16031 16084 +53
+ Misses 2942 2890 -52 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mtfriesen
left a comment
mtfriesen
left a comment
There was a problem hiding this comment.
Generally looks great to me, but please get a second set of eyeballs.
guhetier
left a comment
guhetier
left a comment
There was a problem hiding this comment.
Some initial comments at a quick review pass.
The general approach looks good.
| // | ||
| // Fixture classes for plain test suites (TEST_F). | ||
| // | ||
| class ParameterValidation : public QuicTestFixture {}; |
There was a problem hiding this comment.
Why do we need this? Is there a difference with using QuicTestFixture directly? Is it just a question of naming of the test categories?
If that is the reason why, consider moving this line close to the definition of the test that use it
| // Macros that generate the standard test body boilerplate: | ||
| // logger + kernel-mode dispatch + user-mode call. | ||
| // | ||
| #define QUIC_TEST_F(Suite, Name, Func) \ |
There was a problem hiding this comment.
I initially avoided to factor this in a macro because it can't be used in 100% of cases: some tests are UM only or need some extra parameter processing.
It makes it harder to understand why QUIC_TEST_F or TEST_F is used on a specific test since the logic is hidden far away in the file.
If we go for an extra layer of wrapping macro, let's make the name more explicit:
- QUIC is irrelevant here, the test could be about anything
- I'd like to see UM and KM appear in the name, to clarify it does some dispatch between the two. Maybe "TEST_UM_KM_F"? Better idea welcome...
| TestLogger Logger(#Func); \ | ||
| if (TestingKernelMode) { \ | ||
| ASSERT_TRUE(InvokeKernelTest(FUNC(Func))); \ | ||
| } else { Func(); } \ |
| ::testing::ValuesIn(WithXdpMapModeArgs::Generate())); | ||
| #endif // _WIN32 && QUIC_API_ENABLE_PREVIEW_FEATURES | ||
|
|
||
| QUIC_TEST_F(ParameterValidation, ValidateCommonParam, QuicTestCommonParam) |
There was a problem hiding this comment.
nit: Consider using the test function as a name, instead of having to parameters that are very similar most of the time.
It might be harder if the same function is reused in multiple tests, but I think I already removed all of those.
| ASSERT_TRUE(InvokeKernelTest(FUNC(QuicTestRegistrationParam))); | ||
| } else { | ||
| QuicTestRegistrationParam(); | ||
| static bool SuiteSkip; |
There was a problem hiding this comment.
From the gtest doc (https://google.github.io/googletest/primer.html#same-data-multiple-tests), I don't think these should be static.
The test can access them.
| SuiteFailureReason = nullptr; | ||
|
|
||
| if (TestingKernelMode) { | ||
| SuiteSkip = true; |
There was a problem hiding this comment.
Can't you just call GTEST_SKIP from here?
| } | ||
| #if defined(_WIN32) && defined(QUIC_API_ENABLE_PREVIEW_FEATURES) | ||
| struct WithXdpMapModeArgs : public QuicTestFixture, | ||
| public ::testing::WithParamInterface<XdpMapModeArgs> { |
There was a problem hiding this comment.
Are the test params used in the fixture itself?
If no, I recommend to split them. Keep the fixture class focused on setup only.
Then add parameter in a new type inheriting from that XDP-map-mode fixture and the params.
It'll make it easier to add tests with different param sets in the future.
| XdpMapState.InterfaceCount = (uint32_t)IfIndices.size(); | ||
| memcpy(XdpMapState.IfIndices, IfIndices.data(), | ||
| sizeof(uint32_t) * IfIndices.size()); | ||
| printf("WithXdpMapModeArgs: discovered %u DuoNic interface(s)\n", |
There was a problem hiding this comment.
Can we avoid printf and use logging mechanism already in use?
3 participants
Description
Removes the --useXdpMaps flag by adding behavior to the Gtest suite to teardown/set up a new MsQuicLib() per test fixture, and also refactor the code to remove code duplication.
Key changes:
Testing
See the CI
Documentation
No documentation impact.