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

Remove CJK browser test #3162

Merged
merged 6 commits into from
Oct 3, 2023
Merged

Remove CJK browser test #3162

merged 6 commits into from
Oct 3, 2023

Conversation

HarelM
Copy link
Member

@HarelM HarelM commented Oct 3, 2023

This removes the CJK test - the png parse was never observed, the diff was too strict and using base64 text isn't a great way to see what is the expected images.
In the past the render test were ran in node, but now they are running in the browser, so this test is not needed anymore.

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (dd3bea1) 75.09% compared to head (35d5ffb) 75.09%.
Report is 29 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3162   +/-   ##
=======================================
  Coverage   75.09%   75.09%           
=======================================
  Files         240      240           
  Lines       19150    19150           
  Branches     4323     4323           
=======================================
  Hits        14380    14380           
  Misses       4770     4770           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM HarelM mentioned this pull request Oct 3, 2023
8 tasks
@HarelM HarelM changed the title Fix CJK test Remove CJK browser test Oct 3, 2023
@HarelM HarelM enabled auto-merge (squash) October 3, 2023 12:57
test('Marker: correct position', async () => {
const markerScreenPosition = await page.evaluate(() => {
const markerMapPosition = [11.40, 47.30];
const markerMapPosition = [11.40, 47.30] as [number, number];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this doesn't matter, but I would usually prefer to write this as follows to avoid a "cast":

const markerMapPosition: [number, number] = [11.40, 47.30];

Copy link
Collaborator

@neodescis neodescis 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. One minor comment, but feel free to ignore :-)

@HarelM HarelM merged commit 8d40454 into main Oct 3, 2023
14 checks passed
@HarelM HarelM deleted the fix-cjk-test branch October 3, 2023 16:46
HarelM pushed a commit that referenced this pull request Oct 6, 2023
* Port CJK browser test to render test [#3006, #3162]

* add platform-specific expected images for local ideographs render test. [#3166]

* Ubuntu expected is tofu

* github actions ubuntu runner: apt install CJK fonts for render tests [#3166]

* Commit expected Ubuntu local ideographs image with noto fonts installed from apt [#3166]
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.

None yet

3 participants