Refactor how skipped tests are defined

[kegsay]

Complement was designed to test the Matrix Specification. However, the spec isn't static and MSCs need tests too. We introduced the build tag system a long time ago to allow tests to opt-in to certain tests. They are defined like this:

//go:build msc3391
// +build msc3391

package tests

// ... rest of code

and used like this:

go test -tags "msc3083 msc3787 msc3874" ./...

The set of tests run is controlled by the user, which can be frustrating when they get out-of-sync: #185

This wasn't the end of it though. We also have unique APIs specific to a homeserver, or sometimes some servers don't implement core features which are enabled by default. To fix this, we (ab)use the same build tag system. To opt-out of a test, they are defined like this:

//go:build !dendrite_blacklist
// +build !dendrite_blacklist

package tests

// ... rest of code

and used like this:

go test -tags "dendrite_blacklist" ./...

This was the status quo for a long time, until there was a need to blacklist some tests in the same file but not others. This introduced the runtime package, where you could skip a test in code, rather than conditional compilation. They are defined like this:

func TestRoomImageRoundtrip(t *testing.T) {
	runtime.SkipIf(t, runtime.Dendrite)
	// .. rest of test
}

A runtime knows what it is by the virtue of the blacklist tag. E.g running -tags "dendrite_blacklist" would skip this test, even if the homeservers were synapse! The code for this is:

//go:build dendrite_blacklist
// +build dendrite_blacklist

package runtime

import (
	"context"
	"time"

	"github.com/docker/docker/client"
)

func init() {
	Homeserver = Dendrite
}

This is clearly at odds with testing between heterogeneous homeservers. It's also been questioned if it is the test's job to say what can and cannot run with it. Surely it would be better if it were more tag-like, and the user can opt-in to these tests?

We also have some other ideas for conditional execution of tests based on /versions output: #549 . Our forebear, sytest, would automatically skip some tests based on if an earlier test which has a special can_ property did not pass. This is problematic if the can_ test is flakey!

From these use cases, there are some properties we want the test execution API to have:

• granular but not too granular: MSC level is about right. Skipping entire files isn't great as the files are mostly arbitrary collections of tests.
• The user should specify which tests to execute. It's a bit of a smell that Complement dictates this currently via runtime.SkipIf, and it means that this repository gets touched way too much just to remove these lines as servers get updated.
• We don't want the set of tests executed to be nondeterminstic (dependent on a server response or a passing/failing previous test) as that is way too unstable and flakey.

A high-level approach here could be:

• One file per group of tests. Similar to mscXXXX_test.go files.
• Make test names part of the public testing API.
• Allow users to specify a set of groups / tests to run / not run.

This would allow us to remove runtime.SkipIf from the codebase, and runtime detection which is flakey and doesn't work with multi-HS support. It also removes the smell of having tests dictate what can/cannot run.

Proposal

• Define a configuration format for specifying the list of groups / test names to run. The list of valid groups/tests must be easily discoverable (autogen from code, like we do with env vars?)
• Read the configuration format and skip appropriately. Always use t.Skipf instead of conditional compilation? This would be beneficial as it is not uncommon for conditionally compiled code to break API wise from core (e.g alter a function signature in core, then patch up tests which whine, the mscXXXX files won't whine due to no build tags set by default in the IDE).
• This implies some kind of test helper/executor that runs before every test, which isn't ideal as it starts to obfuscate what code runs when you hit test. Could just add it as boilerplate though?

[kegsay]

Worth noting that Go supports regexp matching test names, so we could mux the group into the test name itself e.g TestMSC334455_NameOfTest(t *testing.T) but this would be convention and not enforced without extra tooling.

[kegsay]

dmr sez group by package plz.

[kegsay]

So we could change this to:

go test ./tests/csapi ./tests/ssapi ./tests/msc2836 ...

This:

• Lets the caller opt-in to whatever tests they want.
• Doesn't rely on build tags, so refactoring the Complement API shows compilation errors immediately.
• Keeps parallel execution.
• Doesn't change the way you invoke complement tests, so it'll still work in an IDE.

This doesn't address blacklists though, as it isn't possible to use -runto exclude some tests.

Instead of saying "I am dendrite" and the tests then saying "skip this as it doesn't work on dendrite", we could instead add a new env var to control which tests are skipped. If the env var is missing, no tests are skipped (so it'll work in IDEs). If the env var is set, and said env var is a file say, then we could list off the tests to skip. E.g:

# skiptests.txt
TestFoo
TestBar
TestFederationKeyUploadQuery/Can_claim_remote_one_time_key_using_POST

Then in tests, we could add boilerplate to do this check and skip if found:

func TestFoo(t *testing.T) {
    maybeSkip(t)
}

func maybeSkip(t *testing.T) {
    skipFile := os.Getenv("COMPLEMENT_SKIP_TESTS")
    data, _ := io.ReadAll(skipFile)
    if strings.Contains(string(data), t.Name()) {
        t.Skipf("skipping test as it is in COMPLEMENT_SKIP_TESTS")
    }
}

The downside is it isn't clear what test names are always e.g:

func TestFederationKeyUploadQuery(t *testing.T) {
    t.Run("Can claim remote one time key using POST", func(t *testing.T) {
        t.Name() == "TestFederationKeyUploadQuery/Can_claim_remote_one_time_key_using_POST"
    }
}

So you can imagine future frustration if someone wants to skip this test and doesn't know the magic incantation of swapping for _. We could be kind here and add logs with COMPLEMENT_DEBUG=1 such that it logs the test name (and says it isn't skipping because it !=). I don't propose mapping the text lines to automatically escape stuff, as that is more magic.

To avoid needing to call maybeSkip all the time, we could be cheeky and bundle it into Deploy which is what basically all tests immediately do, though subtests are an exception here so maybe not.

[kegsay]

as it isn't possible to use -run to exclude some tests.

Not entirely true, there is -skip 'TestFederatedEventRelationships|TestEventRelationships'. We could then potentially just accept a list of test names, escape them, then slap ORs on them. This would mean we wouldn't need additional boilerplate on every func Test and subtest. It looks like -skip is Go 1.20+ https://go.dev/doc/go1.20#go-command

[kegsay]

Next steps:

• Remove mscXXXX build tags. Move those files to separate packages (directories). Edit locations which previously used -tags to just specify those directories, in addition to ./tests and ./tests/csapi. At this point, any invocations using the old -tags format will fail as they will execute all MSC tests. Remove msc build tags. Add msc packages. #666
• Remove runtime.SkipIf. Replace with manual -skip on go test. Need some way to map a file of test function names to the regexp to apply.
• Remove _blacklist build tags. Any skipped tests now should also go into the skip file. This means new tests added to these files will now need to be manually caught.

We may want to have a test directory up first to make it easier to know/discover which tests to skip?

[kegsay]

MSC work has landed. The remaining bits are to redo how skipped tests are specified in Complement. Renaming issue for clarity.