Migrate from Karma to Web Test Runner

[mourner]

Closes #8939. Migrates from Karma, which was deprecated last year and is no longer maintained, to Web Test Runner, which is a modern successor.

A work in progress — migrated just a few files so far, but it looks like it won't be too difficult to get the whole test suite working.

[jonkoops]

Ok, looks like the majority of the test suite is passing with 758 tests passing and 69 still failing. @mourner I am going to take a break, feel free to fix up some tests as you see fit.

[mourner]

@jonkoops some spec files aren't processed, failing with this error — not sure what's going on here and why it can't resolve chai:

TypeError: Failed to resolve module specifier "chai". Relative references must start with either "/", "./", or "../".
        at node_modules/@web/test-runner-mocha/dist/autorun.js:1:267419

I added the import map chai URL that you removed back for now, but feel free to fix this if you figure out why local one doesn't resolve for some files.

[mourner]

Now down to 786 passed, 24 failed.

[jonkoops]

Very strange that chai resolves from some modules, but not from others, that doesn't make a lot of sense. Perhaps a bug in Web Test Runner.

[Falke-Design]

I don't know what to think of the library yet. It is very difficult to debug, the manual server keeps crashing and apart from the documentation there is hardly any help on the Internet

[mourner]

the manual server keeps crashing

@Falke-Design hasn't crashed on me yet; any stack traces in the console when this happens?

[Falke-Design]

Saddly not. It simply terminates the command:

Now I commeted out a line Icon.Default.js and then I added a debugger to a test. Maybe it has something to do with only, don't know ...

it.only('detect icon images path', () => {
	    debugger;
		const origPath = Icon.Default.imagePath; // set in after.js
		expect(origPath).to.be.ok;
		delete Icon.Default.imagePath;
...

[jonkoops]

This is not working because the css file leaflet.css is not included.

This is now fixed, all tests except one are now passing, which is the one related to setup.js not being included. False alarm, the test suite was terminating pre-maturely.

[jonkoops]

I don't know what to think of the library yet. It is very difficult to debug, the manual server keeps crashing and apart from the documentation there is hardly any help on the Internet

Yes, I am having the same issue, the server keeps crashing (without any sort of logs). This is happening on my Apple Silicon Mac, so it seems this is a cross-platform issue, as @Falke-Design is running his stuff on Windows.

[jonkoops]

Updated the CI so that it will now run the tests on Playwright proper, and it looks like the same bug @Falke-Design and I were having is showing up there as well. This seems like a blocker for us to port the tests to Web Test Runner, it will likely need to be fixed upstream.

I've logged modernweb-dev/web#2636 to hopefully get this resolved.

[jonkoops]

@mourner @Falke-Design I've created an alternative implementation to this PR using Vitest (see #9243). This is just to play around and see if we might consider it a valid option, considering the trouble with Web Test Runner at the moment.

[mourner]

OK, now I can reproduce the weird quite crash. It happened randomly before because of concurrency (multiple tests were run in parallel) — if you switch to concurrency: 1, it seems to fail in the same spot. Now need to figure out where exactly... Will possibly need to write our own reporter because the existing ones don't give enough info.

[mourner]

Narrowed it down to crashing specifically in MapSpec.js...

[mourner]

OK, found it — the crash happened because of Geolocation methods. I think this specific construct is what's probably causing the crash, as Web Test Runner may depend on it in some way:

Object.defineProperty(window, 'navigator', {value: ...});

[mourner]

OK, looking a bit better now:

Chromium: |██████████████████████████████| 49/49 test files | 1072 passed, 1 failed, 15 skipped
Firefox:  |██████████████████████████████| 49/49 test files | 1073 passed, 7 failed, 8 skipped
Webkit:   |██████████████████████████████| 49/49 test files | 1053 passed, 12 failed, 14 skipped

A few other comments aside from the test failures:

• Installing Playwright browsers in the test jobs can take up to 2 minutes. We could probably cache those.
• Now that we run all three browsers in the same job, it might be worth combining them with the setup jobs — will probably make the workflow faster. For platform-agnostic stuff like lint/etc., we could probably have a separate dedicated setup job.
• Not sure what's our plan for additional flags for browser environments we had before like NoTouch or Retina.

[mourner]

Another issue is that the workflow overall takes about 2+ times longer than the Karma-based one, due to various factors I guess. Ideally we'd want to bring this down so that it's closer. Caching Playwright will help but we need to look into other optimizations.

[Falke-Design]

I mean that i saw that we can run the test files parallel

[mourner]

@Falke-Design yes, I turned it back on — it runs in parallel now but is still 2x slower.

[jonkoops]

Some more strange behaviour I am seeing is Map.TouchZoomSpec.js failing, but only if I run the full suite:

Works

npx web-test-runner spec/suites/map/handler/Map.TouchZoomSpec.js

Breaks

npx web-test-runner "spec/suites/**/*.js"

Perhaps we have some tests that are not cleaning stuff up properly?

[jonkoops]

Added some code to load the setup file, and we're now loading the Leaflet CSS as well, but it seems that the test is still failing:

❌ Icon.Default > detect icon images path
AssertionError: expected 'http://localhost:8000/dist/images/' to equal 'http://localhost:8000/base/dist/image…'
+ expected - actual

  -http://localhost:8000/dist/images/
  +http://localhost:8000/base/dist/images/
  
  at n.<anonymous> (spec/suites/layer/marker/Icon.DefaultSpec.js:29:37)

This seem however to be the correct path to the image files, so perhaps we just need to update this test to use the new path? @mourner @Falke-Design

Either way, this is the last test that is failing on Chromium, if we get this resolved we just have to figure out what is breaking in Firefox and Webkit.

[jonkoops]

Yeah, seems like this assertion is different in the new scenario, I have updated the paths, now all tests are passing on Chromium!

[jonkoops]

Managed to get the tests in Firefox passing as well.

[jonkoops]

Ok, I think everything is working pretty consistently now. The main issue is that the tests seem to fail on Webkit, specifically when creating a TouchEvent in Prosthetic Hand.

TypeError: Type error
  at _createTouchEvent (node_modules/prosthetic-hand/lib/Hand.js:348:25)
  at _dispatchEvents (node_modules/prosthetic-hand/lib/Hand.js:278:40)
  at node_modules/prosthetic-hand/lib/Hand.js:489:26

If someone wants to look at this be my guest, because I am tired and calling it for today 😪

[mourner]

Summoning @IvanSanchez for thoughts on the prosthetic hand thing, looks pretty cryptic...

[jonkoops]

Running the tests on Safari locally (using --manual) makes them pass. I am not sure what we should do in terms of committing to Playwright as it seems that these are some kind of custom build of browsers. Perhaps using a different type of browser runner would be best, so we can test against actual builds of Firefox, Chrome and Safari.

[jonkoops]

After switching over to @web/test-runner-webdriver, which uses just a regular old-school WebDriver, all tests seem to pass just fine! And, it also removes the additional setup work that was required for Playwright.

This means we are now testing against Firefox, Chrome and Safari (macOS only) on Linux, macOS and Windows! 🥳

[jonkoops]

So I think we still need to resolve the following things before this PR is ready to merge:

• Fix skip()ed tests
• Update the contribution guide to reflect the new workflow
• Figure out how we can test the 'no touch' and 'retina' variants

@mourner @Falke-Design WDYT? Feel free to pick up one of these tasks.

[mourner]

@jonkoops wow, that looks great! And they seem to run much faster on CI now — even faster than the current Karma setup.

One more remaining problem is that it doesn't work for me locally. 😬 Running npm test, it waits for a second and then exists without any tests running or any errors in the console (same for watch mode). Manual mode works, so it must be something WebDriver-related.

[jonkoops]

Strange, I am able to run the tests both on my Linux and macOS machines (don't have a Windows machine). I wonder what is causing it.

[Falke-Design]

npm test works for me on Windows.

Without any investigations: It is interessting, that Firefox runs 7 tests more.

[Falke-Design]

I discovered another bug. When I enable the #locate tests in MapSpec it crashes but without any error:

[jonkoops]

@Falke-Design that is likely the same bug @mourner mentioned in #9242 (comment), those tests were disabled by him under 537c2ed.

[jonkoops]

Strangely enough, the same test passes just fine when running npm test -- --manual.

[Falke-Design]

I fixed the geolocation test. web-test-runner relays on userAgent and serviceWorker so we need to keep them in the navigator object.

Object.defineProperty(window, 'navigator', {
	value: {
		userAgent: window.navigator.userAgent,
		serviceWorker: window.navigator.serviceWorker,
	}
});

[Falke-Design]

We could change the import map from 'leaflet': '/src/Leaflet.js', to 'leaflet': '/src/LeafletWithGlobals.js', but then the object is not extendsible. A workaround could be following: window.L = {...window.L} but then the child objects are still not extendable and need to be overwritten with the workaround too window.L.Util = {...window.L.Util}. I don't think that it makes sense to test this, if we need a lot of workarounds

[jonkoops]

@Falke-Design I was toying with the idea that we could perhaps use a Proxy for the geolocation stuff, rather than using defineProperty(), that would allow us to not mutate the rest of navigator. WDYT?

[Falke-Design]

@Falke-Design I was toying with the idea that we could perhaps use a Proxy for the geolocation stuff, rather than using defineProperty(), that would allow us to not mutate the rest of navigator. WDYT?

Sounds good, if it works, I have never worked with Proxy

[jonkoops]

Sounds good, if it works, I have never worked with Proxy

I'll do some experimentation in the coming days, let's see if we can wrap this up.

[jonkoops]

I've done a quick rebase and squash to get rid of the merge conflicts.

[mourner]

I don't know what changed since the last time, but it kinda works for me locally now! Although it only runs tests on Firefox:

• Chrome: Failed downloading chromedriver v121.0.6167.184: Download failed: server returned code 404. URL: https://edgedl.me.gvt1.com/edgedl/chrome/chrome-for-testing/121.0.6167.184/mac-arm64/chromedriver-mac-arm64.zip, retrying ...
• Safari: Failed to create session. Could not create a session: You must enable the 'Allow Remote Automation' option in Safari's Develop menu to control Safari via WebDriver.

Perhaps worth defaulting to only one runner for local npm test (making development easier), and run all three only on CI?

[jonkoops]

Perhaps worth defaulting to only one runner for local npm test (making development easier), and run all three only on CI?

Yeah, I think we can run the local tests on Chrome only. I'll make something.

[jonkoops]

So, I've been doing some research and the more I look at it, the more I think we might be better off using WebdriverIO directly, rather than through Web Test Runner. It's very actively maintained, and it has a lot more fine grained control and features, and arguably more complete and up-to-date documentation.

For example, we can get rid of prostetic-hand and use this fantastic built-in API for gestures, which I think we can use to get rid of our conditional touch testing, and help us migrate fully to pointer events. It comes with it's own runner, a built-in assertion library (with a migration path for Chai), and also has support for popular testing frameworks.

I am going to do some research to see if there are any blockers adopting this. I think a lot of the work that has happened in this PR will port over without modification. Sorry I haven't been able to spend more time on this recently, my days have been more busy than expected.

@mourner @Falke-Design WDYT? What's your take on this?

[jonkoops]

I've merged all the changed imports in a seperate PR (#9284), that should make it a bit easier to review this PR.

[mourner]

@jonkoops I'm not very familiar with WebdriverIO, but if we can switch to its pure form without too much pain, I'm all for it! The arguments sound enticing, and I'm always up for removing wrappers where possible.