matrix-org / matrix-org/complement

Refactor how skipped tests are defined

Open
#654 7 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
99
Forks
72
Avg merge
4d 1h
Merged PRs (30d)
8

Description

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
//go:build msc3391
// +build msc3391

package tests

// ... rest of code
```
and used like this:
```bash
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: https://github.com/matrix-org/complement/issues/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
//go:build !dendrite_blacklist
// +build !dendrite_blacklist

package tests

// ... rest of code
```
and used like this:
```bash
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:
```go
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
//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: https://github.com/matrix-org/complement/issues/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?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading the runtime.SkipIf implementation and the existing mscXXXX_test.go organization described in the issue. Trace how build tags select tests and how runtime detects the homeserver, then compare that behavior with the proposed user configuration and t.Skipf-based execution. Done means the design supports discoverable test groups, explicit user selection, deterministic skipping, and removal of runtime-driven blacklist behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.