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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
