Fix counter vizualization #4385

dmudro · 2019-11-22T08:25:52Z

What type of PR is this? (check all applicable)

Add Unit Tests
Bug Fix

Description

Small change in behaviour of the counter visualisation as we want to always display a value when count rows option is enabled; even when the value is 0 (i. e. 0 rows in the underlying data set). Introduces tests for logic contained in utils, though there is more to be done in this area (another PR coming as it touches a more general point).

Related Tickets & Documents

Closes #4265.

dmudro · 2019-11-22T10:12:39Z

client/app/visualizations/counter/utils.test.js

@@ -0,0 +1,168 @@
+import { getCounterData } from './utils';


Not able to stub getCounterData and other private functions in utils at the moment. Will be addressed in another PR.

Why would you want to stub it?

I meant to say 'getRowNumber and other private functions'. Stubbing (mocking) would depend on the testing strategy though.

Atm all these 'private' functions including getRowNumber, numberFormat, formatValue etc:

Contain logic and are not covered. This is fixable by simply exporting and running cases on them.

From a puristic perspective, when executing tests on getCounterData one would want to mock the calls to these private functions to isolate the test subject. Otherwise it's more of an integration testing (which is not too bad anyway). This type of mocking is not an easy one in React as it does not have dependency injection and available solutions are cumbersome or would require changing coding style.

I assume it's ok to test on integration level and gradually add tests for other functions.

I meant to say 'getRowNumber and other private functions'.

Oh, ok. This makes sense now :-)

I assume it's ok to test on integration level and gradually add tests for other functions.

It's indeed ok. We're not purists here but rather pragmatists :-) So even having just a test for the getCounterData function is enough considering its scope/size.

We're not purists here but rather pragmatists :-)

Nice one. Aligned.

arikfr

Thank you for fixing this! Taking the extra effort of adding the tests is appreciated 🙇‍♂️

arikfr · 2019-11-24T08:58:51Z

client/app/visualizations/counter/utils.js

@@ -70,31 +70,27 @@ function formatTooltip(value, formatString) {
  return toString(value);
 }

-export function getCounterData(rows, options, visualizationName) {
+export function getCounterData(rows = [], options = {}, visualizationName = '') {


Why add the default values here? This function has a very specific usage and will always get these values.

If it's for the tests, then it's better to add the defaults in the tests... Will also make sure we're actually testing real use cases.

I agree refactoring code base for the sole sake of testing is generally not ideal.

rows = [] was meant to protect against calling getCounterData() at runtime which would trow TypeError on rows.length (and since there is no type checking at build time). Fair enough such case is not realistic and rows should be a mandatory param.

Same for options = {} although the nature of options may suggest they should be optional. In that sense keeping the default value is not just for tests and the same is true for visualizationName = '' which one may want to fallback to an empty string.

Would following amended function signature do the job here?:
function getCounterData(rows, options = {}, visualizationName = '')

There will always be options and a visualization name... I'm actually not sure about rows as I don't know what happens when we're still waiting for the results to load. But having a default value won't help in such case anyway, because we definitely pass some value, it just might be empty.

My suggestion is to revert the function definition to be as it was to make it clear that it needs these values, but for convenience in the test you can wrap it with a function that has default values.

ok cool, default param values removed

arikfr · 2019-11-24T09:00:27Z

client/app/visualizations/counter/utils.test.js

@@ -0,0 +1,168 @@
+import { getCounterData } from './utils';


Why would you want to stub it?

dmudro · 2019-11-24T10:28:29Z

Thank you for fixing this! Taking the extra effort of adding the tests is appreciated 🙇

Am not a huge fan of putting an extreme emphasis on tests but here the code with logic has been begging for decent coverage 🤓.

On related note getCounterData() seems to be slightly bloated and may benefit from splitting in more atomic parts, perhaps while addressing #4366. Wondering if values formatting could be done outside, i. e. in the React component.

gabrieldutra

Thanks @dmudro, I'll check quick wins for getCounterData later on #4392, so far I removed the getRowNumber in favor of lodash functions, thus it's one less function to worry about.

gabrieldutra · 2019-11-29T18:16:02Z

client/app/visualizations/counter/utils.test.js

+        { city: 'Tokyo', population: 38140000 },
+      ],
+      options: {},
+      visualisationName: 'Visualisation Name',


visualisation -> visualization 🙂

Nice catch and both forms are correct in terms of grammar if it's what you meant. These are just test dummies; might as well be foo name. Please feel free to rename to whatever you prefer.

It was actually to keep it as is in the rest of the codebase, but NP, I'll revisit those tests in #4392.

gabrieldutra

@arikfr got any more comments? A note is that I plan to grow those tests in #4392 with the different aggregation functions :)

gabrieldutra · 2019-12-09T18:27:42Z

client/app/visualizations/counter/utils.test.js

+        { city: 'Tokyo', population: 38140000 },
+      ],
+      options: {},
+      visualisationName: 'Visualisation Name',


It was actually to keep it as is in the rest of the codebase, but NP, I'll revisit those tests in #4392.

arikfr · 2019-12-10T11:38:43Z

Thank you @dmudro for the contribution and patience during the review :-)

dmudro added 3 commits November 21, 2019 13:11

crude unit tests for counter visualisation utils

4992eaf

improve type safety with default param values for getCounterData()

4f42b0a

fix count rows never shows zero

8956a46

kravets-levko self-requested a review November 22, 2019 09:08

kravets-levko added Frontend Visualizations Visualizations: Counter labels Nov 22, 2019

Merge branch 'master' into fix-counter-vizualization

1525afa

dmudro commented Nov 22, 2019

View reviewed changes

dmudro marked this pull request as ready for review November 22, 2019 10:32

arikfr reviewed Nov 24, 2019

View reviewed changes

dmudro requested a review from arikfr November 24, 2019 10:30

weekly-digest bot mentioned this pull request Nov 25, 2019

Weekly Digest (18 November, 2019 - 25 November, 2019) #4398

Closed

gabrieldutra mentioned this pull request Nov 26, 2019

Counter visualization enhancements #4392

Closed

5 tasks

remove default values for getCounterData() params

213a095

gabrieldutra reviewed Nov 29, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Dec 2, 2019

Weekly Digest (25 November, 2019 - 2 December, 2019) #4417

Closed

gabrieldutra approved these changes Dec 9, 2019

View reviewed changes

arikfr merged commit 0385b6f into getredash:master Dec 10, 2019

weekly-digest bot mentioned this pull request Dec 16, 2019

Weekly Digest (9 December, 2019 - 16 December, 2019) #4451

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix counter vizualization #4385

Fix counter vizualization #4385

dmudro commented Nov 22, 2019 •

edited by arikfr

Loading

dmudro Nov 22, 2019

arikfr Nov 24, 2019

dmudro Nov 24, 2019 •

edited

Loading

arikfr Nov 27, 2019

dmudro Nov 29, 2019

arikfr left a comment

arikfr Nov 24, 2019

dmudro Nov 24, 2019

arikfr Nov 27, 2019

dmudro Nov 29, 2019

arikfr Nov 24, 2019

dmudro commented Nov 24, 2019

gabrieldutra left a comment

gabrieldutra Nov 29, 2019 •

edited

Loading

dmudro Dec 1, 2019

gabrieldutra Dec 9, 2019

gabrieldutra left a comment

gabrieldutra Dec 9, 2019

arikfr commented Dec 10, 2019

Fix counter vizualization #4385

Fix counter vizualization #4385

Conversation

dmudro commented Nov 22, 2019 • edited by arikfr Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmudro Nov 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikfr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmudro commented Nov 24, 2019

gabrieldutra left a comment

Choose a reason for hiding this comment

gabrieldutra Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikfr commented Dec 10, 2019

dmudro commented Nov 22, 2019 •

edited by arikfr

Loading

dmudro Nov 24, 2019 •

edited

Loading

gabrieldutra Nov 29, 2019 •

edited

Loading