Apply aggregation for single-view charts

[pluehne]

Due to a mistake (see #120), the aggregation method was invoked only on the new multiview charts but not on single-view charts. With this change, single-view charts are handled like multiview charts with only one view. This fixes the aggregation issue, makes things more consistent, and cleans up the code.

Fixes #120.

[filmaj]

We should write a unit test for this so we don't regress in the future 😄

[pluehne]

@filmaj: Good idea. Don’t know why I didn’t think about a unit test earlier. I’ll write one 👍.

[pluehne]

Writing unit tests for this turns out not to be straightforward. Find my work-in-progress unit tests in this separate branch.

Let’s first assume that we want to have a DOM-based unit test in place, as I sketched in that branch. Then, we run into the following difficulties:

• We have to get a DOM element created before the onload routine in charts.js is executed, which turns the placeholders into actual charts.
• The placeholder has a data-url that needs to point to an actual file that is accessible from within Karma.
• The document variable doesn’t seem to be accessible by default, and using a global for that seems odd.
• The fixture is somewhat clunky.

We could also question whether to use actual DOM for this at all. Instead, we could change the charts.js interface in such a way that all functions can be invoked unaware of AJAX calls and DOM creation. However, this would mean that we need to make the data variables configurable from outside and move AJAX calls etc. outside the createChart functions.

@filmaj: Do you see how we could get this tested elegantly?

[filmaj]

@pluehne I have created what I think is the scaffold for a unit test for this on my fork (same branch name): https://github.com/filmaj/hubble/tree/patrick/fix-aggregation-in-pull-request-usage-chart

My approach short-circuits some of the assumptions you wrote out:

1. The DOM is secondary in our concern here: what we want to isolate and test is the behaviour of the createHistoryChart. The fact that function interacts with the DOM is just a visual result of the execution of that function. In this PR, we're interested in making assertions about how the function composes its views, I think. I'm looking to you to fill this out with the right assertions.
2. We can control the data the chart pulls in by spying on d3 methods and having them call fake functions within our tests temporarily, so that we have fine-grained control of what "the data" should look like, but just for the lifetime of the single test.
3. document is available, since the tests run in the context of a browser via Karma.
4. Agree the fixture is clunky.

Hopefully my test scaffold helps point you in the right direction. Let me know if you have more questions.

[filmaj]

One more thing: as to how to assert / set expectations on changing state inside the createHistoryChart method, if we're interested in the state of the views variable therein, it looks like the views eventually get passed to $(canvas).data('views', views);. So I think inside the test we can:

1. Call createHistoryChart
2. Retrieve the views data from the canvas object let views = $(canvas).data('views');
3. Assert on the state of views.

This is all assuming I'm understanding what's going on here properly ;)

[pluehne]

@filmaj: Thanks for your helpful replies! I won’t have the time to look at this until Monday, but I’m sure your scaffold will bring me on the right track 😄!

Just one minor thing, I recall that using document within the unit tests resulted in undeclared identifier 'document' error messages pointing out that Karma was configured in such a way to warn about using undeclared variables.

[filmaj]

Just one minor thing, I recall that using document within the unit tests resulted in undeclared identifier 'document' error messages pointing out that Karma was configured in such a way to warn about using undeclared variables.

Hmm, I wouldn't expect that. Running my test scaffold does not yield any warnings. Perhaps that was outside of an it() test block? Anything inside it and before/after blocks, if I understand correctly, should run inside the context of a (headless Chrome) browser.

[pluehne]

@filmaj: Note that I added document to the globals to prevent this issue from happening. I think that’s not a nice fix, though 🙂. Could you test whether you also run into this issue when removing document from the globals list?

[filmaj]

Ah yes, the linting fails if I remove it from the globals list. I think the better solution for that is to add the env: { browser: true } configuration to the test's eslintrc.json file (the one under the docs/spec directory). That would add all the globals one would expect in a standard browser environment: document, window, etc. Since the tests run in a browser environment via Karma, I think that is a better solution.

[pluehne]

@filmaj: Cool, that’s what I had been looking for the other day. I didn’t find that option in the documentation immediately, and I gave up too early as it seems 😄.

[filmaj]

The docs are big. StackOverflow is your friend 😉

[filmaj]

I've updated my branch to add that change, btw.

[pluehne]

Cool, will continue looking at this in a few days!

[pluehne]

@filmaj: We merged this without the unit test for the time being. I have looked into your suggestions, and I think that too much work is involved for testing such a simple property. As we finally wanted the fix to be available, we merged this pull request, but I’ll keep the unit test on my to-do list and may come back to it later. Thanks for your explanations on how this could be tested, they may be useful at some point!