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.

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

Use short labels for the legend tests so as not to be affected by the font width #5842

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Nov 16, 2018

This fixes the #5816 test failures that can happen depending on environments.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

I still have one failing test :\

Firefox 63.0.0 (Windows 10.0.0) Legend block tests should draw correctly when the legend is positioned on the top FAILED
        Expected 105 to be close to pixel 107.
        @test/specs/plugin.legend.tests.js:213:48
        @test/specs/plugin.legend.tests.js:211:5
Chrome 70.0.3538 (Windows 10.0.0): Executed 759 of 763 (skipped 4) SUCCESS (4.835 secs / 4.629 secs)
Firefox 63.0.0 (Windows 10.0.0): Executed 535 of 763 (1 FAILED) (skipped 4) (0 secs / 4.832 secs)
Firefox 63.0.0 (Windows 10.0.0) Legend block tests should draw correctly when the legend is positioned on the top FAILED
        Expected 105 to be close to pixel 107.
Chrome 70.0.3538 (Windows 10.0.0): Executed 759 of 763 (skipped 4) SUCCESS (4.835 secs / 4.629 secs)
Firefox 63.0.0 (Windows 10.0.0): Executed 759 of 763 (1 FAILED) (skipped 4) (6.886 secs / 6.609 secs)
TOTAL: 1 FAILED, 1517 SUCCESS

@nagix
Copy link
Contributor Author

nagix commented Nov 19, 2018

All the text labels for the label box position tests are changed to empty. @simonbrunel Can you test again?

simonbrunel
simonbrunel previously approved these changes Nov 19, 2018
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

It works, are these tests still pertinent without labels?

@nagix
Copy link
Contributor Author

nagix commented Nov 19, 2018

Well, they test the sizes and positions of legendHitBoxes which consist of a sample box + a padding area + a text label box. If we check if the size of the text label box is reflected in legendHitBoxes, would it be better to use some simple string like ' '?

@simonbrunel
Copy link
Member

I think it will be the same issue with ' ' but we can try (though, that's weird it doesn't generate the same width for a space character). We can also try to use a more basic / common / generic font family in our tests (e.g. 'Arial'). Default is 'Helvetica Neue', 'Helvetica', 'Arial', sans-serif but I'm on Windows and don't have Helvetica Neue or Helvetica installed.

@nagix
Copy link
Contributor Author

nagix commented Nov 19, 2018

I tried Arial with different browsers on both Mac and Windows, but the results are not consistent at all (reference). Eventually, I reverted the changes in labels and added some adjustment to pixel values so that differences across environments stay within the margin range (+/-2).

@nagix
Copy link
Contributor Author

nagix commented Nov 19, 2018

It was passed on Mac and Windows, but still fails on Linux.

@nagix nagix changed the title Use empty labels for the legend tests so as not to be affected by the font width Use short labels for the legend tests so as not to be affected by the font width Nov 20, 2018
@nagix
Copy link
Contributor Author

nagix commented Nov 20, 2018

Now all the test passed. The results were more consistent when ' ' is used for labels.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @nagix, it passes on my machine now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants