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

Unskip dendrite skipif on a members presence test #516

Merged
merged 11 commits into from
May 19, 2023

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Oct 16, 2022

As per matrix-org/dendrite#2803, removes the skipif.

A previous iteration of this PR fixed #515, but #563 already addresses it, noting it here to auto-close it once merged.

Signed-off-by: Jonathan de Jong <[email protected]>

and able to be ran in isolation
@ShadowJonathan ShadowJonathan requested review from a team as code owners October 16, 2022 12:05
@ShadowJonathan ShadowJonathan changed the title Make local members presence test more reliable Fix local members presence test Oct 18, 2022
@H-Shay
Copy link
Contributor

H-Shay commented Oct 18, 2022

looks like there are some test failures that need to be fixed?

@ShadowJonathan
Copy link
Contributor Author

Fixed, it was a build issue with imports

S7evinK added a commit to matrix-org/dendrite that referenced this pull request Oct 19, 2022
This makes the following changes:
- get state deltas without the user supplied filter, so we can actually
"calculate" state transitions
- closes `stmt` when using SQLite
- Adds presence for users who newly joined a room, even if the syncing
user already knows about the presence status (should fix
matrix-org/complement#516)
@S7evinK S7evinK reopened this Oct 19, 2022
Copy link
Contributor

@S7evinK S7evinK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor things. :)
Otherwise looks good.

tests/csapi/rooms_members_local_test.go Outdated Show resolved Hide resolved
tests/csapi/rooms_members_local_test.go Outdated Show resolved Hide resolved
@erikjohnston erikjohnston removed the request for review from a team October 27, 2022 09:02
# Conflicts:
#	tests/csapi/rooms_members_local_test.go
@ShadowJonathan
Copy link
Contributor Author

ShadowJonathan commented Nov 5, 2022

I'm going to re-add the skipif, to unblock this PR, and if matrix-org/dendrite#2803 is fixed, i'll remove the skipif in another PR.

@S7evinK
Copy link
Contributor

S7evinK commented Nov 5, 2022

It should be working, once matrix-org/dendrite#2854 is merged

# Conflicts:
#	tests/csapi/rooms_members_local_test.go
# Conflicts:
#	tests/csapi/rooms_members_local_test.go
@ShadowJonathan
Copy link
Contributor Author

It seems #563 was made before this PR has had any chance to progress.

Seeing as how the changes introduced there are antithetical to the actual tests being done (i.e. bob joining a room, and alice seeing bob for the first time, but this being subverted with the addition of an extra pre-existing one-on-one room), I will be reverting that.

@ShadowJonathan ShadowJonathan changed the title Fix local members presence test Unskip dendrite skipif on members presence test Dec 10, 2022
@DMRobertson DMRobertson requested review from a team February 6, 2023 18:21
@DMRobertson DMRobertson changed the title Unskip dendrite skipif on members presence test Unskip dendrite skipif on a members presence test Feb 6, 2023
@ShadowJonathan
Copy link
Contributor Author

(CI needs to be re-booped because I only added the signature now, though since it is in code, i'm not sure if it'll go through)

@kegsay
Copy link
Member

kegsay commented May 19, 2023

I can see the sign off despite CI not.

@kegsay
Copy link
Member

kegsay commented May 19, 2023

Rerunning CI then merging.

@kegsay kegsay merged commit 68c7643 into matrix-org:main May 19, 2023
@ShadowJonathan ShadowJonathan deleted the local-members-presence-fix branch May 19, 2023 18:05
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

Successfully merging this pull request may close these issues.

TestMembersLocal -> "member presence" test relies on parallel side-effects
5 participants