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
fix: sum([invalid]) -> undefined #3849
Conversation
Drat, I was hoping the checks would run automatically :) OK I'll be less lazy and set things up locally! |
I approved😀 |
Thank you! I'll fix up the tests and do some more checks a bit later :) |
All tests passing! |
@@ -330,7 +330,7 @@ tape('Aggregate handles empty/invalid data', t => { | |||
'exponential', | |||
'exponentialb' | |||
]; | |||
const res = [4, 3, 0, 0]; // higher indices 'undefined' |
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.
What does this change do? Does this sufficiently test the new behavior?
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 way the test is written, it checks the output of each aggregator in the list above when fed invalid values. The first four aggregators in the list used to return ints, the rest return undefined. sum
is the fourth in that list and it used to return 0 but now returns undefined, so all I had to do was delete that one entry in the res
array, and now we assert that sum([NaN, null, undefined, '']) = undefined
just like mean
and min
etc.
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.
So yes, I believe this is a sufficient test, or at least it's an equivalently-sufficient one to all the other aggregtors :)
Is there anything else I can do to make us confident about merging this? |
Thank you. |
Updating the behaviour of
sum
per #3848