Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: validate index size when transforming #422

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

jeqo
Copy link
Contributor

@jeqo jeqo commented Oct 17, 2023

Index size can potentially be zero or too large for transformation to work.

@jeqo jeqo requested a review from a team as a code owner October 17, 2023 14:35
@jeqo jeqo force-pushed the jeqo/rsm-tx-index-logging branch 2 times, most recently from 8287729 to 90e4675 Compare October 18, 2023 04:52
@ivanyu ivanyu self-assigned this Oct 20, 2023
ivanyu
ivanyu previously approved these changes Oct 20, 2023
Copy link
Contributor

@AnatolyPopov AnatolyPopov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general LGTM but should be refactored later.

@@ -465,6 +469,79 @@ void testRequiresCompression(final CompressionType compressionType, final boolea
assertThat(requires).isEqualTo(expectedResult);
}

@ParameterizedTest
@ValueSource(booleans = {true, false})
void testTransformingIndexes(final boolean encryption) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say these tests should not be in this class but somewhere in /test directory since these are plain unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if statements based on parameterized tests arguments is no the best approach and raise a question about overall feasibility of the tests being parameterized but since we had it here earlier let's address it later altogether.

Index size can potentially be zero or too large for transformation to work.
@jeqo jeqo force-pushed the jeqo/rsm-tx-index-logging branch from e721b4a to a15f1cd Compare October 20, 2023 13:18
@AnatolyPopov AnatolyPopov merged commit ccce813 into main Oct 20, 2023
7 checks passed
@AnatolyPopov AnatolyPopov deleted the jeqo/rsm-tx-index-logging branch October 20, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants