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.

Sign up for GitHub

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

Jump to bottom

Fix counter vizualization #4385

Merged
merged 5 commits into from
Dec 10, 2019
Merged

Fix counter vizualization #4385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4992eaf
crude unit tests for counter visualisation utils
dmudro Nov 19, 2019
4f42b0a
improve type safety with default param values for getCounterData()
dmudro Nov 21, 2019
8956a46
fix count rows never shows zero
dmudro Nov 21, 2019
1525afa
Merge branch 'master' into fix-counter-vizualization
dmudro Nov 22, 2019
213a095
remove default values for getCounterData() params
dmudro Nov 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 6 additions & 10 deletions client/app/visualizations/counter/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,29 +72,25 @@ function formatTooltip(value, formatString) {

export function getCounterData(rows, options, visualizationName) {
const result = {};

const rowsCount = rows.length;
if (rowsCount > 0) {
const rowNumber = getRowNumber(options.rowNumber, rowsCount);
const targetRowNumber = getRowNumber(options.targetRowNumber, rowsCount);

if (rowsCount > 0 || options.countRow) {
const counterColName = options.counterColName;
const targetColName = options.targetColName;
const counterLabel = options.counterLabel;

if (counterLabel) {
result.counterLabel = counterLabel;
} else {
result.counterLabel = visualizationName;
}
result.counterLabel = options.counterLabel || visualizationName;

if (options.countRow) {
result.counterValue = rowsCount;
} else if (counterColName) {
const rowNumber = getRowNumber(options.rowNumber, rowsCount);
result.counterValue = rows[rowNumber][counterColName];
}

result.showTrend = false;

if (targetColName) {
const targetRowNumber = getRowNumber(options.targetRowNumber, rowsCount);
result.targetValue = rows[targetRowNumber][targetColName];

if (Number.isFinite(result.counterValue) && isFinite(result.targetValue)) {
Expand Down
192 changes: 192 additions & 0 deletions client/app/visualizations/counter/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
import { getCounterData } from './utils';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Why would you want to stub it?

Copy link
Contributor Author

@dmudro dmudro Nov 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. Contain logic and are not covered. This is fixable by simply exporting and running cases on them.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Nice one. Aligned.


let dummy;

describe('Visualizations -> Counter -> Utils', () => {
beforeEach(() => {
dummy = {
rows: [
{ city: 'New York City', population: 18604000 },
{ city: 'Shangai', population: 24484000 },
{ city: 'Tokyo', population: 38140000 },
],
options: {},
visualisationName: 'Visualisation Name',
Copy link
Member

@gabrieldutra gabrieldutra Nov 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

visualisation -> visualization 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

result: {
counterLabel: 'Visualisation Name',
counterValue: '',
targetValue: null,
counterValueTooltip: '',
targetValueTooltip: '',
},
};
});

describe('getCounterData()', () => {
describe('"Count rows" option is disabled', () => {
test('No target and counter values return empty result', () => {
const result = getCounterData(dummy.rows, dummy.options, dummy.visualisationName);
expect(result).toEqual({
...dummy.result,
showTrend: false,
});
});

test('"Counter label" overrides vizualization name', () => {
const result = getCounterData(
dummy.rows,
{ counterLabel: 'Counter Label' },
dummy.visualisationName,
);
expect(result).toEqual({
...dummy.result,
counterLabel: 'Counter Label',
showTrend: false,
});
});

test('"Counter Value Column Name" must be set to a correct non empty value', () => {
const result = getCounterData(
dummy.rows,
{ rowNumber: 3 },
dummy.visualisationName,
);
expect(result).toEqual({
...dummy.result,
showTrend: false,
});

const result2 = getCounterData(
dummy.rows,
{ counterColName: 'missingColumn' },
dummy.visualisationName,
);
expect(result2).toEqual({
...dummy.result,
showTrend: false,
});
});

test('"Counter Value Column Name" uses correct column', () => {
const result = getCounterData(
dummy.rows,
{ counterColName: 'population' },
dummy.visualisationName,
);
expect(result).toEqual({
...dummy.result,
counterValue: '18,604,000.000',
counterValueTooltip: '18,604,000',
showTrend: false,
});
});

test('Counter and target values return correct result including trend', () => {
const result = getCounterData(
dummy.rows,
{
rowNumber: 1,
counterColName: 'population',
targetRowNumber: 2,
targetColName: 'population',
},
dummy.visualisationName,
);
expect(result).toEqual({
...dummy.result,
counterValue: '18,604,000.000',
counterValueTooltip: '18,604,000',
targetValue: '24484000',
targetValueTooltip: '24,484,000',
showTrend: true,
trendPositive: false,
});

const result2 = getCounterData(
dummy.rows,
{
rowNumber: 2,
counterColName: 'population',
targetRowNumber: 1,
targetColName: 'population',
},
dummy.visualisationName,
);
expect(result2).toEqual({
...dummy.result,
counterValue: '24,484,000.000',
counterValueTooltip: '24,484,000',
targetValue: '18604000',
targetValueTooltip: '18,604,000',
showTrend: true,
trendPositive: true,
});
});
});

describe('"Count rows" option is enabled', () => {
beforeEach(() => {
dummy.result = {
...dummy.result,
counterValue: '3.000',
counterValueTooltip: '3',
showTrend: false,
};
});

test('Rows are counted correctly', () => {
const result = getCounterData(
dummy.rows,
{ countRow: true },
dummy.visualisationName,
);
expect(result).toEqual(dummy.result);
});

test('Counter value is ignored', () => {
const result = getCounterData(
dummy.rows,
{
countRow: true,
rowNumber: 3,
counterColName: 'population',
},
dummy.visualisationName,
);
expect(result).toEqual(dummy.result);
});

test('Target value and trend are computed correctly', () => {
const result = getCounterData(
dummy.rows,
{
countRow: true,
targetRowNumber: 2,
targetColName: 'population',
},
dummy.visualisationName,
);
expect(result).toEqual({
...dummy.result,
targetValue: '24484000',
targetValueTooltip: '24,484,000',
showTrend: true,
trendPositive: false,
});
});

test('Empty rows return counter value 0', () => {
const result = getCounterData(
[],
{ countRow: true },
dummy.visualisationName,
);
expect(result).toEqual({
...dummy.result,
counterValue: '0.000',
counterValueTooltip: '0',
});
});
});
});
});