-
Notifications
You must be signed in to change notification settings - Fork 28
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
Improved aggrMethod to accept multiple values #605
Changes from all commits
e53b905
ee9cded
c3601a5
e2fcabd
3b51b4c
885c78c
4abf35f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
- Remove: RPM stuff | ||
- Add: aggrMethod accept multiple values separated by comma (#432, partial) | ||
- Add: aggrMethod 'all' to get all the possible aggregations (#432, partial) | ||
- Remove: RPM stuff |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -324,13 +324,77 @@ describe('sth tests', function() { | |
}) | ||
); | ||
|
||
it( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest to add a couple of cases:
Btw, what happens if aggMethod mixes all with another method? Eg. aggMethod=all,min? Do we get and error or the "min" part is ignored and that's equivalent to aggMethod=all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added test cases in 3b51b4c If aggMethod mixes |
||
'should respond with 200 - OK if aggrMethod are multiple and aggrPeriod query params', | ||
sthTestUtils.status200Test.bind(null, 2, { | ||
aggrMethod: 'min,max', | ||
aggrPeriod: 'second' | ||
}) | ||
); | ||
|
||
it( | ||
'should respond with 200 - OK if aggrMethod are multiple and aggrPeriod query params', | ||
sthTestUtils.status200Test.bind(null, 2, { | ||
aggrMethod: 'all', | ||
aggrPeriod: 'second' | ||
}) | ||
); | ||
|
||
it( | ||
'should respond with 400 - Bad Request if aggrMethod is not from [min,max,sum,sum2,occur,all]', | ||
sthTestUtils.status400Test.bind(null, 2, { | ||
aggrMethod: 'foo', | ||
aggrPeriod: 'second' | ||
}) | ||
); | ||
|
||
it( | ||
'should respond with 400 - Bad Request if aggrMethod are multiple and not from [min,max,sum,sum2,occur,all]', | ||
sthTestUtils.status400Test.bind(null, 2, { | ||
aggrMethod: 'foo,all', | ||
aggrPeriod: 'second' | ||
}) | ||
); | ||
|
||
it( | ||
'should respond with 200 - OK if aggrMethod and aggrPeriod query params - NGSIv1', | ||
sthTestUtils.status200Test.bind(null, 1, { | ||
aggrMethod: 'min', | ||
aggrPeriod: 'second' | ||
}) | ||
); | ||
|
||
it( | ||
'should respond with 200 - OK if aggrMethod are multiple and aggrPeriod query params - NGSIv1', | ||
sthTestUtils.status200Test.bind(null, 1, { | ||
aggrMethod: 'min,max', | ||
aggrPeriod: 'second' | ||
}) | ||
); | ||
|
||
it( | ||
'should respond with 200 - OK if aggrMethod are multiple and aggrPeriod query params - NGSIv1', | ||
sthTestUtils.status200Test.bind(null, 1, { | ||
aggrMethod: 'all', | ||
aggrPeriod: 'second' | ||
}) | ||
); | ||
|
||
it( | ||
'should respond with 400 - Bad Request if aggrMethod is not from [min,max,sum,sum2,occur,all] - NGSIv1', | ||
sthTestUtils.status400Test.bind(null, 1, { | ||
aggrMethod: 'foo', | ||
aggrPeriod: 'second' | ||
}) | ||
); | ||
|
||
it( | ||
'should respond with 400 - Bad Request if aggrMethod are multiple and not from [min,max,sum,sum2,occur,all] - NGSIv1', | ||
sthTestUtils.status400Test.bind(null, 1, { | ||
aggrMethod: 'foo,all', | ||
aggrPeriod: 'second' | ||
}) | ||
); | ||
}); | ||
|
||
function eachEventTestSuiteContainer(attrName, attrType, includeTimeInstantMetadata) { | ||
|
@@ -390,6 +454,16 @@ describe('sth tests', function() { | |
'aggregated data retrieval', | ||
sthTestUtils.aggregatedDataRetrievalSuite.bind(null, 'attribute-float', 'float', 'sum2') | ||
); | ||
|
||
describe( | ||
'aggregated data retrieval', | ||
sthTestUtils.aggregatedDataRetrievalSuite.bind(null, 'attribute-float', 'float', 'max,sum2') | ||
); | ||
|
||
describe( | ||
'aggregated data retrieval', | ||
sthTestUtils.aggregatedDataRetrievalSuite.bind(null, 'attribute-float', 'float', 'all') | ||
); | ||
} | ||
|
||
for (let j = 0; j < sthTestConfig.SAMPLES; 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.
Instead of modifying existing tests, please add new ones. In particular, I'd suggest two new tests cases:
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.
Hi @fgalan,
I have added the new test cases here.
Previously, only one aggrMethod was there so switch case was working fine. But in case of multiple aggregation method we need to update the code for multiple values separately. The above code change is for util class to accommodate the multiple aggregation methods.