Chart.js: OSX master branch unit test errors

[andig]

These are as of b03ab1ca452b0e613d3c3f495f38d78937aa52df, split by Chrome and FireFox. Please let me know how I can contribute.

Chrome: https://pastebin.com/eJNMZNUa
Firefox: https://pastebin.com/9CsAiWgD

[benmccann]

@andig is this one you'd be willing to send a pull request for?

also, quick note, but make sure to test against the latest commit in master. there were a few recent test fixes like https://github.com/chartjs/Chart.js/commit/c6bda02468475fc9b2548e143b64b6a8fff4139c

[andig]

I have unfortunately no clue how to fix these errors. I can submit test resulta for latest master if that helps.

[benmccann]

Sure, it'd be helpful to confirm if this is still an issue after recent changes

Unfortunately I'd have a hard time fixing this one too since I develop on Linux

[andig]

I'm still seeing 40 unit test errors on ff and chrome each on my local machine. To make this a little more accessible- would you mind if I tried to beef the travis integration to automatically run the tests on OSX, too? Not sure this is entirely feasible but I could at least try?

[simonbrunel]

That would be great if only unit tests (gulp test) could be ran on each platform but I'm not sure that's possible without tricky Travis configuration. We don't want Travis to execute everything multiple time, it will take too much time but more importantly, it will break the release process since it will try to deploy multiple time to GH and npm (not good).

[etimberg]

Interesting that it's still broken on OSX. I tested the latest stuff locally and the errors I was seeing are gone.

@andig what version of OSX, Chrome, and FF do you have on your system?

[andig]

OSX: Sierra 10.12.6
Chrome: 59.0.3071.115
Firefox: 47.0.1 (rarely used, I'll check later version, too)

I'll check if I can wip up a simple, unit-test only OSX build to run by default. I have some experience with tricky travis-ci setups.

[andig]

Here is a hacked-up version of the travis build for linux plus osx: https://travis-ci.org/andig/Chart.js/builds/260713745

The osx build only runs the unit test. It is unfortunately much slower than linux as resources are scarce. It should be good enough to get a first impression though. The deployment part is currently missing but can easily be added back in.

[simonbrunel]

Though the goal is not to maintain a tricky Travis config ;) We spent lot of time to get this build/deploy/release process stable, I really don't want to deal with a broken config for future release.

[andig]

Thats up to you. Unfortunately travis has no straightforward way to configure this buidl :/ I could offer to run osx from my repo and post results from time to time but that would make catching errors harder.

[simonbrunel]

I checked the build logs you posted in your first message and it seems that unit tests based on images comparison are failing, but we can't see the error output (image diff). Can you post a screenshot of one of these tests?

[andig]

Can you post a screenshot of one of these tests?

I've look through the various config files but couldn't figure out where to find those?

[simonbrunel]

You need to run gulp unittest --watch which will keep the browser(s) open. On the top/right part of the page, there is a "DEBUG" button, clicking on it will display the tests details. You should see failing image diff as described in this comment

[andig]

You need to run gulp unittest --watch

Great, didn't know that. Here we go:

The other failures on chrome are more or less the same. Failed image is always showing a zoomed section of the excepted image. As if this was some case of resizing and the resize hasn't finished?

[simonbrunel]

Looking at the other failing tests, it seems that all expect a canvas with half the render size, so that might be because your device has a pixel ratio of 2. Can you try to enforce the pixel ratio via the options after this line:

config.options.devicePixelRatio = 1;

[andig]

I'll check. Funny enough I just got a working build on travis without this change: https://travis-ci.org/andig/Chart.js/builds/261027889. Makes it look more like a timing issue?

[andig]

Update: you're on the right track after all.

Travis OSX: 100% success. Might be a non-retina device.

Local build without your fix:

Chrome 60.0.3112 (Mac OS X 10.12.6): Executed 574 of 578 (39 FAILED) (skipped 4) ERROR (18.529 secs / 16.01 secs)
Firefox 54.0.0 (Mac OS X 10.12.0): Executed 574 of 578 (40 FAILED) (skipped 4) (23.813 secs / 22.728 secs)

With fix:

Chrome 60.0.3112 (Mac OS X 10.12.6): Executed 574 of 578 (7 FAILED) (skipped 4) (28.539 secs / 26.799 secs)
Firefox 54.0.0 (Mac OS X 10.12.0): Executed 574 of 578 (8 FAILED) (skipped 4) (34.715 secs / 31.8 secs)

The failed tests are any variety, here is some examples:

Chrome 60.0.3112 (Mac OS X 10.12.6) Chart Retina scale (a.k.a. device pixel ratio) should scale the render size but not the "implicit" display size FAILED
        Expected render height 240 to be equal to 720
            at Object.<anonymous> (test/specs/core.controller.tests.js:606:18)
Chrome 60.0.3112 (Mac OS X 10.12.6): Executed 111 of 578 (3 FAILED) (0 secs / 4.351 secs)
Firefox 54.0.0 (Mac OS X 10.12.0): Executed 4 of 578 SUCCESS (0 secs / 0.854 secs)
Chrome 60.0.3112 (Mac OS X 10.12.6) Chart Retina scale (a.k.a. device pixel ratio) should scale the render size but not the "implicit" display size FAILED
Chrome 60.0.3112 (Mac OS X 10.12.6) Chart Retina scale (a.k.a. device pixel ratio) should scale the render size but not the "explicit" display size FAILED
        Expected render height 240 to be equal to 720
            at Object.<anonymous> (test/specs/core.controller.tests.js:623:18)
Chrome 60.0.3112 (Mac OS X 10.12.6): Executed 112 of 578 (4 FAILED) (0 secs / 4.355 secs)
Firefox 54.0.0 (Mac OS X 10.12.0): Executed 4 of 578 SUCCESS (0 secs / 0.854 secs)
Chrome 60.0.3112 (Mac OS X 10.12.6) Chart Retina scale (a.k.a. device pixel ratio) should scale the render size but not the "explicit" display size FAILED
Chrome 60.0.3112 (Mac OS X 10.12.6) Chart config.options.devicePixelRatio 3 should scale the render size but not the "implicit" display size FAILED
        Expected render height 240 to be equal to 720
            at Object.<anonymous> (test/specs/core.controller.tests.js:654:18)
Chrome 60.0.3112 (Mac OS X 10.12.6): Executed 113 of 578 (5 FAILED) (0 secs / 4.364 secs)
Firefox 54.0.0 (Mac OS X 10.12.0): Executed 4 of 578 SUCCESS (0 secs / 0.854 secs)
Chrome 60.0.3112 (Mac OS X 10.12.6) Chart config.options.devicePixelRatio 3 should scale the render size but not the "implicit" display size FAILED
Chrome 60.0.3112 (Mac OS X 10.12.6) Chart config.options.devicePixelRatio 3 should scale the render size but not the "explicit" display size FAILED

[simonbrunel]

Actually, I'm also getting these failures if I apply the suggested changes locally. I'm not sure if it's possible to change the browser device aspect ratio manually, but let's try to set window.devicePixelRatio = 1 before this line.

[andig]

Better:

Chrome 60.0.3112 (Mac OS X 10.12.6): Executed 574 of 578 (4 FAILED) (skipped 4) ERROR (10.216 secs / 9.661 secs)
Firefox 54.0.0 (Mac OS X 10.12.0): Executed 574 of 578 (5 FAILED) (skipped 4) (19.207 secs / 18.197 secs)

[andig]

And here's the complete remaining list:

Chrome 60.0.3112 (Mac OS X 10.12.6) Chart Retina scale (a.k.a. device pixel ratio) should scale the render size but not the "implicit" display size FAILED
    Expected render height 240 to be equal to 720
        at Object.<anonymous> (test/specs/core.controller.tests.js:606:18)
Chrome 60.0.3112 (Mac OS X 10.12.6) Chart Retina scale (a.k.a. device pixel ratio) should scale the render size but not the "explicit" display size FAILED
    Expected render height 240 to be equal to 720
        at Object.<anonymous> (test/specs/core.controller.tests.js:623:18)
Chrome 60.0.3112 (Mac OS X 10.12.6) Chart config.options.devicePixelRatio 3 should scale the render size but not the "implicit" display size FAILED
    Expected render height 240 to be equal to 720
        at Object.<anonymous> (test/specs/core.controller.tests.js:654:18)
Chrome 60.0.3112 (Mac OS X 10.12.6) Chart config.options.devicePixelRatio 3 should scale the render size but not the "explicit" display size FAILED
    Expected render height 240 to be equal to 720
        at Object.<anonymous> (test/specs/core.controller.tests.js:672:18)
....
WARN: 'Deprecation warning: value provided is not in a recognized RFC2822 or ISO format. moment construction falls back to js Date(), which is not reliable across all browsers and versions. Non RFC2822/ISO date formats are discouraged and will be removed in an upcoming major release. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info.
Arguments: 
[0] _isAMomentObject: true, _isUTC: false, _useUTC: false, _l: undefined, _i: A, _f: undefined, _strict: undefined, _locale: [object Object]
Error
    at Function.createFromInputFallback (http://localhost:9876/absolute/var/folders/w5/_c0nb6n90fn96tzw04dtc6240000gn/T/3076a0bd546f5802dfa49ee68748f329.browserify?9e87072d9a5367e5dc5355bd5e852e2b1a4ad816:1978:94)
Firefox 54.0.0 (Mac OS X 10.12.0) Chart Retina scale (a.k.a. device pixel ratio) should scale the render size but not the "implicit" display size FAILED
    Expected render height 240 to be equal to 720
    @test/specs/core.controller.tests.js:606:4
Firefox 54.0.0 (Mac OS X 10.12.0) Chart Retina scale (a.k.a. device pixel ratio) should scale the render size but not the "explicit" display size FAILED
    Expected render height 240 to be equal to 720
    @test/specs/core.controller.tests.js:623:4
Firefox 54.0.0 (Mac OS X 10.12.0) Chart config.options.devicePixelRatio 3 should scale the render size but not the "implicit" display size FAILED
    Expected render height 240 to be equal to 720
    @test/specs/core.controller.tests.js:654:4
Firefox 54.0.0 (Mac OS X 10.12.0) Chart config.options.devicePixelRatio 3 should scale the render size but not the "explicit" display size FAILED
    Expected render height 240 to be equal to 720
    @test/specs/core.controller.tests.js:672:4
Arguments: 
[0] _isAMomentObject: true, _isUTC: false, _useUTC: false, _l: undefined, _i: A, _f: undefined, _strict: undefined, _locale: [object Object]
TOTAL: 8 FAILED, 1140 SUCCESS
[17:03:37] 'unittest' errored after 23 s
[17:03:37] Error: Karma returned with the error code: 1

A bit confusing that the moment.js stuff is only in my local test? Might be due to both ff and chrome being more recent versions?

[simonbrunel]

Did you remove config.options.devicePixelRatio = 1;?

[andig]

Ehrm no. Doing so limits it to one failure locally:

Firefox 54.0.0 (Mac OS X 10.12.0) Platform.dom event handling should notify plugins about events FAILED
        Expected 255 to be 256.
        @test/specs/platform.dom.tests.js:398:4

Still don't get why momentjs should depend on that?

[simonbrunel]

That looks like to a rounding issue, you could change these tests for:

expect(notifiedEvent.x).toBeCloseToPixel(chart.width / 2);
expect(notifiedEvent.y).toBeCloseToPixel(chart.height / 2);

I don't know about the moment stuff, that's just warnings.

[andig]

Success. This makes the OSX travis build obsolete as it didn't catch the retina errors anyway. Do you want me to send a PR for the changes or update yourself?

Or rather: are these fixes "ok" for running chartjs on a retina/highres device, i.e. did we just hack it to pass or are the test cases just not taking retina into account in their expectations?

[simonbrunel]

Sounds good, can you PR these changes?

Most unit tests assume a DPR of 1 (when comparing with pixel values), same for the image based tests which compare with bitmaps acquired with a DPR of 1. There is unit tests to make sure that the DPR is correctly taken in account, so I think these changes are totally fine. I would simply add a comment above the window.devicePixelRatio = 1 to explain why.

[simonbrunel]

Agree for the OSX Travis builds: we ship a JS library (not binaries) so for a given browser/version, our code should run and render the same whatever the platform is (I guess Chrome/FF have their own cross platform unit tests). If there is a difference between Chrome 59 under Linux and OSX, I think we can assume that's an issue with the browser itself, not with Chart.js.

[andig]

Learned so much again, THANKS!

[simonbrunel]

It makes our tests more reliable, thank you for the time spent on it.

[etimberg]

Great find @simonbrunel @andig. This explains why I didn't see these failures on my MBP as it's old and doesn't have a retina display.