test-render fails since migration to typescript

[wipfli]

maplibre-gl-js version:

lastest

browser:

Steps to Trigger Behavior

1. npm ci
2. npm run build-dev
3. npm run test-render on windows or macos

Link to Demonstration

https://github.com/maplibre/maplibre-gl-js/pull/321/checks?check_run_id=3558826291

Expected Behavior

Test passes

Actual Behavior

file:///Users/runner/work/maplibre-gl-js/maplibre-gl-js/test/suite_implementation.js:160
                now += operation[1];
                ^

ReferenceError: now is not defined

Maybe this is a code block in suite_implementation.js which was not accessed before. The problem lies in suite_implementation.js and not in the tests or actual source code.

[wipfli]

maplibre-gl-js/test/suite_implementation.js

Lines 158 to 164 in 75cddd8

	} else if (operation[0] === 'wait') {
	if (operation.length > 1) {
	now += operation[1];
	map._render();
	applyOperations(map, operations.slice(1), callback);
	} else {

[HarelM]

I removed some of the "now" references. How come this started to fail when ci was green the entire time? Race condition?

[wipfli]

I think test-render was never run on macos before. We only run it on windows here:

maplibre-gl-js/.github/workflows/release.yml

Line 55 in 75cddd8

npm run test-render

This does not explain however why this did not fail before on windows.

[HarelM]

Weird... Let me know if you need my help trying to solve this...

[wipfli]

After a series of confusing GitHub workflow runs I came to the conclusion that test-render has never been effectively used by continuous integration in this project. In ci.yml the script gets only mentioned in a comment but is never actually called. In release.yml the job lint_build_test calls test-render but despite its name this job never actually builds the library. This in turn makes test-render silently throw some errors but still the script exits with code 0 which seems to be a bug related to the custom harness in test/integration/lib/render.js.

[wipfli]

#343

[HarelM]

The following test for example seems to fail in both 1.x and latest version.
It doesn't use any tiles:
failed combinations/background-opaque--symbol-translucent
I'm not sure I know what should be our next steps...

[wipfli]

It is true that many render tests don't need a tile source. Others, like background-opacity/image have a local source:

  "sources": {
    "satellite": {
      "type": "raster",
      "tiles": [
        "local://tiles/{z}-{x}-{y}.satellite.png"
      ],
      "maxzoom": 17,
      "tileSize": 256
    }

Is this local source provided by the test harness?

[wipfli]

Are there render tests which call online mapbox:// sources?

[wipfli]

Are there render tests which call online mapbox:// sources?

The only occurrence of mapbox: in files in the folder test/integration/render-tests is in tile-mode/streets-v11/style.json:

{
    "version": 8,
    "metadata": {
        "test": {
            "mapMode":"tile",
            "debug": "true",
            "allowed": 0.0005,
            "operations": [
                [ "setStyle",  "mapbox://styles/mapbox/streets-v11"]
            ],
            "pixelRatio": 2
        }
    },
    "center": [-73.9959, 40.7106],
    "zoom": 13
}

@msbarry can you elaborate why an online tile source is needed for the render tests? I have the impression that the necessary tiles are contained in the folder test/integration/tiles.

[wipfli]

Having a closer look at the first render test that fails on ubuntu. I am running

xvfb-run -s "-ac -screen 0 1280x1024x24" npm run test-render debug/overdraw

And it fails at the latest commit which is 37bcaf5.

[wipfli]

Rather than installing node 10 on my machine I prefer using GitHub actions to find out when or if this test ever passed. Doing this in pull request #365.

[HarelM]

I have a feeling that the number of failing tests before and after typescript is not that different.
We might find tests that needs to be fixed, don't get me wrong and the approach you took in #365 is great to understand which are failing and why, but the main question is what do we do with the test that fail in the version before typescript, as there are many failing tests there, not just a handful: do we remove them in latest version? do we fix them in latest version? etc...

[wipfli]

Based on https://github.com/maplibre/maplibre-gl-js/runs/3644885743?check_suite_focus=true only a single render test did not pass in the last commit before typescript.

[wipfli]

My intuition is that the changes related to mat4 had an impact rendering, for example in https://github.com/maplibre/maplibre-gl-js/blob/main/src/geo/transform.ts.

[HarelM]

Yes, I have that feeling as well. The changes there are relatively easy to revert and see if it helps. I don't think this is the only root cause though...

[wipfli]

While many render tests fail since the typescript migration, regressions/mapbox-gl-js#9009/style.json even makes the whole test process crash. Running

xvfb-run -s "-ac -screen 0 1280x1024x24" npm run test-render regressions/mapbox-gl-js#9009

Results in the following error:

<--- Last few GCs --->

[182868:0x486d2a0]    24570 ms: Scavenge (reduce) 2046.0 (2081.6) -> 2045.2 (2081.6) MB, 1.9 / 0.0 ms  (average mu = 0.171, current mu = 0.006) allocation failure
[182868:0x486d2a0]    24574 ms: Scavenge (reduce) 2046.0 (2081.6) -> 2045.3 (2081.6) MB, 2.5 / 0.0 ms  (average mu = 0.171, current mu = 0.006) allocation failure
[182868:0x486d2a0]    24577 ms: Scavenge (reduce) 2046.1 (2081.6) -> 2045.4 (2081.9) MB, 2.2 / 0.0 ms  (average mu = 0.171, current mu = 0.006) allocation failure


<--- JS stacktrace --->

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0xa7ac10 node::Abort() [node]
 2: 0x9a3c4d node::FatalError(char const*, char const*) [node]
 3: 0xc63f9e v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]
 4: 0xc64317 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [node]
 5: 0xe2dd95  [node]
 6: 0xe2e93c  [node]
 7: 0xe3c2cb v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 8: 0xe3ffcc v8::internal::Heap::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node]
 9: 0xe03eea v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationType, v8::internal::AllocationOrigin) [node]
10: 0x115accb v8::internal::Runtime_AllocateInYoungGeneration(int, unsigned long*, v8::internal::Isolate*) [node]
11: 0x14f1e19  [node]
Aborted

Maybe this could be a good starting point to investigate which changes introduced with typescript are to blame.

Note that doubling memory to --max-old-space-size=4096 does not solve this problem.

[HarelM]

Yes, I've seen this crash as well when running locally unfortunately :-( I have no clue where to start looking into it. There are a few other changes besides typescript - node 14, packages upgrades etc. I don't know where to start honestly... :-(
Maybe we can post on slack that we need help with this one?

[wipfli]

I made a website which shows roughly 80 render tests which currently fail. Some of them are really ugly:

https://wipfli.github.io/debug-test-render/

[wipfli]

One of my favorite is this one:

[HarelM]

That's an excellent preview :-) feels like a lot of the failures are related to "accuracy".
Let's try and revert as many of the transform changes as possible, as you suggested and see what we are left with, what do you think?

[HarelM]

I've reverted locally the changes in all files containing mat4.create or fromValues and it seems a lot of test are passing now.
Unfortunately, the only way to get this passing in typescript is to use as any in a lot of these places...
How can I make a PR with the relevant changes in order to see the render test runs? Do you want to create a branch for this so we can collaborate the work?

[HarelM]

I've created #367 to allow collaboration on a branch.
Still 171 tests fail there:

1029 passed (84.9%)
1 passed but were ignored (0.1%)
10 ignored (0.8%)
171 failed (14.1%)
1 errored (0.1%)

[msbarry]

@msbarry can you elaborate why an online tile source is needed for the render tests? I have the impression that the necessary tiles are contained in the folder test/integration/tiles.

Ah I thought that tests like this were using a remote datasource, but I see now that some of the low zoom tiles are served directly out of the repo. That's great we only need a data provider for one of them.

[chippieTV]

Sorry these are my changes to mat4 etc. :(

regarding raster-extent/maxzoom (@wipfli's favorite), the only required fix from main seems to be src/geo/transform.ts:733

from this.invProjMatrix = mat4.invert(mat4.create(), this.projMatrix);
to this.invProjMatrix = mat4.invert([] as any, this.projMatrix);
or this.invProjMatrix = mat4.invert(new Float64Array(16) as any, this.projMatrix); // I expect better/faster

if you look at the source of mat4.invert (which as far as I remember I did for any where I wasn't sure, to confirm these changes should be functionally equivalent), it didn't immediately make sense to me why this original change would not work..

the signature is like this function mat4.invert(out: mat4, a: ReadonlyMat4): mat4

inside you see all 16 elements get explicitly set so it shouldn't matter what type of array is in the output.. however the input array is actually set on line 712 as Float64Array.

let m = new Float64Array(16) as any;

then reassigned on line 732 this.projMatrix = m;

One of the 'problems' here is that overriding the type system with any makes the tooling less useful. It might be good to consider making a 64bit version/extend glmatrix to play better with Typescript for things like this..

I know this is just one fix, but I think the majority of the type changes with mat4 are correct, but of course it only takes one broken one to cause havoc . :( sorry again, I'll have a look at the others too

[HarelM]

Don't worry about it. I reviewed it and missed it as well. I thought it was the right direction too.
Once we fix the tests we'll see how to improve the typings. I agree this needs to be fixed.
I have created a PR and branch with some reverts, take a look and see if you spot other possible reverts.

[wipfli]

Absolutely no problem @chippieTV. Thanks for all the contributions you made. Interestingly, I was just editing in my copy exactly the thing you proposed:

-this.invProjMatrix = mat4.invert([] as any, this.projMatrix);
+this.invProjMatrix = mat4.invert(new Float64Array(16) as any, this.projMatrix);

But so far still 80 tests fail.