pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2429738842


##########
cpp/src/arrow/util/bpacking_test.cc:
##########
@@ -201,52 +198,116 @@ class TestUnpack : public 
::testing::TestWithParam<TestUnpackSize> {
 
   template <typename Int>
   void TestAll(UnpackFunc<Int> unpack) {
-    // Known values
-    TestUnpackZeros(unpack);
-    TestUnpackOnes(unpack);
-    TestUnpackAlternating(unpack);
-
-    // Roundtrips
-    TestRoundtripAlignment(unpack, /* alignment_offset= */ 0);
-    TestRoundtripAlignment(unpack, /* alignment_offset= */ 1);
+    constexpr int kMaxBitWidth = std::is_same_v<Int, bool> ? 1 : 8 * 
sizeof(Int);
+    // Given how many edge cases there are in unpacking integers, it is best 
to test all
+    // sizes
+    for (int bit_width = 0; bit_width <= kMaxBitWidth; ++bit_width) {
+      SCOPED_TRACE(::testing::Message() << "Testing bit_width=" << bit_width);
+
+      // Known values
+      TestUnpackZeros(unpack, bit_width);
+      TestUnpackOnes(unpack, bit_width);
+      TestUnpackAlternating(unpack, bit_width);
+
+      // Roundtrips
+      TestRoundtripAlignment(unpack, bit_width, /* alignment_offset= */ 0);
+      TestRoundtripAlignment(unpack, bit_width, /* alignment_offset= */ 1);
+    }
   }
 };
 
-INSTANTIATE_TEST_SUITE_P(
-    UnpackMultiplesOf64Values, TestUnpack,
-    ::testing::Values(TestUnpackSize{64, 1}, TestUnpackSize{128, 1},
-                      TestUnpackSize{2048, 1}, TestUnpackSize{64, 31},
-                      TestUnpackSize{128, 31}, TestUnpackSize{2048, 1},
-                      TestUnpackSize{2048, 8}, TestUnpackSize{2048, 13},
-                      TestUnpackSize{2048, 16}, TestUnpackSize{2048, 31},
-                      TestUnpackSize{2048, 32}));
+// There are actually many differences across the different sizes.
+// It is best to test them all.
+INSTANTIATE_TEST_SUITE_P(UnpackMultiplesOf64Values, TestUnpack,
+                         ::testing::Values(64, 128, 2048),
+                         [](const 
::testing::TestParamInfo<TestUnpack::ParamType>& info) {
+                           return "Count" + std::to_string(info.param);

Review Comment:
   Nit: perhaps `Length=` instead of `Count`, for clarity.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to