libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
8.73k stars 1.64k forks source link

Add Emscripten (downstream) tests for SDL 2 here #9103

Open ericoporto opened 4 months ago

ericoporto commented 4 months ago

It would be nice if the tests used by Emscripten for SDL2 could exist here so failures could be catched earlier.

https://github.com/emscripten-core/emscripten/blob/3319a313d3b589624d342b650884caaf8cd9ef30/test/browser/test_sdl2_gfx.cpp https://github.com/emscripten-core/emscripten/blob/3319a313d3b589624d342b650884caaf8cd9ef30/test/browser/test_sdl2_unwasteful.cpp https://github.com/emscripten-core/emscripten/blob/3319a313d3b589624d342b650884caaf8cd9ef30/test/browser/test_sdl2_key.c https://github.com/emscripten-core/emscripten/blob/3319a313d3b589624d342b650884caaf8cd9ef30/test/browser/test_sdl2_text.c https://github.com/emscripten-core/emscripten/blob/3319a313d3b589624d342b650884caaf8cd9ef30/test/browser/test_sdl2_image.c

Problem is I have no idea how feasible this would be and how much work. Emscripten doesn't even use GitHub Actions and they have a much more complex testing infrastructure.

icculus commented 1 month ago

These don't look like they add any testing we don't already have in our own test directory (which we build on GitHub Actions for Emscripten, afaik.)

icculus commented 1 month ago

(If I'm incorrect, feel free to let me know, though!)

ericoporto commented 1 month ago

I don't get it, the tests in actions don't run in the browser, the way Emscripten does is it runs in the browser and takes screenshots for checking things. It also passes messages. It's a more complex test infrastructure though. The only thing that can't be automatically tested and has to be manually ran in Emscripten are the audio tests, it does do automatic checks but it has to be manually ran because the browser doesn't have a way to automatically enable sound - you have to click on the browser screen once to enable sound and then run the auto tests.

icculus commented 1 month ago

Ah, I meant the test sources themselves.

Our test apps all work in Emscripten, but we don't currently have the infrastructure to run those tests automatically on GitHub Actions, and I'm not sure how much work that would be. I'll reopen this for now and toss it out to the 3.x milestone, but I'm not sure if/when someone will work on this.

(Also CC'ing @madebr in case there's a super-easy answer for this, but he's doing a million other tasks right now, too.)

madebr commented 1 month ago

I had an idea about automatically running the tests using selenium, which is available on the GitHub runners. But I have zero experience with this, so any help would be appreciated.

madebr commented 1 month ago

@ericoporto I'm trying to add ci for emscripten in #9937, but some tests of our test suite are failing. Some love by an emscripten developer would be appreciated.

ericoporto commented 1 month ago

Amazing work @madebr ! I am not really an emscripten developer - but maybe I can pretend, lol. Looking at the failures they seem to be in

I would need to try this locally with Selenium to see what is going on specifically in a debugger before saying anything.

ericoporto commented 1 month ago

@madebr I think Emscripten is missing SDL_JOYSTICK_VIRTUAL in it's config, or at least I don't see it in https://github.com/madebr/SDL/blob/main/include/build_config/SDL_build_config_emscripten.h

ericoporto commented 1 month ago

Ah, the timer test is either wrong or the implementation is wrong

https://github.com/madebr/SDL/blob/3f1143b4f5c3b0fef67fd32ade2daad780d2f22f/src/timer/SDL_timer.c#L437

Here it can only return either 0 or 1 (SDL_FALSE or SDL_TRUE), it will never return something smaller than 0 like it's expected in the test.

I also see the test uses a SDL_Delay to wait while the callback triggers... I think this will only work if both SDL and the test are built with Asyncify. Otherwise you will be locked in this loop.

ericoporto commented 1 month ago

Finally the video position test, you can check the video position sources here

https://github.com/madebr/SDL/blob/main/src/video/emscripten/SDL_emscriptenvideo.c

As far as I can tell the x and y are always set to zero in these source files, so I don't see how the test could move the canvas position. I may be wrong here because this code plugs in other SDL source codes that I am having a bit of a trouble following... But I think the window is meant to be the canvas in here.

ericoporto commented 1 month ago

cc @Daft-Freak and @sbc100 for ideas

sbc100 commented 1 month ago

Why not just exclude the currently failing tests and submit #9937 as a starting point? Then you/we can followup by opening bugs, fixing tests and enabling the remaining ones at a later date.

sbc100 commented 1 month ago

Great work on #9937 by the way! We've been thinking about moving towards a system like that downstream in emscripten too. If we ever get around it we may well use this work as inspiration.