MINOR: refactor AbstractSegmentsTests to reduce redundancies#21674
MINOR: refactor AbstractSegmentsTests to reduce redundancies#21674mjsax wants to merge 2 commits intoapache:trunkfrom
Conversation
AbstractSegments was refactored in apache#21520 moving shared code from subclassed into AbstractSegments to reduce code duplication. This implies that we should test shared functionality only once, and not re-test for each subclass. This PR removes redundant tests from subclass tests into AbstractSegmentsTest to reduce test duplication.
| protected abstract void openSegmentDB(final S segment, final StateStoreContext context); | ||
| protected void openSegment(final S segment, final StateStoreContext context) { | ||
| segment.openDB(context.appConfigs(), context.stateDir()); | ||
| } |
There was a problem hiding this comment.
Let me know what you think of this -- I thought it would be good to have a default impl, which works for most Segments but it's a little bit of a hack to make it work. Cf other comments.
| public void openDB(final Map<String, Object> configs, final File stateDir) { | ||
| super.openDB(configs, stateDir); | ||
| // skip the registering step | ||
| } |
There was a problem hiding this comment.
If we make openDB public on RocksDBStore, we don't need this override any longer.
| @Override | ||
| protected void openSegmentDB(final KeyValueSegment segment, final StateStoreContext context) { | ||
| segment.openDB(context.appConfigs(), context.stateDir()); | ||
| } |
There was a problem hiding this comment.
Not needed any longer -- can re-sure the new one from AbstractSegments now -- if we don't like the implication of having a default impl in AbstractSegments for this method, would need to add it back here. Similar for other segments classes.
| openIterators.clear(); | ||
| } | ||
| if (iterators.size() != 0) { | ||
| if (!iterators.isEmpty()) { |
| protected void openSegmentDB(final LogicalKeyValueSegment segment, final StateStoreContext context) { | ||
| protected void openSegment(final LogicalKeyValueSegment segment, final StateStoreContext context) { | ||
| // no-op -- a logical segment is just a view on an underlying physical store | ||
| } |
There was a problem hiding this comment.
Only LogicalKeyValueSegments cannot re-use the new default from AbstractSegments, so keeping the override here.
| assertFalse(first.isOpen()); | ||
| assertFalse(second.isOpen()); | ||
| assertFalse(third.isOpen()); | ||
| public void shouldCreateSegmentsOfCorrectType() { |
| } | ||
|
|
||
| @Test | ||
| public void shouldOpenExistingSegments() { |
| } | ||
|
|
||
| @Test | ||
| public void shouldGetSegmentIdsFromTimestamp() { |
There was a problem hiding this comment.
Same. Redundant test unified in AbstractSegmentsTest
| assertFalse(first.isOpen()); | ||
| assertFalse(second.isOpen()); | ||
| assertFalse(third.isOpen()); | ||
| public void shouldCreateSegmentsOfCorrectType() { |
| } | ||
|
|
||
| @Test | ||
| public void shouldOpenExistingSegments() { |
1 participant
AbstractSegments was refactored in
#21520 moving shared code from
subclassed into AbstractSegments to reduce code duplication. This
implies that we should test shared functionality only once, and not
re-test for each subclass. This PR removes redundant tests from subclass
tests into AbstractSegmentsTest to reduce test duplication.