[HUDI-568] Improve unit test coverage #1473
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1473 +/- ##
============================================
+ Coverage 67.64% 68.05% +0.40%
Complexity 261 261
============================================
Files 348 348
Lines 16672 16670 -2
Branches 1694 1694
============================================
+ Hits 11278 11344 +66
+ Misses 4653 4586 -67
+ Partials 741 740 -1
Continue to review full report at Codecov.
|
| @@ -75,9 +75,6 @@ public RocksDBDAO(String basePath, String rocksDBBasePath) { | |||
| * Create RocksDB if not initialized. | |||
| */ | |||
| private RocksDB getRocksDB() { | |||
| if (null == rocksDB) { | |||
There was a problem hiding this comment.
What if getRocksDB is called repetitively ? It should not init() every time, do we plan to add a different check in init to allow for better code coverage ?
There was a problem hiding this comment.
i looked at the entire code. it seems like init() gets called from the only constructor in this class. so there will never be a case where rocksDB == null. this code seemed redundant and was never going to get called. that's why removed it. without this code, we are just going to return the rocksDB instance everytime without checking. Also checked the code and there is no where the variable is being set to null
There was a problem hiding this comment.
sounds good, thanks for the explanation
There was a problem hiding this comment.
refactored the code as you requested. lmk if this can be merged. thanks
| @@ -99,25 +104,119 @@ public void testRocksDBManager() { | |||
| List<Pair<String, Payload>> gotPayloads = | |||
| dbManager.<Payload>prefixSearch(family, prefix).collect(Collectors.toList()); | |||
| Integer expCount = countsMap.get(family).get(prefix); | |||
| System.out.printf("%s,%s: %d, %d\n", prefix, family, expCount == null ? 0L : expCount.longValue(), gotPayloads.size()); | |||
There was a problem hiding this comment.
can we make this log ? if there is already an assert, this might not be needed
There was a problem hiding this comment.
this was just a debug statement. so removed it
| } | ||
|
|
||
| @Test | ||
| public void write() throws IOException { |
There was a problem hiding this comment.
Can you add comments for each of the tests you have added to explain what you are trying to test here ? Especially with the mock, it is unclear to a new reader
There was a problem hiding this comment.
added comments
| public class TestHoodieRealtimeFileSplit { | ||
|
|
||
| private HoodieRealtimeFileSplit split; | ||
| private String basePath = "/tmp"; |
There was a problem hiding this comment.
Take a look at https://github.com/apache/incubator-hudi/blob/master/hudi-common/src/test/java/org/apache/hudi/common/HoodieCommonTestHarness.java#L48 to better initialize things like basePath and other variables that you intend to use throughout the execution of the test
There was a problem hiding this comment.
this is a pure unit test and everything that happens is within the context of this class. all the variables used here are just dummy paths. since i am doing checks on the length of data written down the call stack, it will be hard to validate things if we use temporary folder names given by the testing framework.
There was a problem hiding this comment.
The path is something you can keep in a temporary variable in a @BeforeClass or @before, it's best to use that or if you prefer use the TemporaryFolder from junit -> would like to avoid hard-coded temp paths used for testing
Classes improved: * HoodieTableMetaClient * RocksDBDAO * HoodieRealtimeFileSplit
What is the purpose of the pull request
Improve code coverage for the following classes
Brief change log
Added new tests for functions missing coverage
Verify this pull request
Added tests in the following files
Committer checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.