Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

There is no way to extract deployment-created room IDs after the fact #254

Open
ShadowJonathan opened this issue Dec 2, 2021 · 9 comments

Comments

@ShadowJonathan
Copy link
Contributor

This is something i found while looing at BlueprintOneToOneRoom, and seeing where its one-on-one room has been (re)used in any tests.

Turns out, it doesnt, every test creates its own additional rooms, which makes pre-setting or otherwise prefilling a room with events feel a bit moot.

I think it would help if there's some way to extract "execution results" (IDs) from a Deployment object after it has been finalised, to then act upon and work with the deployed template, this would make templates a bit more useful.

@kegsay
Copy link
Member

kegsay commented Dec 9, 2021

100% agreed, I just haven't thought of a good way to do this yet. It's a bit of a shame that blueprints are mostly just used to create servers / users only at present.

@ShadowJonathan
Copy link
Contributor Author

Maybe we can alter blueprint in general to assign those generated variables to "known names", so that afterwards we can simply do fetch_room("alias"), fetch_user("alias"), or fetch_event("alias") on the deployment objects themselves.

@richvdh
Copy link
Member

richvdh commented Feb 11, 2022

I just haven't thought of a good way to do this yet

Looks like we have a mechanism for saving the access tokens we create for users - could we do something for saving room ids?

@kegsay
Copy link
Member

kegsay commented Feb 11, 2022

We do - this is done via labels on the docker image. The problem comes in the API. It's easy to say map user_id -> access_token, but for rooms it will be... ? -> room_id. It could just be the index offset on the blueprint itself (i.e index position in https://github.com/matrix-org/complement/blob/master/internal/b/federation_one_to_one_room.go#L16 ) but then how do you present this to tests in a way that makes sense and is clear and obvious? That's the thing I haven't got a good way yet.

Whilst it is slower to have to create rooms in each test, it is undoubtedly clearer when you can see the creation content, the creator, etc up front, and that has intrinsic value. For example, it was via this mechanism that we accidentally found out that Dendrite and Synapse disagree on what the preset is if it is missing. Tests which failed to set this behaved differently on synapse vs dendrite.

We could maybe cache the (creator, creation content) and if the client calls CreateRoom and the blueprint has a literal match, then return that, but then it's unclear that this room was not newly created (important for say device list changes) and potentially has other users in it already (typically Bob).

@richvdh
Copy link
Member

richvdh commented Feb 14, 2022

Whilst it is slower to have to create rooms in each test, it is undoubtedly clearer when you can see the creation content, the creator, etc up front, and that has intrinsic value.

Well, I don't disagree with that, but if that's the path you'd prefer to go down, shall we just clear all the room-creation out of the blueprints? It feels very odd that we create them and then never use them.

@richvdh
Copy link
Member

richvdh commented Feb 16, 2022

It could just be the index offset on the blueprint itself (i.e index position in https://github.com/matrix-org/complement/blob/master/internal/b/federation_one_to_one_room.go#L16 ) but then how do you present this to tests in a way that makes sense and is clear and obvious? That's the thing I haven't got a good way yet.

Why not use the Ref field as a key in a map? (ie, in the above example, the key would be alice_room).

@ShadowJonathan
Copy link
Contributor Author

And then have something like Deployment.fetch_room("alice_room") to fetch the room ID?

@kegsay
Copy link
Member

kegsay commented Feb 17, 2022

shall we just clear all the room-creation out of the blueprints?

Room functionality is used with Homerunner and the whole account snapshot stuff, so we cannot just remove them entirely.

Why not use the Ref field as a key in a map? (ie, in the above example, the key would be alice_room).

This doesn't help make it clear what the room is configured with. roomID := Deployment.fetch_room("alice_room") is an option, but clarity will suffer (is it public or private, who else is in the room, is it encrypted, etc).

Removing room functionality in Complement tests is something I'm considering, mostly because it cannot provide a clear API surface when you read tests. However, at that point one can question the value of blueprints in Complement: it's only really useful to spin up multiple homeservers and a few users. Blueprints were originally conceived as a way to speed up tests by providing a clear, well-known snapshot to base your tests on. It was hard to know back then which snapshots would be useful as we lacked many tests. I think there is value in going back through the existing tests to look for commonality which could benefit from being made into a blueprint. If no such commonality exists, then really rooms support should be removed.

@kegsay
Copy link
Member

kegsay commented Feb 18, 2022

Alternatively we could allows snapshots in tests. This would allow people to roll back to earlier states to test things. This is similar to blueprints but instead of having them be static structs they are formed by earlier commands in the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants