search

maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.58k stars 707 forks source link

Add WebGL 2 capabilities to test suites #2420

Closed birkskyum closed 1 year ago

birkskyum commented 1 year ago

Rationale

Both unit tests, integration tests and render tests currently rely heavily on the gl npm package, which is the undermaintained headless-gl. It rely on an unmaintained version of ANGLE, which is a distant fork of the the google repo.

The gl package has been good enough up until now for development to run our tests in node, but unfortunately it has only WebGL 1 support, and with WebGL 2 having wide support, and WebGPU being released within a week, we need a more modern testing story. This lack of modern support has already proved problematic in pr's like #1891 , and it'll only get worse. The google ANGLE repo actually got WebGL 2 support, not to long ago, but there is slim hope that will ever be available in the ANGLE fork used by the gl package. There has been an open WebGL 2 ticket since 2017, so I wouldn't hold my breath on that one https://github.com/stackgl/headless-gl/issues/109#issuecomment-1374537584

How does the current test suites work?

Currently, our unit tests are run with Jest in a JSDOM environment. In each unit test, the first thing we call is beforeMapTest(), which is a lot of browser mocking code we have in /src/util/util.ts. Part of that mocking process is creating a WebGL context, and that is where we pull in the gl package to help us out with that.

For render tests, we don't use JSDOM or beforeMapTest, but instead run the tests with the jest node environment, and here we additionally run the /test/integration/render/mock_browser_for_node.ts to create all the globals like global.navigator,

How could we do?

We could pull in a browser through either playwright or puppeteer, create an actual canvas, and a webgl/webgl2/webgpu context inside. This would allow us to delete all the mocking code for the integration/render tests, and have features as they get added to the evergreen browsers. The unit tests still need the mocks, because it's necessary to generate the coverage reports.

birkskyum commented 1 year ago

Ah, okay - I thought it was possible to propose any bounty size now. I'd prefer to keep the this perfectly confined issue instead of splitting it up solely to accommodate the bounty system, as it's hard to find good logical boundaries of this work. If it solves the problem, I won't mind a 3k bounty.

birkskyum commented 1 year ago

Made an extra void issue too as requested that can be used for this.

HarelM commented 1 year ago

The main point of another issue is to split the work so that part of it can be delivered and rewarded separately, sorry if that wasn't clear.

HarelM commented 1 year ago

For example this issue which was partially referenced here: https://github.com/maplibre/maplibre-gl-js/issues/2411

birkskyum commented 1 year ago

I changed the other issue to be the swap of Playwright to Puppeteer, because it's the only step I think can be taken separately without leaving the codebase in a broken state. #2481

birkskyum commented 1 year ago

I also added a separate issue for splitting out render tests to a separate workflow, as it's also possible to do separately #2482

birkskyum commented 1 year ago

I don't see how this can be split further without reaching a strange intermediate state where tests doesn't pass.

HarelM commented 1 year ago

Assigning bounty XL. Bounty direction: https://github.com/maplibre/maplibre/issues/192 Requires:

2482 #2481 #2411

HarelM commented 1 year ago

As part of this issue there's a need to see if we can share the build dist results between workflows to reduce a few minutes more in the CI. If this is big, we can split to another issue, but the goal is to keep the CI at around 10-15 minutes.

birkskyum commented 1 year ago

It's of course good to free up runners faster, by avoiding duplicate work, but unless some of the test workflows are queued due to a lack of runners, I can't see how it would reduce the overall runtime because the build-dist processes run in parallel.

HarelM commented 1 year ago

You have a point there... My mistake.

birkskyum commented 1 year ago

Can I start this task? There is a lot of debugging left to do here, and decisions regarding updates of 'expected' pictures, but it'll be more clear in the draft PR what the status is.

HarelM commented 1 year ago

Enjoy! :-)

birkskyum commented 1 year ago

OC - https://opencollective.com/maplibre/expenses/138575