Move CSAPI only tests to tests/csapi (#171)

kegsay · web-flow · commit 86bf4bb10167 · 2021-07-22T12:57:08.000+01:00
* Move CSAPI only tests to tests/csapi

This allows them to run in parallel with federation tests.

* go test ./...

* Update github actions

* Track number of cpu cores on GH actions

* Linting

* Force 2 parallel executions

* No more lscpu
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -42,6 +42,6 @@ jobs:
     steps:
       - uses: actions/checkout@v2
       - run: docker build -t homeserver -f dockerfiles/${{ matrix.homeserver }}.Dockerfile dockerfiles/
-      - run: go test -v -tags "${{ matrix.tags }}" ./tests
+      - run: go test -p 2 -v -tags "${{ matrix.tags }}" ./tests/...
         env:
           COMPLEMENT_BASE_IMAGE: homeserver
diff --git a/ONBOARDING.md b/ONBOARDING.md
@@ -103,6 +103,13 @@ Adding `// sytest: ...` means `sytest_coverage.go` will know the test is convert
 when run! Use `go run sytest_coverage.go -v` to see the exact string to use, as they may be different to the one produced
 by an actual sytest run due to parameterised tests.
 
+### Where should I put new tests?
+
+If the test *only* has CS API calls, then put it in `/tests/csapi`. If the test involves both CS API and Federation, or just Federation, put it in `/tests`.
+This is because of how parallelisation works currently. All federation tests MUST be in the same directory due to the use of shared resources (for example,
+the local Complement server always binds to `:8448` which is a problem if 2 fed tests want to do that at the same time). This will be resolved in the future
+by the use of `.well-known` but at present this is how things stand.
+
 ### Should I always make a new blueprint for a test?
 
 Probably not. Blueprints are costly, and they should only be made if there is a strong case for plenty of reuse among tests. In the same way that we don't always add fixtures to sytest, we should be sparing with adding blueprints.
@@ -140,6 +147,9 @@ t.Run("parallel", func(t *testing.T) {
 })
 ```
 
+Tests in a directory will run in parallel with tests in other directories by default. You can disable this by invoking `go test -p 1` which will
+force a parallelisation factor of 1 (no parallelisation).
+
 ### How should I do comments in the test?
 
 Add long prose to the start of the function to outline what it is you're testing (and why if it is unclear). For example:
@@ -177,10 +187,6 @@ Use one of `t.Skipf(...)` or `t.SkipNow()`.
 
 Error will fail the test but continue execution, where Fatal will fail the test and quit. Use Fatal when continuing to run the test will result in programming errors (e.g nil exceptions).
 
-### Why do I get the error "Error response from daemon: Conflict. The container name "/complement_rooms_state_alice.hs1_1" is already in use by container "c2d1d90c6cff7b7de2678b56c702bd1ff76ca72b930e8f2ca32eef3f2514ff3b". You have to remove (or rename) that container to be able to reuse that name."?
-
-The Docker daemon has a lag time between removing containers and them actually being removed. This means you cannot remove a container called 'foo' and immediately recreate it as 'foo'. To get around this, you need to use a different name. This probably means the namespace you have given the deployment is used by another test. Try changing it to something else e.g `Deploy(t, "rooms_state_2", b.BlueprintAlice.Name)`
-
 ### How do I run tests inside my IDE?
 
 For VSCode, add to `settings.json`:
diff --git a/README.md b/README.md
@@ -10,7 +10,7 @@ Complement is a black box integration testing framework for Matrix homeservers.
 You need to have Go and Docker installed, as well as `libolm3` and `libolm-dev`. Then:
 
 ```
-$ COMPLEMENT_BASE_IMAGE=some-matrix/homeserver-impl COMPLEMENT_BASE_IMAGE_ARGS='-foo bar -baz 1' go test -v ./tests
+$ COMPLEMENT_BASE_IMAGE=some-matrix/homeserver-impl COMPLEMENT_BASE_IMAGE_ARGS='-foo bar -baz 1' go test -v ./tests/...
 ```
 
 You can install `libolm3` on Debian using something like:
@@ -33,7 +33,7 @@ You can either use your own image, or one of the ones supplied in the [dockerfil
 A full list of config options can be found [in the config file](./internal/config/config.go). All normal Go test config
 options will work, so to just run 1 named test and include a timeout for the test run:
 ```
-$ COMPLEMENT_BASE_IMAGE=complement-dendrite:latest go test -timeout 30s -run '^(TestOutboundFederationSend)$' -v ./tests
+$ COMPLEMENT_BASE_IMAGE=complement-dendrite:latest go test -timeout 30s -run '^(TestOutboundFederationSend)$' -v ./tests/...
 ```
 
 ### Running against Dendrite
@@ -43,7 +43,7 @@ For instance, for Dendrite:
 # build a docker image for Dendrite...
 $ (cd dockerfiles && docker build -t complement-dendrite -f Dendrite.Dockerfile .)
 # ...and test it
-$ COMPLEMENT_BASE_IMAGE=complement-dendrite:latest go test -v ./tests
+$ COMPLEMENT_BASE_IMAGE=complement-dendrite:latest go test -v ./tests/...
 ```
 
 ### Running against Synapse
@@ -61,7 +61,7 @@ To run Complement against a specific release of Synapse, set the
 
 ```sh
 docker build -t complement-synapse:v1.36.0 -f dockerfiles/Synapse.Dockerfile --build-arg=SYNAPSE_VERSION=v1.36.0 dockerfiles
-COMPLEMENT_BASE_IMAGE=complement-synapse:v1.36.0 go test ./tests
+COMPLEMENT_BASE_IMAGE=complement-synapse:v1.36.0 go test ./tests/...
 ```
 
 ### Image requirements
@@ -95,7 +95,7 @@ being picked up by `go test`. For example, `apidoc_presence_test.go` has:
 ```
 and all Dendrite tests run with `-tags="dendrite_blacklist"` to cause this file to be skipped. You can run tests with build tags like this:
 ```
-COMPLEMENT_BASE_IMAGE=complement-synapse:latest go test -v -tags="synapse_blacklist,msc2403" ./tests
+COMPLEMENT_BASE_IMAGE=complement-synapse:latest go test -v -tags="synapse_blacklist,msc2403" ./tests/...
 ```
 This runs Complement with a Synapse HS and ignores tests which Synapse doesn't implement, and includes tests for MSC2403.
 
diff --git a/tests/csapi/account_change_password_pushers_test.go b/tests/csapi/account_change_password_pushers_test.go
@@ -1,6 +1,6 @@
 // +build !dendrite_blacklist
 
-package tests
+package csapi_tests
 
 import (
 	"testing"
diff --git a/tests/csapi/account_change_password_test.go b/tests/csapi/account_change_password_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"io/ioutil"
diff --git a/tests/csapi/account_deactivate_test.go b/tests/csapi/account_deactivate_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"net/http"
diff --git a/tests/csapi/apidoc_device_management_test.go b/tests/csapi/apidoc_device_management_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"testing"
diff --git a/tests/csapi/apidoc_login_test.go b/tests/csapi/apidoc_login_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"encoding/json"
diff --git a/tests/csapi/apidoc_presence_test.go b/tests/csapi/apidoc_presence_test.go
@@ -2,7 +2,7 @@
 
 // Rationale for being included in Dendrite's blacklist: https://github.com/matrix-org/complement/pull/104#discussion_r617646624
 
-package tests
+package csapi_tests
 
 import (
 	"testing"
diff --git a/tests/csapi/apidoc_profile_avatar_url_test.go b/tests/csapi/apidoc_profile_avatar_url_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"testing"
diff --git a/tests/csapi/apidoc_profile_displayname_test.go b/tests/csapi/apidoc_profile_displayname_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"testing"
diff --git a/tests/csapi/apidoc_register_test.go b/tests/csapi/apidoc_register_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"encoding/json"
diff --git a/tests/csapi/apidoc_request_encoding_test.go b/tests/csapi/apidoc_request_encoding_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"testing"
diff --git a/tests/csapi/apidoc_room_create_test.go b/tests/csapi/apidoc_room_create_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"testing"
diff --git a/tests/csapi/apidoc_room_state_test.go b/tests/csapi/apidoc_room_state_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"net/url"
diff --git a/tests/csapi/apidoc_version_test.go b/tests/csapi/apidoc_version_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"fmt"
diff --git a/tests/csapi/main_test.go b/tests/csapi/main_test.go
@@ -0,0 +1,129 @@
+package csapi_tests
+
+import (
+	"context"
+	"fmt"
+	"log"
+	"os"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/sirupsen/logrus"
+
+	"github.com/matrix-org/complement/internal/b"
+	"github.com/matrix-org/complement/internal/config"
+	"github.com/matrix-org/complement/internal/docker"
+	"github.com/matrix-org/complement/internal/federation"
+)
+
+var namespaceCounter uint64
+
+// persist the complement builder which is set when the tests start via TestMain
+var complementBuilder *docker.Builder
+
+// TestMain is the main entry point for Complement.
+//
+// It will clean up any old containers/images/networks from the previous run, then run the tests, then clean up
+// again. No blueprints are made at this point as they are lazily made on demand.
+func TestMain(m *testing.M) {
+	cfg := config.NewConfigFromEnvVars()
+	cfg.PackageNamespace = "csapi"
+	log.Printf("config: %+v", cfg)
+	builder, err := docker.NewBuilder(cfg)
+	if err != nil {
+		fmt.Printf("Error: %s", err)
+		os.Exit(1)
+	}
+	complementBuilder = builder
+	// remove any old images/containers/networks in case we died horribly before
+	builder.Cleanup()
+
+	if os.Getenv("COMPLEMENT_CA") == "true" {
+		log.Printf("Running with Complement CA")
+		// make sure CA certs are generated
+		_, _, err = federation.GetOrCreateCaCert()
+		if err != nil {
+			fmt.Printf("Error: %s", err)
+			os.Exit(1)
+		}
+	}
+
+	// we use GMSL which uses logrus by default. We don't want those logs in our test output unless they are Serious.
+	logrus.SetLevel(logrus.ErrorLevel)
+
+	exitCode := m.Run()
+	builder.Cleanup()
+	os.Exit(exitCode)
+}
+
+// Deploy will deploy the given blueprint or terminate the test.
+// It will construct the blueprint if it doesn't already exist in the docker image cache.
+// This function is the main setup function for all tests as it provides a deployment with
+// which tests can interact with.
+func Deploy(t *testing.T, blueprint b.Blueprint) *docker.Deployment {
+	t.Helper()
+	timeStartBlueprint := time.Now()
+	if complementBuilder == nil {
+		t.Fatalf("complementBuilder not set, did you forget to call TestMain?")
+	}
+	if err := complementBuilder.ConstructBlueprintsIfNotExist([]b.Blueprint{blueprint}); err != nil {
+		t.Fatalf("Deploy: Failed to construct blueprint: %s", err)
+	}
+	namespace := fmt.Sprintf("%d", atomic.AddUint64(&namespaceCounter, 1))
+	d, err := docker.NewDeployer(namespace, complementBuilder.Config)
+	if err != nil {
+		t.Fatalf("Deploy: NewDeployer returned error %s", err)
+	}
+	timeStartDeploy := time.Now()
+	dep, err := d.Deploy(context.Background(), blueprint.Name)
+	if err != nil {
+		t.Fatalf("Deploy: Deploy returned error %s", err)
+	}
+	t.Logf("Deploy times: %v blueprints, %v containers", timeStartDeploy.Sub(timeStartBlueprint), time.Since(timeStartDeploy))
+	return dep
+}
+
+// nolint:unused
+type Waiter struct {
+	mu     sync.Mutex
+	ch     chan bool
+	closed bool
+}
+
+// NewWaiter returns a generic struct which can be waited on until `Waiter.Finish` is called.
+// A Waiter is similar to a `sync.WaitGroup` of size 1, but without the ability to underflow and
+// with built-in timeouts.
+// nolint:unused
+func NewWaiter() *Waiter {
+	return &Waiter{
+		ch: make(chan bool),
+		mu: sync.Mutex{},
+	}
+}
+
+// Wait blocks until Finish() is called or until the timeout is reached.
+// If the timeout is reached, the test is failed.
+func (w *Waiter) Wait(t *testing.T, timeout time.Duration) {
+	t.Helper()
+	select {
+	case <-w.ch:
+		return
+	case <-time.After(timeout):
+		t.Fatalf("Wait: timed out after %f seconds.", timeout.Seconds())
+	}
+}
+
+// Finish will cause all goroutines waiting via Wait to stop waiting and return.
+// Once this function has been called, subsequent calls to Wait will return immediately.
+// To begin waiting again, make a new Waiter.
+func (w *Waiter) Finish() {
+	w.mu.Lock()
+	defer w.mu.Unlock()
+	if w.closed {
+		return
+	}
+	w.closed = true
+	close(w.ch)
+}
diff --git a/tests/csapi/rooms_state_test.go b/tests/csapi/rooms_state_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"testing"
diff --git a/tests/csapi/sync_filter_test.go b/tests/csapi/sync_filter_test.go
@@ -1,4 +1,4 @@
-package tests
+package csapi_tests
 
 import (
 	"encoding/json"
diff --git a/tests/csapi/user_query_keys_test.go b/tests/csapi/user_query_keys_test.go
@@ -2,7 +2,7 @@
 
 // Rationale for being included in Synapse's blacklist: https://github.com/matrix-org/synapse/issues/10354
 
-package tests
+package csapi_tests
 
 import (
 	"testing"

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-package tests`
	`1`	`+package csapi_tests`
`2`	`2`
`3`	`3`	`import (`
`4`	`4`	`"io/ioutil"`