Skip to content

Do once-over on all SkipIfs in codebase, adding issue numbers, un-skipping passing tests #519

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 27, 2022
Merged

Do once-over on all SkipIfs in codebase, adding issue numbers, un-skipping passing tests #519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5941dd
Do once-over on all SkipIfs in codebase
ShadowJonathan Oct 16, 2022
65ad7c8
Unskip test
ShadowJonathan Oct 26, 2022
c2a29cf
ah yes, imports
ShadowJonathan Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions tests/csapi/apidoc_room_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package csapi_tests

import (
"net/http"
"net/url"
"testing"
"time"
Expand All @@ -11,9 +12,6 @@ import (
"github.com/matrix-org/complement/internal/client"
"github.com/matrix-org/complement/internal/match"
"github.com/matrix-org/complement/internal/must"
"github.com/matrix-org/complement/runtime"

"net/http"
)

func TestRoomState(t *testing.T) {
Expand Down Expand Up @@ -330,7 +328,6 @@ func TestRoomState(t *testing.T) {
})
})
t.Run("GET /rooms/:room_id/joined_members is forbidden after leaving room", func(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // https://github.com/matrix-org/complement/pull/424
t.Parallel()
roomID := authedClient.CreateRoom(t, map[string]interface{}{})
authedClient.LeaveRoom(t, roomID)
Expand Down
2 changes: 1 addition & 1 deletion tests/csapi/device_lists_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ func TestDeviceListUpdates(t *testing.T) {
t.Run("when remote user leaves a room", func(t *testing.T) { testOtherUserLeave(t, deployment, "hs1", "hs2") })
t.Run("when leaving a room with a local user", func(t *testing.T) { testLeave(t, deployment, "hs1", "hs1") })
t.Run("when leaving a room with a remote user", func(t *testing.T) {
runtime.SkipIf(t, runtime.Synapse) // https://github.com/matrix-org/synapse/issues/13650
runtime.SkipIf(t, runtime.Synapse) // FIXME: https://github.com/matrix-org/synapse/issues/13650
testLeave(t, deployment, "hs1", "hs2")
})
t.Run("when local user rejoins a room", func(t *testing.T) { testOtherUserRejoin(t, deployment, "hs1", "hs1") })
Expand Down
2 changes: 1 addition & 1 deletion tests/csapi/invalid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func getFilters() []map[string]interface{} {

// sytest: Check creating invalid filters returns 4xx
func TestFilter(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // TODO remove if https://github.com/matrix-org/dendrite/issues/2067 is fixed
runtime.SkipIf(t, runtime.Dendrite) // FIXME: https://github.com/matrix-org/dendrite/issues/2067

deployment := Deploy(t, b.BlueprintAlice)
defer deployment.Destroy(t)
Expand Down
4 changes: 0 additions & 4 deletions tests/csapi/push_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,10 @@ import (
"github.com/matrix-org/complement/internal/client"
"github.com/matrix-org/complement/internal/match"
"github.com/matrix-org/complement/internal/must"
"github.com/matrix-org/complement/runtime"
)

// @shadowjonathan: do we need this test anymore?
// sytest: Getting push rules doesn't corrupt the cache SYN-390
func TestPushRuleCacheHealth(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // Dendrite does not support push notifications (yet)

deployment := Deploy(t, b.BlueprintAlice)
defer deployment.Destroy(t)

Expand Down
3 changes: 0 additions & 3 deletions tests/csapi/room_relations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ import (
"github.com/matrix-org/complement/internal/client"
"github.com/matrix-org/complement/internal/match"
"github.com/matrix-org/complement/internal/must"
"github.com/matrix-org/complement/runtime"
)

func TestRelations(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // not supported
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be #499?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not spot that, should I remove these changes so they don't conflict?

Copy link
Member

Choose a reason for hiding this comment

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

I guess leave it, they'll conflict and they can fix it there.

deployment := Deploy(t, b.BlueprintAlice)
defer deployment.Destroy(t)

Expand Down Expand Up @@ -112,7 +110,6 @@ func TestRelations(t *testing.T) {
}

func TestRelationsPagination(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // not supported
deployment := Deploy(t, b.BlueprintAlice)
defer deployment.Destroy(t)

Expand Down
2 changes: 1 addition & 1 deletion tests/csapi/rooms_members_local_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestMembersLocal(t *testing.T) {

// sytest: Existing members see new members' presence
t.Run("Existing members see new members' presence", func(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // Still failing
runtime.SkipIf(t, runtime.Dendrite) // FIXME: https://github.com/matrix-org/dendrite/issues/2803
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#516 removes the line entirely, the PR might conflict

The reason it can't be done here is because that PR also introduces changes to fix the test itself

t.Parallel()
alice.MustSyncUntil(t, client.SyncReq{},
client.SyncJoinedTo(bob.UserID, roomID),
Expand Down
8 changes: 4 additions & 4 deletions tests/csapi/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func TestTentativeEventualJoiningAfterRejecting(t *testing.T) {
}

func TestSync(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // too flakey, fails with sync_test.go:135: unchanged room !7ciB69Jg2lCc4Vdf:hs1 should not be in the sync
runtime.SkipIf(t, runtime.Dendrite) // FIXME: https://github.com/matrix-org/dendrite/issues/1324
// sytest: Can sync
deployment := Deploy(t, b.BlueprintOneToOneRoom)
defer deployment.Destroy(t)
Expand Down Expand Up @@ -162,7 +162,7 @@ func TestSync(t *testing.T) {
})
// sytest: Newly joined room has correct timeline in incremental sync
t.Run("Newly joined room has correct timeline in incremental sync", func(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // does not yet pass
runtime.SkipIf(t, runtime.Dendrite) // FIXME: https://github.com/matrix-org/dendrite/issues/1324
t.Parallel()
filter = map[string]interface{}{
"room": map[string]interface{}{
Expand Down Expand Up @@ -211,7 +211,7 @@ func TestSync(t *testing.T) {
})
// sytest: Newly joined room includes presence in incremental sync
t.Run("Newly joined room includes presence in incremental sync", func(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // does not yet pass
runtime.SkipIf(t, runtime.Dendrite) // FIXME: https://github.com/matrix-org/dendrite/issues/1324
roomID := alice.CreateRoom(t, map[string]interface{}{"preset": "public_chat"})
alice.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(alice.UserID, roomID))
_, nextBatch := bob.MustSync(t, client.SyncReq{})
Expand All @@ -231,7 +231,7 @@ func TestSync(t *testing.T) {
})
// sytest: Get presence for newly joined members in incremental sync
t.Run("Get presence for newly joined members in incremental sync", func(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // does not yet pass
runtime.SkipIf(t, runtime.Dendrite) // FIXME: https://github.com/matrix-org/dendrite/issues/1324
roomID := alice.CreateRoom(t, map[string]interface{}{"preset": "public_chat"})
nextBatch := alice.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(alice.UserID, roomID))
sendMessages(t, alice, roomID, "dummy message", 1)
Expand Down
2 changes: 1 addition & 1 deletion tests/csapi/upload_keys_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestUploadKey(t *testing.T) {

// sytest: Rejects invalid device keys
t.Run("Rejects invalid device keys", func(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite, runtime.Synapse) // Dendrite doesn't pass, Synapse has it blacklisted
runtime.SkipIf(t, runtime.Dendrite, runtime.Synapse) // Blacklisted on Synapse, Dendrite FIXME: https://github.com/matrix-org/dendrite/issues/2804
Copy link
Member

Choose a reason for hiding this comment

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

Is there a corresponding Synapse issue?

Copy link
Contributor Author

@ShadowJonathan ShadowJonathan Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There is one in this repo: #517

I looked for one on synapse, but didn't open one there as I wasn't entirely sure what the reasoning was why these tests were blacklisted in the first place, so I opened one here to start that inquiry.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine. 👍 Thanks.

t.Parallel()
// algorithms, keys and signatures are required fields, but missing
reqBody := client.WithJSONBody(t, map[string]interface{}{
Expand Down
2 changes: 1 addition & 1 deletion tests/federation_room_ban_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
// Create a federation room. Bob bans Alice. Bob unbans Alice. Bob invites Alice (unbanning her). Ensure the invite is
// received and can be accepted.
func TestUnbanViaInvite(t *testing.T) {
runtime.SkipIf(t, runtime.Synapse) // https://github.com/matrix-org/synapse/issues/1563
runtime.SkipIf(t, runtime.Synapse) // FIXME: https://github.com/matrix-org/synapse/issues/1563
deployment := Deploy(t, b.BlueprintFederationOneToOneRoom)
defer deployment.Destroy(t)

Expand Down
9 changes: 7 additions & 2 deletions tests/federation_room_join_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,10 @@ func TestJoinFederatedRoomWithUnverifiableEvents(t *testing.T) {
alice.JoinRoom(t, roomAlias, nil)
})
t.Run("/send_join response with state with unverifiable auth events shouldn't block room join", func(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // https://github.com/matrix-org/dendrite/issues/2028
// FIXME: https://github.com/matrix-org/dendrite/issues/2800
// (previously https://github.com/matrix-org/dendrite/issues/2028)
runtime.SkipIf(t, runtime.Dendrite)

room := srv.MustMakeRoom(t, ver, federation.InitialRoomEvents(ver, charlie))
roomAlias := srv.MakeAliasMapping("UnverifiableAuthEvents", room.RoomID)

Expand Down Expand Up @@ -528,7 +531,9 @@ func typeAndStateKeyForEvent(result gjson.Result) string {
}

func TestJoinFederatedRoomFromApplicationServiceBridgeUser(t *testing.T) {
runtime.SkipIf(t, runtime.Dendrite) // Dendrite doesn't read AS registration files from Complement yet
// Dendrite doesn't read AS registration files from Complement yet
runtime.SkipIf(t, runtime.Dendrite) // FIXME: https://github.com/matrix-org/complement/issues/514

deployment := Deploy(t, b.BlueprintHSWithApplicationService)
defer deployment.Destroy(t)

Expand Down
4 changes: 2 additions & 2 deletions tests/restricted_rooms_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ func TestRestrictedRoomsRemoteJoinLocalUser(t *testing.T) {
}

func doTestRestrictedRoomsRemoteJoinLocalUser(t *testing.T, roomVersion string, joinRule string) {
runtime.SkipIf(t, runtime.Dendrite) // requires more debugging
runtime.SkipIf(t, runtime.Dendrite) // FIXME: https://github.com/matrix-org/dendrite/issues/2801

deployment := Deploy(t, b.BlueprintFederationTwoLocalOneRemote)
defer deployment.Destroy(t)
Expand Down Expand Up @@ -352,7 +352,7 @@ func TestRestrictedRoomsRemoteJoinFailOver(t *testing.T) {
}

func doTestRestrictedRoomsRemoteJoinFailOver(t *testing.T, roomVersion string, joinRule string) {
runtime.SkipIf(t, runtime.Dendrite) // requires more debugging
runtime.SkipIf(t, runtime.Dendrite) // FIXME: https://github.com/matrix-org/dendrite/issues/2801

deployment := Deploy(t, b.Blueprint{
Name: "federation_three_homeservers",
Expand Down