ajaxerror not serialized properly

[oberhamsi]

fixes #4024

the original cause for load not firing was that source_cache did not see the 404 returned by a failed tile request here https://github.com/maplibre/maplibre-gl-js/blob/main/src/source/source_cache.ts#L172

this was caused by AjaxError no longer being serialized since commit d68b919#diff-0c7952cb97b3abe689d267fa5663ac8d387fbd6a637905cce9581a5cc2f1ff1c AjaxError is registered for transfer serialization but it's also an instance of Error and was thus just returned

i think the worker_transfer tests are better if they use structuredClone. this works for the other tests too but not for the once check against Blob - seems to be a bug with jsdoms custom Blob in combination with nodes native structuredClone. i'm hoping this will be fixed with jsdom/jsdom#3363

if you don't like the rather big change to the tests i can revert this and not do any tests for this issue.

Launch Checklist

• 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.
• Link to related issues.
• [x ] Write tests for all new functionality.
• Add an entry to CHANGELOG.md under the ## main section.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 64.35644% with 36 lines in your changes missing coverage. Please review.

Project coverage is 65.15%. Comparing base (0945152) to head (0146720).
Report is 10 commits behind head on main.

Files	Patch %	Lines
src/util/web_worker_transfer.ts	64.35%	19 Missing and 17 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #4211       +/-   ##
===========================================
- Coverage   87.75%   65.15%   -22.60%     
===========================================
  Files         242      242               
  Lines       33082    33081        -1     
  Branches     2178     1427      -751     
===========================================
- Hits        29032    21555     -7477     
- Misses       3067    10701     +7634     
+ Partials      983      825      -158     

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

[oberhamsi]

i had so much trouble with jsdom+structuredclone that i wrote a quick demo how it really behaves in browsers, might be helpful to understand what i did https://codepen.io/oberhamsi/pen/VwOpYqz

hm, during manual testing i noticed problems with this approach. i'll wait for feedback but will probably handle the check in worker_transfer differently

Error: can't serialize object of unregistered class DOMException at serialize (5ba59c91-290f-49b7-91c6-9fd6b8a70c33:13103:19)

[HarelM]

Thanks for taking the time to look into this!

[oberhamsi]

thanks for the quick feedback! i'll keep working on it

[oberhamsi]

i'll look into the tests. it's not due to the environment :/

[HarelM]

The coverage of these methods wasn't great before you changed the code :-(
So the only way to know the logic wasn't changed is by reviewing the code manually, which isn't a great idea...
What are the odds you might be willing to fully cover this class with tests?
Sorry to nag, I'm just not sure from reading the code that it has the same logic...

[oberhamsi]

i've added a few more tests and will keep looking into it.

thanks again for the feedback.

[oberhamsi]

test coverage is now good but if you're not happy with the sweeping changes, what do you think about reverting the refactor? 7fd8cab

you didn't like the duplication but they were (mostly) present before and i don't see how to make this significantly better without moving a few things, as i did. this is how it was before the refactor: https://github.com/orfon/maplibre-gl-js/blob/1374644017b439854a4bd1ab5246fe878fe27266/src/util/web_worker_transfer.ts

maybe I can do the refactor once structureClone arrives in jsdom. this would make me feel a lot better about the unit tests.

[HarelM]

The coverage of the file is at about 78%, I would feel better if is was closer to 85-90%.
I think some error flows are needed to be checked.
Overall the changes look good and I think you did a good job of refactoring.
I reviewed the code and it seemed to me mostly the same, but still confusing to compare, especially the $name, constructor etc...
So I think a few more tests should be good and making sure they pass also before the changes in the file to make sure the logic is the same.

[HarelM]

It would also be great if we could solve the following comment:

// (Temporary workaround) allow a class to provide static
        // `serialize()` and `deserialize()` methods to bypass the generic
        // approach.
        // This temporary workaround lets us use the generic serialization
        // approach for objects whose members include instances of dynamic
        // StructArray types. Once we refactor StructArray to be static,
        // we can remove this complexity.

I'm not sure how easy it would be to solve this though. StructArray are generated classes, so it might be easy to change the generation code? I don't know, might be out of scope for this PR.
I just want to pick your brain since you dug deep into this class.

[oberhamsi]

alright! coverage is now high 90s. i think the bot didn't update the coverage properly

[oberhamsi]

regarding serialize. yeah, one thing i don't like about that logic is how implicit it is. imo register() should have an additional parameter hasSerialize boolean. as it is right now, someone can remove serialize() from the StructArray implementation and it would fail but not be obvious that the failure is related to web_transfer

the reverse is also true: the comment tells us only StructArray uses serialize - but is that true? (it is, but it requires manually checking all registered classes)

otherwise i think it's fine to have custom serialize/deserialize if an object needs special handling to get revived after a transfer. i think moving the StructArray.serialize/deserialize into web_transfer would not be an improvement. and it does need special handling.

[oberhamsi]

also wanted to mention again, that we should switch the tests to mock the transfer with structuredClone to properly test the code related to transferable

[HarelM]

Can you update the branch from main? I think the conflicts prevent it from running the CI, maybe...

[HarelM]

Looks good in general.
Can you check if the tests pass (except for the ajax) also for the version of the code of the web worker in main? i.e. see if you can copy paste the web_worker content and see if the tests pass. Since this is a refactoring all the tests (except ajax) should pass in the old version it self.

[oberhamsi]

tests work in main 🥳

i also added an explicit return type in two more places

[oberhamsi]

i also checked the 4 lines in main not covered by the new tests.

two of them are still not covered because i don't think they can be triggered

one is covered once refactored (okay, semi-great). and one line is now removed because of the new Error handling.

[HarelM]

Are you sure about the coverage?
I see the following in the CI log for the unit tests:

web_worker_transfer.ts                     |   84.25 |     78.4 |     100 |   84.25 | 147-150,152-156,158-161,174-175,178-179,183-189,202-203,205-208,211-212,230-231,234-235,238-239,242-243

[oberhamsi]

yeah, that's odd. i'll investigate. some of the lines CI says are not covered, i'm certain they should be hit by the test.

[oberhamsi]

okay this is a fun one.

node 20.10 has a coverage bug jestjs/jest#14766 nodejs/node#51251

if i downgrade to node 20.9 the test coverage for web_worker_transfer is correct. the test coverage for a lot of other files is also different with 20.9

my screenshots where from a run with npx jest --coverage src/util/web_worker_transfer.test.ts --reporters=default which I believe uses codeCoverage=babel. if i change codeCoverage to babel in jest.config.js the coverage output by npm run test-unit-ci looks different (correct for web_worker_transfer).

i would suggest a different MR and switch to codeCoverage=babel. the only benefit of v8 would be speed.

[HarelM]

We are using a special coverage report tool due to the differences between browser test coverage (render tests) and unit test.
The response in this package is usually fast: monocart-coverage-reports
It might be interesting to poke around there to see if we can get the right results.
We can move the build to use 20.9 if there's no solution in later version.
IDK... I don't understand why coverage is reinvented in node and jest... so annoying...

[oberhamsi]

the reporting is fine but the coverage reported by node is just wrong in 20.10

i would suggest switching to babel coverageReporter instead of v8, which is a simple change and the default used by jest anyways. i don't see a reason to use v8 expect for speed.

[HarelM]

These are the relevant PR related to this topic, I honestly don't understand all the details, I just want it to work...

• Replace v8 with monocart for coverage #3598
• Change the coverage for jest to be aligned with render tests #3615

Feel free to create a different PR to do the switch.

[oberhamsi]

I will look into it but probably mid july

[HarelM]

What do you suggest to do? I think general fixes to coverage can wait for a different PR and there's no need to hold this PR hostage...

[oberhamsi]

honestly i haven't worked with jest coverage before, so i don't know how to fix it just for this test.

looking at the jest-docs yesterday, and trying different things, i feel changing this line to babel (the default) should fix the coverage reported. but it will change the coverage for a bunch of files (interestingly not all of them). so i would want to dig deeper before really doing this change. and mabye there's a way to change the coverageProvider only for for the "unit" project in jest.

btw: thanks for all the guidance & hints! i'm really interested in getting this MR done - our current workaround involves setTimeout to check for the loaded state :) but i'll be on holidays now - so for me it's okay if the PR waits a little longer.

[HarelM]

@cenfun any insight regarding the above issue with the coverage? Do you have any tips?

[cenfun]

I don't think it will work if we merge babel coverage (unit test ) with v8 coverage (e2e test). This is why we use Jest v8 coverage providor.
However, there is indeed a bug caused by the upgrade of nodejs 20.10, which may cause Jest's v8 coverage to not work.
I have created a PR to fix the Jest v8 issue, but unfortunately it does not get merged
SimenB/collect-v8-coverage#235

It's been a month already, It seems like it hasn't been fixed, whether it's Nodejs or Jest.
I don't know how to push forward it.

[HarelM]

Thanks for the quick reply @cenfun!

I'll merge this and hope the fix will merged soon...