refactor(test): add macros for quantizer tests to reduce code duplication#1738
refactor(test): add macros for quantizer tests to reduce code duplication#1738
Conversation
LHT129
added
kind/improvement
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces macros to simplify quantizer test definitions and reduce code duplication. The primary goal is to make the test code more maintainable and easier to understand, while preserving the existing test coverage. The changes involve adding macro definitions in Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Refactors quantizer unit tests by introducing reusable Catch2 macro helpers to reduce duplication, then migrates FP32 quantizer tests to the new macros.
Changes:
- Added a set of
DEFINE_QUANTIZER_*macros inquantizer_test.hto generate common encode/decode, compute, and serialization test cases. - Refactored
fp32_quantizer_test.cppto use the new macros and centralize test parameters (dims/counts/error).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/quantization/quantizer_test.h | Adds macro-based helpers that generate Catch2 test cases for multiple metrics/dims/counts. |
| src/quantization/fp32_quantizer_test.cpp | Replaces duplicated FP32 quantizer test logic with the new macro helpers. |
| #include "data_type.h" | ||
| #include "fixtures.h" | ||
| #include "impl/allocator/safe_allocator.h" | ||
| #include "iostream" |
There was a problem hiding this comment.
iostream is a standard library header and should be included with angle brackets (#include <iostream>). Using quotes can accidentally pick up a project-local header named iostream, leading to confusing build behavior.
| #include "iostream" | |
| #include <iostream> |
| @@ -15,81 +14,16 @@ | |||
|
|
|||
There was a problem hiding this comment.
This file now uses std::vector but does not include <vector> directly. Please add #include <vector> to avoid relying on transitive includes (which can break with unrelated header changes).
| #include <vector> |
| const auto dims = std::vector<int>{64, 128}; | ||
| const auto counts = std::vector<int>{10, 101}; |
There was a problem hiding this comment.
This file now uses std::vector but does not include <vector> directly. Please add #include <vector> to avoid relying on transitive includes (which can break with unrelated header changes).
src/quantization/quantizer_test.h
Outdated
| constexpr MetricType metrics[] = {MetricType::METRIC_TYPE_L2SQR, MetricType::METRIC_TYPE_COSINE, MetricType::METRIC_TYPE_IP}; \ | ||
| for (auto dim : dims_val) { \ | ||
| for (auto count : counts_val) { \ | ||
| for (auto metric : metrics) { \ | ||
| auto allocator = SafeAllocator::FactoryDefaultAllocator(); \ | ||
| if (metric == MetricType::METRIC_TYPE_L2SQR) { \ | ||
| QuantizerT<MetricType::METRIC_TYPE_L2SQR> quantizer(dim, allocator.get()); \ | ||
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_L2SQR>, MetricType::METRIC_TYPE_L2SQR>(quantizer, dim, count, error_val); \ | ||
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_L2SQR>, MetricType::METRIC_TYPE_L2SQR>(quantizer, dim, count, error_val); \ | ||
| } else if (metric == MetricType::METRIC_TYPE_COSINE) { \ | ||
| QuantizerT<MetricType::METRIC_TYPE_COSINE> quantizer(dim, allocator.get()); \ | ||
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_COSINE>, MetricType::METRIC_TYPE_COSINE>(quantizer, dim, count, error_val); \ | ||
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_COSINE>, MetricType::METRIC_TYPE_COSINE>(quantizer, dim, count, error_val); \ | ||
| } else { \ | ||
| QuantizerT<MetricType::METRIC_TYPE_IP> quantizer(dim, allocator.get()); \ | ||
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_IP>, MetricType::METRIC_TYPE_IP>(quantizer, dim, count, error_val); \ | ||
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_IP>, MetricType::METRIC_TYPE_IP>(quantizer, dim, count, error_val); \ |
There was a problem hiding this comment.
Several macro lines are very long and hard to read/review (and likely violate the project’s 100-char line length). Consider wrapping the repeated test invocations into small helper functions/templates (e.g., per-metric helpers) so the macro only declares the TEST_CASE and calls a readable helper, or split the template arguments across multiple lines using additional line continuations.
| constexpr MetricType metrics[] = {MetricType::METRIC_TYPE_L2SQR, MetricType::METRIC_TYPE_COSINE, MetricType::METRIC_TYPE_IP}; \ | |
| for (auto dim : dims_val) { \ | |
| for (auto count : counts_val) { \ | |
| for (auto metric : metrics) { \ | |
| auto allocator = SafeAllocator::FactoryDefaultAllocator(); \ | |
| if (metric == MetricType::METRIC_TYPE_L2SQR) { \ | |
| QuantizerT<MetricType::METRIC_TYPE_L2SQR> quantizer(dim, allocator.get()); \ | |
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_L2SQR>, MetricType::METRIC_TYPE_L2SQR>(quantizer, dim, count, error_val); \ | |
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_L2SQR>, MetricType::METRIC_TYPE_L2SQR>(quantizer, dim, count, error_val); \ | |
| } else if (metric == MetricType::METRIC_TYPE_COSINE) { \ | |
| QuantizerT<MetricType::METRIC_TYPE_COSINE> quantizer(dim, allocator.get()); \ | |
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_COSINE>, MetricType::METRIC_TYPE_COSINE>(quantizer, dim, count, error_val); \ | |
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_COSINE>, MetricType::METRIC_TYPE_COSINE>(quantizer, dim, count, error_val); \ | |
| } else { \ | |
| QuantizerT<MetricType::METRIC_TYPE_IP> quantizer(dim, allocator.get()); \ | |
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_IP>, MetricType::METRIC_TYPE_IP>(quantizer, dim, count, error_val); \ | |
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_IP>, MetricType::METRIC_TYPE_IP>(quantizer, dim, count, error_val); \ | |
| constexpr MetricType metrics[] = { \ | |
| MetricType::METRIC_TYPE_L2SQR, \ | |
| MetricType::METRIC_TYPE_COSINE, \ | |
| MetricType::METRIC_TYPE_IP, \ | |
| }; \ | |
| for (auto dim : dims_val) { \ | |
| for (auto count : counts_val) { \ | |
| for (auto metric : metrics) { \ | |
| auto allocator = SafeAllocator::FactoryDefaultAllocator(); \ | |
| if (metric == MetricType::METRIC_TYPE_L2SQR) { \ | |
| QuantizerT<MetricType::METRIC_TYPE_L2SQR> quantizer(dim, allocator.get()); \ | |
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_L2SQR>, \ | |
| MetricType::METRIC_TYPE_L2SQR>(quantizer, dim, count, \ | |
| error_val); \ | |
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_L2SQR>, \ | |
| MetricType::METRIC_TYPE_L2SQR>(quantizer, dim, count, \ | |
| error_val); \ | |
| } else if (metric == MetricType::METRIC_TYPE_COSINE) { \ | |
| QuantizerT<MetricType::METRIC_TYPE_COSINE> quantizer(dim, allocator.get()); \ | |
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_COSINE>, \ | |
| MetricType::METRIC_TYPE_COSINE>(quantizer, dim, count, \ | |
| error_val); \ | |
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_COSINE>, \ | |
| MetricType::METRIC_TYPE_COSINE>(quantizer, dim, count, \ | |
| error_val); \ | |
| } else { \ | |
| QuantizerT<MetricType::METRIC_TYPE_IP> quantizer(dim, allocator.get()); \ | |
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_IP>, \ | |
| MetricType::METRIC_TYPE_IP>(quantizer, dim, count, \ | |
| error_val); \ | |
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_IP>, \ | |
| MetricType::METRIC_TYPE_IP>(quantizer, dim, count, \ | |
| error_val); \ |
src/quantization/quantizer_test.h
Outdated
| } | ||
|
|
||
| #define DEFINE_QUANTIZER_ENCODE_DECODE_TESTS(TestName, QuantizerT, dims_val, counts_val, error_val, code_max_val) \ | ||
| TEST_CASE(TestName " Encode and Decode", "[ut][" TestName "]") { \ |
There was a problem hiding this comment.
DEFINE_QUANTIZER_ENCODE_DECODE_TESTS and DEFINE_QUANTIZER_ENCODE_DECODE_TESTS_SIMPLE generate the exact same Catch2 test case name (TestName " Encode and Decode"). If a quantizer test file ever uses both macros with the same TestName, this can create duplicate/ambiguous test names. Consider differentiating the names (e.g., " Encode and Decode (Same)" vs " Encode and Decode") or adding a macro parameter for the suffix.
| TEST_CASE(TestName " Encode and Decode", "[ut][" TestName "]") { \ | |
| TEST_CASE(TestName " Encode and Decode (Same)", "[ut][" TestName "]") { \ |
src/quantization/quantizer_test.h
Outdated
| } | ||
|
|
||
| #define DEFINE_QUANTIZER_ENCODE_DECODE_TESTS_SIMPLE(TestName, QuantizerT, dims_val, counts_val, error_val) \ | ||
| TEST_CASE(TestName " Encode and Decode", "[ut][" TestName "]") { \ |
There was a problem hiding this comment.
DEFINE_QUANTIZER_ENCODE_DECODE_TESTS and DEFINE_QUANTIZER_ENCODE_DECODE_TESTS_SIMPLE generate the exact same Catch2 test case name (TestName " Encode and Decode"). If a quantizer test file ever uses both macros with the same TestName, this can create duplicate/ambiguous test names. Consider differentiating the names (e.g., " Encode and Decode (Same)" vs " Encode and Decode") or adding a macro parameter for the suffix.
| TEST_CASE(TestName " Encode and Decode", "[ut][" TestName "]") { \ | |
| TEST_CASE(TestName " Encode and Decode (Simple)", "[ut][" TestName "]") { \ |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively reduces code duplication in quantizer tests by introducing macros. The approach is sound. I have two suggestions for improvement: one to further reduce duplication within the newly created macros for better maintainability, and another to address a potential reduction in test coverage for fp32_quantizer_test.cpp.
| } | ||
| } | ||
| } | ||
| DEFINE_QUANTIZER_ENCODE_DECODE_TESTS_SIMPLE("FP32Quantizer", FP32Quantizer, dims, counts, error) |
There was a problem hiding this comment.
The original test code for FP32Quantizer's "Encode and Decode" test included a call to TestQuantizerEncodeDecodeSame. By using DEFINE_QUANTIZER_ENCODE_DECODE_TESTS_SIMPLE, this test is now omitted, which reduces test coverage. To maintain the original test coverage, you should use DEFINE_QUANTIZER_ENCODE_DECODE_TESTS and provide the code_max_val (65536) that was used previously.
| DEFINE_QUANTIZER_ENCODE_DECODE_TESTS_SIMPLE("FP32Quantizer", FP32Quantizer, dims, counts, error) | |
| DEFINE_QUANTIZER_ENCODE_DECODE_TESTS("FP32Quantizer", FP32Quantizer, dims, counts, error, 65536) |
src/quantization/quantizer_test.h
Outdated
| #define DEFINE_QUANTIZER_COMPUTE_TESTS_WITH_SAME(TestName, QuantizerT, dims_val, counts_val, error_val, code_max_val) \ | ||
| TEST_CASE(TestName " Compute", "[ut][" TestName "]") { \ | ||
| constexpr MetricType metrics[] = {MetricType::METRIC_TYPE_L2SQR, MetricType::METRIC_TYPE_COSINE, MetricType::METRIC_TYPE_IP}; \ | ||
| for (auto dim : dims_val) { \ | ||
| for (auto count : counts_val) { \ | ||
| for (auto metric : metrics) { \ | ||
| auto allocator = SafeAllocator::FactoryDefaultAllocator(); \ | ||
| if (metric == MetricType::METRIC_TYPE_L2SQR) { \ | ||
| QuantizerT<MetricType::METRIC_TYPE_L2SQR> quantizer(dim, allocator.get()); \ | ||
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_L2SQR>, MetricType::METRIC_TYPE_L2SQR>(quantizer, dim, count, error_val); \ | ||
| TestComputeCodesSame<QuantizerT<MetricType::METRIC_TYPE_L2SQR>, MetricType::METRIC_TYPE_L2SQR>(quantizer, dim, count, code_max_val); \ | ||
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_L2SQR>, MetricType::METRIC_TYPE_L2SQR>(quantizer, dim, count, error_val); \ | ||
| } else if (metric == MetricType::METRIC_TYPE_COSINE) { \ | ||
| QuantizerT<MetricType::METRIC_TYPE_COSINE> quantizer(dim, allocator.get()); \ | ||
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_COSINE>, MetricType::METRIC_TYPE_COSINE>(quantizer, dim, count, error_val); \ | ||
| TestComputeCodesSame<QuantizerT<MetricType::METRIC_TYPE_COSINE>, MetricType::METRIC_TYPE_COSINE>(quantizer, dim, count, code_max_val); \ | ||
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_COSINE>, MetricType::METRIC_TYPE_COSINE>(quantizer, dim, count, error_val); \ | ||
| } else { \ | ||
| QuantizerT<MetricType::METRIC_TYPE_IP> quantizer(dim, allocator.get()); \ | ||
| TestComputeCodes<QuantizerT<MetricType::METRIC_TYPE_IP>, MetricType::METRIC_TYPE_IP>(quantizer, dim, count, error_val); \ | ||
| TestComputeCodesSame<QuantizerT<MetricType::METRIC_TYPE_IP>, MetricType::METRIC_TYPE_IP>(quantizer, dim, count, code_max_val); \ | ||
| TestComputer<QuantizerT<MetricType::METRIC_TYPE_IP>, MetricType::METRIC_TYPE_IP>(quantizer, dim, count, error_val); \ | ||
| } \ | ||
| } \ | ||
| } \ | ||
| } \ | ||
| } |
There was a problem hiding this comment.
The macro definitions contain a lot of duplicated code in the if/else if/else blocks for handling different metric types. This can be simplified by extracting the common logic into a helper structure with a templated static method, defined within the TEST_CASE to avoid name clashes. This improves maintainability and readability.
This suggestion applies to all the new macros defined in this file.
#define DEFINE_QUANTIZER_COMPUTE_TESTS_WITH_SAME(TestName, QuantizerT, dims_val, counts_val, error_val, code_max_val) \
TEST_CASE(TestName " Compute", "[ut][" TestName "]") { \
struct TestRunner { \
template <MetricType metric> \
static void run(int dim, int count, float error, int code_max) { \
auto allocator = SafeAllocator::FactoryDefaultAllocator(); \
QuantizerT<metric> quantizer(dim, allocator.get()); \
TestComputeCodes<QuantizerT<metric>, metric>(quantizer, dim, count, error); \
TestComputeCodesSame<QuantizerT<metric>, metric>(quantizer, dim, count, code_max); \
TestComputer<QuantizerT<metric>, metric>(quantizer, dim, count, error); \
} \
}; \
constexpr MetricType metrics[] = {MetricType::METRIC_TYPE_L2SQR, MetricType::METRIC_TYPE_COSINE, MetricType::METRIC_TYPE_IP}; \
for (auto dim : dims_val) { \
for (auto count : counts_val) { \
for (auto metric : metrics) { \
if (metric == MetricType::METRIC_TYPE_L2SQR) { \
TestRunner::run<MetricType::METRIC_TYPE_L2SQR>(dim, count, error_val, code_max_val); \
} else if (metric == MetricType::METRIC_TYPE_COSINE) { \
TestRunner::run<MetricType::METRIC_TYPE_COSINE>(dim, count, error_val, code_max_val); \
} else { \
TestRunner::run<MetricType::METRIC_TYPE_IP>(dim, count, error_val, code_max_val); \
} \
} \
} \
} \
}
LHT129
force-pushed
the
yellow/quantization-test-template-refactor
branch
from
de2395e to
f823f4c
Compare
| #define DEFINE_QUANTIZER_ENCODE_DECODE_TESTS( \ | ||
| TestName, QuantizerT, dims_val, counts_val, error_val, code_max_val) \ | ||
| TEST_CASE(TestName " Encode and Decode", "[ut][" TestName "]") { \ | ||
| constexpr MetricType metrics[] = {MetricType::METRIC_TYPE_L2SQR, \ | ||
| MetricType::METRIC_TYPE_IP}; \ | ||
| for (auto dim : dims_val) { \ | ||
| for (auto count : counts_val) { \ | ||
| for (auto metric : metrics) { \ | ||
| auto allocator = SafeAllocator::FactoryDefaultAllocator(); \ | ||
| if (metric == MetricType::METRIC_TYPE_L2SQR) { \ | ||
| QuantizerT<MetricType::METRIC_TYPE_L2SQR> quantizer(dim, allocator.get()); \ | ||
| TestQuantizerEncodeDecode(quantizer, dim, count, error_val); \ |
There was a problem hiding this comment.
The newly added test-definition macros contain multiple lines that significantly exceed the repo’s configured 100-column limit, which makes them hard to read and can fight clang-format. Consider wrapping long template/type lines and/or factoring repeated branch bodies into a helper macro/lambda to keep macro lines within the column limit.
| } | ||
| } | ||
| } | ||
| DEFINE_QUANTIZER_ENCODE_DECODE_TESTS_SIMPLE("FP32Quantizer", FP32Quantizer, dims, counts, error) |
There was a problem hiding this comment.
This refactor drops the TestQuantizerEncodeDecodeSame(...) coverage that previously ran for FP32 encode/decode (the new _SIMPLE macro only calls TestQuantizerEncodeDecode). If that check is still desired, switch to DEFINE_QUANTIZER_ENCODE_DECODE_TESTS and pass code_max=65536 so the same-behavior assertions remain equivalent to the old test.
| } | ||
| } | ||
| DEFINE_QUANTIZER_ENCODE_DECODE_TESTS_SIMPLE("FP32Quantizer", FP32Quantizer, dims, counts, error) | ||
| DEFINE_QUANTIZER_COMPUTE_TESTS_WITH_SAME("FP32Quantizer", FP32Quantizer, dims, counts, error, 65536) |
There was a problem hiding this comment.
This macro invocation line is over the 100-column limit; please wrap it across multiple lines for readability and to match the repository formatting configuration.
| DEFINE_QUANTIZER_COMPUTE_TESTS_WITH_SAME("FP32Quantizer", FP32Quantizer, dims, counts, error, 65536) | |
| DEFINE_QUANTIZER_COMPUTE_TESTS_WITH_SAME("FP32Quantizer", | |
| FP32Quantizer, | |
| dims, | |
| counts, | |
| error, | |
| 65536) |
| #include "data_type.h" | ||
| #include "fixtures.h" | ||
| #include "impl/allocator/safe_allocator.h" |
There was a problem hiding this comment.
quantizer_test.h is used as a header across many test translation units, but it has no include guard (#pragma once or #ifndef/#define). This can cause ODR/redefinition errors if it ever gets included transitively more than once in a TU (especially now that it also defines macros). Add an include guard at the top of this file to make inclusion safe and consistent with the rest of the codebase’s headers (e.g., src/quantization/quantizer.h uses #pragma once).
…tion Introduce test macros to simplify quantizer test cases and reduce boilerplate code. Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
LHT129
force-pushed
the
yellow/quantization-test-template-refactor
branch
from
f823f4c to
59f3263
Compare
Summary
Add macro definitions in
quantizer_test.hto simplify quantizer test definitions and reduce code duplication across multiple test files.Changes
DEFINE_QUANTIZER_ENCODE_DECODE_TESTSmacro for encode/decode testsDEFINE_QUANTIZER_ENCODE_DECODE_TESTS_SIMPLEmacro for simple encode/decode testsDEFINE_QUANTIZER_COMPUTE_TESTSmacro for compute testsDEFINE_QUANTIZER_COMPUTE_TESTS_WITH_SAMEmacro for compute tests with TestComputeCodesSameDEFINE_QUANTIZER_SERIALIZE_TESTSmacro for serialize/deserialize testsfp32_quantizer_test.cppto use new macros (95 lines → 28 lines, 70% reduction)Technical Details
The macros use runtime metric selection (if/else) instead of
TEMPLATE_TEST_CASEsinceMetricTypeis an enum value, not a type. This approach:Testing
All tests pass:
Files Changed
src/quantization/quantizer_test.h- Added 5 macros for test definitionsrc/quantization/fp32_quantizer_test.cpp- Refactored to use macrosChecklist