Platform-specific skipped tests

[thislooksfun]

It seemed silly to me that we were skipping tests everywhere just because they were behaving badly on one platform, so now we have the option to only skip tests on certain platforms if we so wish!

By default tests are skipped on all platforms, but you can now provide a (lower-case comma-separated) list of which platforms the test should be skipped on. The available options mirror the AK_OS_

I restricted the Worker tests to macOS since that is why they were added in the first place, but I did not go through and audit the rest of them. I did also add Text/input/HTML/DedicatedWorkerGlobalScope-instanceof.html to the list of skipped tests on macOS, since it is consistently hanging on my machine. See #1306 for details.

[ADKaster]

First thought:

What about a Skipped_<platform> section in the ini file instead of making weird looking relative-path=list,of,strings key-value pairs?

[ADKaster]

If we did that the logic would be like

for platform in my_platform_list:
   if group = config.group(`Skipped_${platform}`):
       for key in group:
          list_of_skipped_tests.append(key)

Which seems a bit simpler than what you have now

[thislooksfun]

I did play around with that approach first, but I ran into two issues pretty quickly:

Firstly, the "Unknown group" warning would either have to be a lot more complicated (Skipped_linux is a valid key even if you're running on macOS), or will need to be dropped entirely. I'm not sure how much value it's really adding though, so I think that would be ok.

The bigger issue though is that if you ever wanted to skip a test on two platforms it would be much more verbose than it is now. Skipping on both (and only) macOS and iOS would need to look like:

[Skipped_macos]
Some/Test/Path.html

[Skipped_ios]
Some/Test/Path.html

We could add in even more helpers for these cases (i.e. something like Skipped_apple), but it's still not a great solution.

That being said I'm also not happy with the solution I landed on either, if only because it makes my syntax highlighting go weird. If the config format were more structured (i.e. YAML/JSON) I'd probably do something like:

skipped:
  - Some/Test/Skipped/Everywhere
  - path: Some/Conditionally/Skipped/Test
    platforms: [macos, ios]

But that was hard to replicate in the Serenity Config format.

If you think the duplication concerns and lack of warning are ok then I'm happy to do that approach.

[ADKaster]

:man_shrugging: ini is a very restrictive format. if we want more interesting things to happen, loading a JSON file with AK::JsonValue::from_string wouldn't be the worst thing in the world. Lemme poke the other maintainers to leave feedback here or in #testing.

[alimpfard]

You underestimate the power of ini! It's possible to make this work:

[Skip:macos,ios]
Foo/bar

[Skip:macos]
Bar/baz

(The group names don't have to be \w+ you know...)

[sideshowbarker]

If we’re going to do this, a different approach to consider is something like the way the WebKit project handles it — which is to organize skip/expected-to-fail/expected-to-be-flaky data into TestExpectations files into platform-specific directories.

See https://github.com/WebKit/WebKit/tree/main/LayoutTests/platform and for a specific example, see https://github.com/WebKit/WebKit/blob/main/LayoutTests/platform/mac/TestExpectations

I’m not voting for that as being an absolutely-better way of handling it — but instead just offering it up as something worth taking a look at that was designed to address the same problems.

[nico]

Another idea (which I'm also not voting for but just offering) is to keep the metadata for each test in that test, in a magic comment near the top of the file or something. Maybe:

<!--
SKIP: mac, win
-->

[thislooksfun]

You underestimate the power of ini! [snip]

True! But that also makes the pattern matching part of the skip logic slightly more complicated. Not impossibly so by any means, but the key/value approach was less work, which is why I went with that. This does look cleaner though, so I'm tempted to give it another shot.

If we’re going to do this, a different approach to consider is something like the way the WebKit project handles it [snip]

Definitely an interesting idea to keep in mind, but I personally am not a fan of having bespoke file formats for things like this, and it seems overkill for what we need at the moment, especially since afaik Ladybird's internal tests are currently strictly pass/fail. Something to keep in mind though, especially with its ability to "un-skip" tests per-platform, which has the same effect as saying "skip except on <platform>", which could be useful.

Another idea (which I'm also not voting for but just offering) is to keep the metadata for each test in that test, in a magic comment near the top of the file or something. [snip]

Ooh, I both really do and really don't like this idea. On the one hand it's simple and keeps the skips tightly coupled to the tests, but on the other hand it makes it a little harder to find all the skipped tests (though since we list all the skipped tests at the end of the run it's not that big of a deal, especially if we have a consistent and grepable prefix). It would also entirely prevent having any sort of pattern-matched skips since you would have to add them to each file individually. Maybe some CLI tooling could help with that, but that feels like a pain point waiting to happen.

The more I think about that last option (the inline comments one) the more I like it actually. The only real pain point is the inherent lack of globbing support. Maybe we could do a hybrid approach? Have per-file skips as described then also have a directory-scoped file (i.e. Tests/LibWeb/platform/macos/Skips.txt [or some other format]) that applies to the whole directory? It still wouldn't allow for skipping some-but-not-all tests in a directory, but that could push us towards better organizing the tests' directory structure anyway, which might not be a bad thing.

At the end of the day though, I'm not the one who will be interacting with this system the most, so I'm happy to go with whatever the group consensus is.

[thislooksfun]

On that vein, I'm going to hold off on addressing the review comments until we settle on an approach.

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

[github-actions[bot]]

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!