Test that invites from ignored users are omitted from incremental syncs by DMRobertson · Pull Request #267 · matrix-org/complement

DMRobertson · 2021-12-03T18:51:28Z

Leaving this as a draft until I fix the issue in Synapse. Sticking up a PR for now to see what Dendrite does.

Also note: fd3a555 is cherry-picked from #237

DMRobertson · 2021-12-03T19:01:18Z

Fails on Dendrite:

 PUT hs1/_matrix/client/v3/user/@alice:hs1/account_data/m.ignored_user_list => 404 Not Found (722.71µs)

Guessing this isn't implemented in Dendrite yet: if so, suggest blacklisting this test.

ShadowJonathan · 2021-12-04T18:34:37Z

You can do that yourself by adding runtime.SkipIf(t, runtime.Dendrite) to the start of the test, fwiw

Maybe this doesn't need a helper though?

DMRobertson · 2021-12-06T13:43:19Z

Guessing this isn't implemented in Dendrite yet: if so, suggest blacklisting this test.

matrix-org/dendrite#600

DMRobertson · 2021-12-06T13:52:40Z

You can do that yourself by adding runtime.SkipIf(t, runtime.Dendrite) to the start of the test, fwiw

My understanding is that build constraints are the done thing here, as in 0b453d9

ShadowJonathan · 2021-12-06T14:07:57Z

Maybe, but more recently, skipping specific tests were introduced, these are triggered on *_blacklist in the same way.

clokep · 2021-12-07T16:17:05Z

tests/csapi/ignored_users_test.go

+// Synapse does not have this property, as detailed in
+// https://github.com/matrix-org/synapse/issues/11506.
+// This reproduces that bug.


Maybe?

Suggested change

// Synapse does not have this property, as detailed in

// https://github.com/matrix-org/synapse/issues/11506.

// This reproduces that bug.

// Synapse does not handle this properly, as detailed in

// https://github.com/matrix-org/synapse/issues/11506.

// This reproduces that bug.

I don't think this is true though since this is now fixed, it might be better to say:

This is a regression test for link to issue.

clokep · 2021-12-07T16:19:10Z

tests/csapi/ignored_users_test.go

+// https://github.com/matrix-org/synapse/issues/11506.
+// This reproduces that bug.
+func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) {
+	deployment := Deploy(t, b.BlueprintCleanHS)


This test could be much simpler, I think, by starting with BlueprintOneToOneRoom? I'm not sure the third user is very helpful here.

The third user was an attempt to stop this test from being racey. We want to be convinced that the server has processed Bob's invite and chosen to ignore it. I could assert that there's no invite from Bob in Alice's next sync, but... what if that's only true because Alice synced really quickly?

In #237 (comment) suggested sending and awaiting a dummy event. The third user was my attempt at doing something similar here.

I think you could do that by sending a dummy user into the room, yes.

Oh, would Alice expect to see state updates for a room she's invited to, but hasn't joined yet? If so that would be much clearer.

Oh, would Alice expect to see state updates for a room she's invited to, but hasn't joined yet? If so that would be much clearer.

No, they would not see those. I think I'm confused though -- isn't this just checking that some sort of events got processed by any room? (I.e. sync is working at all?)

clokep · 2021-12-07T16:19:48Z

I requested review from @kegsay since I believe he has thoughts about the generic infrastructure of complement.

kegsay

For now, this is fine and I'm happy for it to be merged.

For later, this is not fine. I've filed #271 and will retrospectively fix up this PR to use the new API when it lands.

kegsay · 2021-12-09T10:24:34Z

tests/csapi/ignored_users_test.go

+	chris := deployment.RegisterUser(t, "hs1", "chris", "sufficiently_long_password_chris")
+
+	// Alice creates a room for herself.
+	public_room := alice.CreateRoom(t, map[string]interface{}{


Please do not use underscores for variable names.

Sorry, old habits. Is there some way to configure gofmt/goimports or VSCode to flag this up?

kegsay · 2021-12-09T10:25:48Z

tests/csapi/ignored_users_test.go

+	alice.MustDoFunc(
+		t,
+		"PUT",
+		[]string{"_matrix", "client", "v3", "user", alice.UserID, "account_data", "m.ignored_user_list"},


Dendrite doesn't implement any /v3/ endpoint. Is there some spec documentation on the changes between r0 and v3?

Dendrite doesn't implement any /v3/ endpoint. Is there some spec documentation on the changes between r0 and v3?

All r0 endpoints became v3 with Matrix 1.1 -- @DMRobertson Maybe we should just use r0 for now here?

Happy to use r0; I was just blindly going by what the latest spec said.

maybe dendrite does support account data under r0?

DMRobertson · 2021-12-09T15:00:36Z

Using r0 means that we PUT the account data, but the ignored invite isn't still ignored. I'd forgotten that matrix-org/dendrite#600 was a thing.

This reverts commit 1086814.

DMRobertson and others added 4 commits November 29, 2021 16:38

Change SyncUntil* functions to return next_batch

fd3a555

Add a matcher which checks that a key is missing

301f2e9

Reproduce matrix-org/synapse#11506

910528e

goimports

5963201

David Robertson added 3 commits December 6, 2021 13:26

SyncUntilGlobalAccountDataHas

908f4d1

Maybe this doesn't need a helper though?

Explicitly wait for the ignore to be synced

3373df1

Fix format of the ignore request

a347f1e

DMRobertson mentioned this pull request Dec 6, 2021

Correctly ignore invites from ignored users matrix-org/synapse#11511

Merged

Goimports

f38ae64

Blocklist this test on Dendrite

0b453d9

DMRobertson force-pushed the dmr/ignored-users-invites branch from b2ea172 to 0b453d9 Compare December 6, 2021 13:44

DMRobertson marked this pull request as ready for review December 6, 2021 13:44

DMRobertson changed the title ~~Reproduce matrix-org/synapse#11506~~ Test that invites from ignored users are omitted from incremental syncs Dec 6, 2021

DMRobertson requested a review from a team December 6, 2021 13:50

clokep reviewed Dec 7, 2021

View reviewed changes

clokep requested a review from kegsay December 7, 2021 16:19

kegsay mentioned this pull request Dec 9, 2021

Revamp the SyncUntil API #271

Closed

kegsay approved these changes Dec 9, 2021

View reviewed changes

David Robertson added 3 commits December 9, 2021 14:35

Use lowerCamelCase for variables

dfc81b7

Use r0 instead of v3

512c226

Try without dendrite blacklist

1086814

maybe dendrite does support account data under r0?

Revert "Try without dendrite blacklist"

7f6ec25

This reverts commit 1086814.

Describe this as a regression test

8b434d7

DMRobertson requested a review from clokep December 9, 2021 15:36

clokep approved these changes Dec 9, 2021

View reviewed changes

DMRobertson merged commit e7f6921 into master Dec 9, 2021

Conversation

DMRobertson commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DMRobertson commented Dec 3, 2021

Uh oh!

ShadowJonathan commented Dec 4, 2021

Uh oh!

DMRobertson commented Dec 6, 2021

Uh oh!

DMRobertson commented Dec 6, 2021

Uh oh!

ShadowJonathan commented Dec 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clokep Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clokep commented Dec 7, 2021

Uh oh!

kegsay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DMRobertson commented Dec 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DMRobertson commented Dec 3, 2021 •

edited

Loading

clokep Dec 7, 2021 •

edited

Loading