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

add test to ensure you can query devicelist of invited users. #462

Closed
wants to merge 3 commits into from

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Jul 10, 2018

also add known broken test (commented out to stop it running and slowing things down
using the new broken_test directive) to track matrix-org/synapse#3503 - that we
don't get device_list updates from invited users.

also add known broken test (commented out to stop it running and slowing things down
using the new broken_test directive) to track matrix-org/synapse/#3503 - that we
don't get device_list updates from invited users.
@ara4n
Copy link
Member Author

ara4n commented Jul 10, 2018

@richvdh if you can cast an opinion on this mess at some point it'd be appreciated.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

these two tests seem very closely related, and I'm not quite sure which is doing what: they both seem to try to test what the other is claiming to be testing.

I'd be inclined to lump the two together, but ymmv.

local *multi_test = sub { _push_test( $filename, 1, @_ ); };
local *test = sub { _push_test( $filename, 0, @_ ); };
local *multi_test = sub { _push_test( $filename, 1, @_ ); };
local *broken_test = sub {};
Copy link
Member

Choose a reason for hiding this comment

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

well, if we're going to do this, then let's document what it's for, both here and in DEVELOP.rst. But I'm a bit unconvinced - the bug param means we'll expect it to fail, so why isn't that good enough?

Copy link
Member

Choose a reason for hiding this comment

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

stick a timeout on sync_until_user_in_device_list if necessary.

})->then( sub {
matrix_invite_user_to_room( $user1, $user2, $room_id )
# })->then( sub {
# # KNOWN BROKEN: (https://github.com/matrix-org/synapse/issues/3503)
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand what you're doing here. If you just comment it out and run the query anyway, surely it is going to fail, because the update might not yet have propagated over federation?

# })->then( sub {
# # KNOWN BROKEN: we should get an update from the invited user when they
# # update their devices, but we don't because of
# # https://github.com/matrix-org/synapse/issues/3504
Copy link
Member

Choose a reason for hiding this comment

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

I can't see 3504 getting fixed for quite a while, since it will require some significant changes to the way invites work. Building even a failing test around assuming it will work seems a bit pointless.

@richvdh richvdh assigned ara4n and unassigned richvdh Jul 18, 2018
@richvdh
Copy link
Member

richvdh commented May 15, 2019

Revisiting this 10 months later, I'm even more confused about what it is doing.

AFAICT it is meant to be a repro test for matrix-org/synapse#3503, however without a fix for that on the horizon, I don't think the open PR is adding much value. If, at some point in the future, we get around to addressing matrix-org/synapse#3503, we can always resurrect this PR.

@richvdh richvdh closed this May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants