-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Enhanced StarTreeIntegrationTest to test group-by results. Added more query of different types to increase test coverage. #244
Conversation
606c946
to
23449de
Compare
double[] actualSums = actualResult.get(expectedKey); | ||
|
||
for (int j = 0; j < expectedSums.length; j++) { | ||
Assert.assertEquals(expectedSums[j], actualSums[j], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention is to put actual value as first argument and expected as second. In the error message, no need to explicitly log actual and expected value since they will already show up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
23449de
to
f81ff1b
Compare
"select sum(m1) from T where d1 <> 'd1-v1' group by d2", | ||
"select sum(m1) from T where d1 in ('d1-v1', 'd1-v2') group by d2", | ||
"select sum(m1) from T where d1 in ('d1-v1', 'd1-v2') and d2 not in ('d2-v1') group by d3", | ||
"select sum(m1) from T where d1 in ('d1-v1', 'd1-v2') and d2 not in ('d2-v1') group by d3, d4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a query pattern of "select sum(m1), sum(m2) from T where..."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the comment, the test runs aggregation on all metric columns, not just the one in the query, to be more exhaustive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Then it's fine 👍
LGTM besides the comments. |
Current coverage is 45.84%
|
Added more query of different types to increase test coverage.
f81ff1b
to
43d2552
Compare
No description provided.