matrix-org / complement

Matrix compliance test suite
Apache License 2.0
61 stars 50 forks source link

Support HS upgrade tests #150

Open kegsay opened 3 years ago

kegsay commented 3 years ago

Dendrite has https://github.com/matrix-org/dendrite/tree/master/cmd/dendrite-upgrade-tests which will automatically pull and run different versions of dendrite with the same database and ensure db migrations work correctly. It can be run for example by doing ./dendrite-upgrade-tests --from 0.1.0 --to 0.3.1. This will:

If Complement standardised the format of this (e.g thou shalt accept the build arg HS_VERSION=$semver) then we could, in theory, make https://github.com/matrix-org/dendrite/tree/master/cmd/dendrite-upgrade-tests work for any HS (modulo working out how to do persistence, as currently Dendrite uses postgres and that assumption is baked into the upgrade testing infra: this could just be stating that the directory /data is persisted across runs and you need to just dump your DB there).

This arguably is and isn't feature creep. It is feature creep because it has nothing to do with the integration tests in /tests. It isn't feature creep because it is still ultimately testing HSes, just the upgrade paths.

Thoughts @richvdh (on the basis that you added SYNAPSE_VERSION?

richvdh commented 3 years ago

Firstly: I love the idea of running upgrade tests; I wish we had something like that for Synapse. Do you run it as part of your CI?

To answer the question here: I agree that it does feel a bit feature-creepy; but given that Complement has most of the framework needed to make it happen, it seems like a bit of a shame for every HS impl to have to implement it from scratch.

I think you can say much the same of Homerunner. Generally, having a collection of commands powered by the internals of Complement doesn't feel like a terrible thing to me.

So basically, +1?

kegsay commented 3 years ago

Yeah CI runs the last published semver release and then the pushed commit over the top, as it takes a while to run every single release on CI all the time. This catches thinkos in upgrade scripts but isn't capable of catching everything as sometimes data corruption happens in earlier versions which then affects the next database migration. For example, dendrite 0.1 had corrupted state snapshots which then caused problems when those rooms made on 0.1 were migrated to HEAD. For that, we need to run everything from the beginning, which we'll just do as a pre-release checklist.

I'll go ahead then with this and see what we end up with.

ShadowJonathan commented 2 years ago

Why should this be part of complement?

Complement, in my eyes, about black-box compliance testing, there's nothing that the spec does to address, acknowledge, see, or even know about server upgrades, as that is a completely implementation-specific concept.

What exactly should complement test for? That servers dont crash when they upgrade? How would that exactly be Complement's job to execute? Why can't that just be a standard external tool that's executed separately in a homeserver's CI pipeline instead?

In my opinion, this is heavily feature creep, this should be its standalone tool, or banished to ./cmd/.

It could maybe be something along the lines of (calling it) semver-hs-db-upgrade, which is then fed a list of "significant" homeserver versions (for synapse: the versions where database upgrades happen), and then produce and execute every variation of A->B upgrades between them.