Replace random test values by discrete ones #23956
Conversation
The use of Random() function in the parquet.batchreader.decoders tests may cause flakiness. Adds TestMode.java to parameterize tests with an arbitrary value and an upper-/lower-bounded value. Resolves: prestodb#23840
presto-parquet/src/test/java/com/facebook/presto/parquet/batchreader/decoders/TestMode.java
Outdated
Show resolved
Hide resolved
| */ | ||
| package com.facebook.presto.parquet.batchreader.decoders; | ||
|
|
||
| public enum TestMode |
There was a problem hiding this comment.
Instead of embedding the concept of TestMode into the data generator, I'm just wondering if we should have method overloads for each test mode. For example, instead of one method generatePlainValuesPage that takes in TestMode, we have three lightweight methods generatePlainValuesPageUpperBound etc. that generate the test data in different ways. They could delegate to a common inner method which does most of the work.
There was a problem hiding this comment.
+1 to going with the object-oriented approach. I think just having a superclass with the main logic and maybe delegating the mode-specific details to an abstract method implementation would be better than having to pass in the TestMode.
There was a problem hiding this comment.
As per now I committed an idea integrating both of your suggestions. The TestMode is removed. It got replaced by three lightweight methods implemented in three subclasses that refer to the TestParquetUtils, where still the main part is handled. Would be curious to know your thoughts, because I still have the feeling it might be not as clean as it can be.
...rquet/src/test/java/com/facebook/presto/parquet/batchreader/decoders/TestValuesDecoders.java
Show resolved
Hide resolved
| */ | ||
| package com.facebook.presto.parquet.batchreader.decoders; | ||
|
|
||
| public enum TestMode |
There was a problem hiding this comment.
+1 to going with the object-oriented approach. I think just having a superclass with the main logic and maybe delegating the mode-specific details to an abstract method implementation would be better than having to pass in the TestMode.
|
Thanks for the comments you two 👍 FYI: I’ll be able to access my notebook next week again. So you’re aware I’m not ignoring it. |
Description
Adds TestMode.java to parameterize tests with an arbitrary value and an upper-/lower-bounded value.
Used upper and lower bounds of the previously used Random.getInt()/Random.getLong() functions. Although before the lower bound with Random.getInt()/Random.getLong() was 0 it was lowered to the most negative values for the datatype where possible.
com/facebook/presto/parquet/batchreader/decoders/TestParquetUtils.javathe valueString in line112 was randomly chosen. This might deserve more attention on code review.Motivation and Context
The use of Random() function in the parquet.batchreader.decoders tests may cause flakiness.
Resolves: #23840
Impact
Developer level: Reduced flakiness in test.
Test Plan
CI should run.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.