Several tests segfault if wl_shell is absent

[elinorbgr]

The frame_submission test unconditionally tries to use wl_shell even if it is not advertized by the server, and not listed in the supported extensions of the plugin. In that case it segfaults on this line because client.shell() returns a null pointer that is used without checking.

https://github.com/MirServer/wlcs/blob/71ab6819d7be76f82761142a0d07622de67c9c93/tests/frame_submission.cpp#L69

Similarly create_visible_surface also assumes wl_shell is present, and thus all tests using it segfault if it is not:

https://github.com/MirServer/wlcs/blob/71ab6819d7be76f82761142a0d07622de67c9c93/src/in_process_server.cpp#L1636-L1639

Given some compositors have dropped wl_shell support as it has been deprecated for a long time, it might be a good idea to not have the WLCS test suite rely on its presence?

[AlanGriffiths]

@vberger what you suggest makes sense. Any chance of a PR to make it so?

[elinorbgr]

Well, I personally don't have any familiarity with GTest nor the internals of WLCS (and this is the same for the other people at Smithay), so while we can try and give it a shot, some guidance about how to approach this properly would be very helpful.

[AlanGriffiths]

I've now had a look over the code and, unfortunately, it looks like simply reworking the default helper functions (like window_under_cursor()) used by many tests to use xdg-shell would be pervasive. They are an unfortunate legacy.

To ignore tests your compositor doesn't pass you can, for example, use --gtest_filter=-FrameSubmission*.

My feeling is that there should be versions of FrameSubmission and similar for each of the tested extensions and that for unsupported extensions these tests should be "expected fail" (and not, as you rightly say, segfault).

There are helper functions such as create_xdg_shell_stable_surface() that would help with a piecemeal approach of reworking individual tests. That should make it possible to add (and pass) XdgShellFrameSubmission while ignoring FrameSubmission, the protocols are similar enough that copy-paste-edit ought to be a viable approach.

Maybe @RAOF will have better ideas when he comes online.

(I also think that servers should support protocols while they are still deprecated as clients can be slow to migrate, but that's a separate discussion.)

[Drakulix]

I've now had a look over the code and, unfortunately, it looks like simply reworking the default helper functions (like window_under_cursor()) used by many tests to use xdg-shell would be pervasive. They are an unfortunate legacy.

Is this something, you would consider a "bug" of sorts?

E.g. create_visible_surface sounds like a more generic version of create_wl_shell_surface. So any test using create_visible_surface, but then relying on the surface being a wl_shell-surface, should just use create_wl_shell_surface, no?

This way create_shell_surface could be used in cases, where the type of shell does not matter. E.g. the following filter */SurfacePointerMotionTest* works nicely, when I replace the call inside create_visible_surface with an xdg-shell one.

So for more generic tests, the create_visible_surface logic could be extended to return a surface for any supported shell.

I would assume similar abstractions could be build for other functions, but I don't know wlcs well enough.

If there is interest in this approach, I could begin with a fixing up tests to use specific helper functions, where necessary, and then extend the create_visible_surface function (hopefully without breaking anything).

To ignore tests your compositor doesn't pass you can, for example, use --gtest_filter=-FrameSubmission*.

Currently we only run a very limited set of tests with smithay already, so we could continue to fixup more helper functions as we extend the coverage over time. I guess tackling this problem in one go is not a good idea anyway. Instead the segfaults should be fixed and the whole codebase should gradually migrate to use other shells.

My feeling is that there should be versions of FrameSubmission and similar for each of the tested extensions and that for unsupported extensions these tests should be "expected fail" (and not, as you rightly say, segfault).

Where would conceptionally this check happen? I would assume these tests should be excluded if the compositor descriptor does not contain wl_shell, but that is not what appears to be happening. Should the FrameSubmission test do the check itself? I would be happy to make a PR for that.

(I also think that servers should support protocols while they are still deprecated as clients can be slow to migrate, but that's a separate discussion.)

I think there are multiple valid reasons wlcs should support compositors doing this:

• Implementing wlcs support early for new compositors gives access to a lot of tests and any new effort will likely be either incomplete or even not bother with wl_shell any more.
• Similarly smithay as a framework does still support wl_shell, but our example compositor does not any more, because some of our new abstractions don't given the protocol is deprecated. Which means consumers of our library are free to support wl_shell, but we would rather not go the mile of wiring up test support for wlcs, because that increases our maintenance burden.
• There are also other more special cased compositors out there, which don't implement parts of the core protocol. I don't think wlcs should make an attempt to run all tests on these kinds of compositors, but it would be nice, if it didn't crash at least.

So while I agree with your statement, I think there is still a lot of value in supporting this approach as good as reasonably possible (especially for smaller scale compositors).

[AlanGriffiths]

I wasn't suggesting that wlcs shouldn't support this. But, at least for the Mir team, this is isn't going to be urgent.

There is a way for tests results to be automatically suppressed. Typically we call Client::bind_if_supported(): if not supported this throws an ExtensionExpectedlyNotSupported and that sets a "wlcs-skip-test" test property.

The naming issues come from the early days of wlcs (before xdg-shell). We wouldn't now choose the names we had then. (That's what I meant by "legacy").

I don't think anyone would object to a PR with better names.

[wmww]

It appears this was a far easier job than originally thought. Note that window_under_cursor() and similar functions do not, in fact, require wl_shell.

[Drakulix]

Thanks for the quick fix! Much appreciated.