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

Make render tests more reliable #1007

Closed
HarelM opened this issue Feb 17, 2022 · 8 comments
Closed

Make render tests more reliable #1007

HarelM opened this issue Feb 17, 2022 · 8 comments

Comments

@HarelM
Copy link
Collaborator

HarelM commented Feb 17, 2022

Motivation

Render tests are probably the most important and powerful tests we have to make sure things are not breaking.
Currently the tests are running in node environment using a lot of mocking of the browser code.
This is far from ideal as in some cases we can't relay on node env to provide the same results as the browser.
An example for this is #1002 and #1005 which shows the issue.

Design Alternatives

We should consider migrating the render test to use playwright/cypress/other browser runner for the tests.
After #1003 will be merged it should be much simpler to actually do this as the code is a lot simpler, hopefully.

Design

Still need to fully investigate what are our alternatives

Implementation

This should only affect the render tests.

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 17, 2022

I've experimented with jest and playwright to run the tests in the browser.
It seems possible, especially on the #1003 branch.
The problem is that it's even slower than jest on node which takes 100 render test about 240 seconds (4 minutes).
I've tried to parallelize the browser pages (tabs) but it doesn't seem to improve the situation much (reduces the time to 180 seconds for 100 tests - 10% of the tests).
It might be possible to split it to different processes, but I'm not sure it'll be fast enough even then.
There's also might be a way to not create the map on each page but rather use the same map and only load the style, not sure... It requires more investigation in any case...

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 20, 2022

I tried to use playwright with the modified version of render-test and browser-pool package to improve performance, but it's still just too slow.
I tend to say that for CJK issues we'll create specific test to make sure this is done correctly in the browser and keep the current tests running in node, we have a few browser tests as a reference for how to add new tests.
@Kanahiro, any chance I can enlist you to try and hack this?
Check out browser.test.ts file for all the browser tests.

@Kanahiro
Copy link
Contributor

I'm reading browser.test.ts.
Now test-browser seems to fail in CJK characters, I'll check this first.

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 27, 2022

It might be failing locally, but it passes on the CI server.
But feel free to make it more robust :-)

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 27, 2022

I forgot to reference the relevant PR: #1018.

@Kanahiro
Copy link
Contributor

Now test-browser seems to fail in CJK characters

This was just my misundestanding, sorry.

https://github.com/Kanahiro/maplibre-gl-js/tree/%231007_browser_test/test/integration/browser
I've tried restructuring test/integration/browser, what do you think?

I did

  • separate fixtures into dirs by situations, like render-test
  • compare snapshots as image (this make console more silence)

@HarelM
Copy link
Collaborator Author

HarelM commented Feb 27, 2022

It might be an overkill to separate to folders as there are currently 3 tests, but I don't know, your call...
The main question here is how to compare the images.
I didn't want to over complicate the test with images compare logic as currently exists in test-render, but it's probably the right solution.
In any case, it's not enough to separate to folders, you also need to execute the relevant test for each folder, which is different when it comes to interaction and images comparison.
In any case, any improvement is welcome, feel free to submit a PR and I'll be happy to review it :-)

@HarelM
Copy link
Collaborator Author

HarelM commented Mar 12, 2022

Closing as I'm not planning to migrate the render tests to the browser at that point as it takes them too long to run.
In special cases we can use the browser tests as was done in this case.

@HarelM HarelM closed this as completed Mar 12, 2022
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

No branches or pull requests

2 participants