[GitHub] [pinot] richardstartin commented on a change in pull request #8035: refine segment consistency check

[GitBox <gi...@apache.org>]

richardstartin commented on a change in pull request #8035:
URL: https://github.com/apache/pinot/pull/8035#discussion_r787198792



##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java
##########
@@ -1115,6 +1116,18 @@ private void testIfNeedProcess()
       assertFalse(processor.needProcess());
     }
 
+    // No preprocessing needed if required to add certain index on non-existing, sorted or non-dictionary column.
+    for (Consumer<IndexLoadingConfig> prepFunc : createConfigPrepFunctionNeedNoops()) {

Review comment:
       The purpose of a test is to prevent someone from breaking the implementation unexpectedly in the future as much as it is to demonstrate that a change/feature works, so a test must surface diagnostics about failure. This test was already problematic in this regard, but this change doesn't make it better: If this test ever fails, someone will need to put a breakpoint in this loop to find out when the test actually fails, and when they do that, they will see a `Consumer` without any identifiable state with a class name like "...SegmentPreProcessorTest$$Lambda$86/0x00000008001a5c40". The only way to know which consumer the test fails for would be to modify the test to add a counter to be incremented on each iteration, and then look at the nth position in the list created in `createConfigPrepFunctionNeedNoops`. 
   
   This can all be circumnavigated by using `DataProvider` to create combinations of test inputs (including a descriptive test name) to supply to generic testing logic.




-- 
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: commits-unsubscribe@pinot.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org