-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Migrate from Karma to Web Test Runner #9242
base: main
Are you sure you want to change the base?
Conversation
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. |
@jonkoops some spec files aren't processed, failing with this error — not sure what's going on here and why it can't resolve
I added the import map |
Now down to 786 passed, 24 failed. |
Very strange that |
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 |
@Falke-Design hasn't crashed on me yet; any stack traces in the console when this happens? |
Saddly not. It simply terminates the command: Now I commeted out a line
|
|
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. |
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. |
@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. |
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 |
Narrowed it down to crashing specifically in |
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: ...}); |
OK, looking a bit better now:
A few other comments aside from the test failures:
|
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. |
I mean that i saw that we can run the test files parallel |
@Falke-Design yes, I turned it back on — it runs in parallel now but is still 2x slower. |
Some more strange behaviour I am seeing is Works npx web-test-runner spec/suites/map/handler/Map.TouchZoomSpec.js Breaks
Perhaps we have some tests that are not cleaning stuff up properly? |
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:
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. |
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
If someone wants to look at this be my guest, because I am tired and calling it for today 😪 |
Summoning @IvanSanchez for thoughts on the prosthetic hand thing, looks pretty cryptic... |
Running the tests on Safari locally (using |
After switching over to This means we are now testing against Firefox, Chrome and Safari (macOS only) on Linux, macOS and Windows! 🥳 |
So I think we still need to resolve the following things before this PR is ready to merge:
@mourner @Falke-Design WDYT? Feel free to pick up one of these tasks. |
@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 |
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 that is likely the same bug @mourner mentioned in #9242 (comment), those tests were disabled by him under 537c2ed. |
Strangely enough, the same test passes just fine when running |
I fixed the
|
/* global L */ | ||
import 'leaflet'; | ||
|
||
describe('General', () => { | ||
it('namespace extension', () => { | ||
it.skip('namespace extension', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@Falke-Design I was toying with the idea that we could perhaps use a |
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. |
d7a4a35
to
77456c0
Compare
I've done a quick rebase and squash to get rid of the merge conflicts. |
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:
Perhaps worth defaulting to only one runner for local |
77456c0
to
4e837f4
Compare
Yeah, I think we can run the local tests on Chrome only. I'll make something. |
4e837f4
to
8e0f9e9
Compare
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 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? |
Co-authored-by: Jon Koops <jonkoops@gmail.com> Co-authored-by: Florian Bischof <design.falke@gmail.com>
8e0f9e9
to
323ffed
Compare
I've merged all the changed imports in a seperate PR (#9284), that should make it a bit easier to review this PR. |
@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. |
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.