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

Adds missing test to CJK issue #1018

Merged
merged 5 commits into from
Feb 24, 2022
Merged

Adds missing test to CJK issue #1018

merged 5 commits into from
Feb 24, 2022

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Feb 23, 2022

Adds missing test to #1002

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Write tests for all new functionality.

It's not ideal as the image is saved in base64 format and is compared exactly, but I didn't want to complicate the test to use PNG and pixelmatch libraries...

@github-actions
Copy link
Contributor

github-actions bot commented Feb 23, 2022

Bundle size report:

Size Change: 0 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file Before After Change
maplibre-gl.js 185 kB 185 kB 0 B
maplibre-gl.css 9.25 kB 9.25 kB 0 B
ℹ️ View Details No major changes

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 23, 2022

This is failing as the generated image is the following:
image
Need to understand why this is not working in CI :-/

Copy link
Contributor

@wipfli wipfli left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@wipfli wipfli merged commit 5105467 into main Feb 24, 2022
@wipfli wipfli deleted the cjk-characters-test branch February 24, 2022 14:30
@wipfli
Copy link
Contributor

wipfli commented Feb 24, 2022

Haha, running the browser tests locally now looks like this:

image

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 24, 2022

I know :-) but at least you can use the received text, save it to a html file and see how it looks in the browser.
These were my intentions, although not very user friendly, but a lot less complicated than render tests...

@birkskyum
Copy link
Member

birkskyum commented Jul 23, 2022

How come the CI passes when this test fails? Is it because it only fails when we run it locally?

@wipfli
Copy link
Contributor

wipfli commented Jul 23, 2022

I think so, yes. If I remember correctly, @HarelM chose to use base64 image encoding such that one can easily copy-paste the CI actual result to the expected result in version control...

@HarelM
Copy link
Collaborator Author

HarelM commented Jul 24, 2022

There are probably better ways to do it, but I wanted to have a CJK test in the CI as it's not possible to do it in the render tests.
Yes, it fails locally unfortunately.
A better way would've been to use pixelMatch like in the render tests I guess, but I didn't want to over complicate the browser test... Feel free to refactor this...

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

Successfully merging this pull request may close these issues.

3 participants