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.

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

Jump to bottom

Test for SYN-589, that old left rooms don't get sent down new links #139

Merged
merged 4 commits into from
Jan 13, 2016
Merged

Test for SYN-589, that old left rooms don't get sent down new links #139

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d9660e1
Test for SYN-589, that old left rooms don't get sent down new links
erikjohnston Jan 12, 2016
e92b7bc
Fix test to work again
erikjohnston Jan 12, 2016
b1c9a91
Don't send needlessly many messages
erikjohnston Jan 12, 2016
b5de90a
Remove bug annotation
erikjohnston Jan 13, 2016
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
63 changes: 63 additions & 0 deletions tests/31sync/09archived.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,69 @@
};


test "Oldly left rooms don't appear in the leave section of sync",
Copy link
Contributor

Choose a reason for hiding this comment

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

"Oldly" ??

Copy link
Member Author

Choose a reason for hiding this comment

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

Hush

requires => [ local_user_fixture( with_events => 0 ), local_user_fixture( with_events => 0 ),
qw( can_sync ) ],

bug => 'SYN-589',

check => sub {
my ( $user, $user2 ) = @_;

my ( $filter_id, $room_id_1, $room_id_2, $next );

my $filter = {
room => { timeline => { limit => 1 }, include_leave => JSON::true }
};

matrix_create_filter( $user, $filter )->then( sub {
( $filter_id ) = @_;

Future->needs_all(
matrix_create_room( $user )->on_done( sub { ( $room_id_1 ) = @_; } ),
matrix_create_room( $user )->on_done( sub { ( $room_id_2 ) = @_; } ),
);
})->then( sub {
matrix_join_room( $user2, $room_id_1 );
})->then( sub {
matrix_sync( $user, filter => $filter_id );
})->then( sub {
my ( $body ) = @_;

$next = $body->{next_batch};

Future->done(1);
})->then( sub {
Copy link
Contributor

Choose a reason for hiding this comment

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

The combination of the above two lines is redundant; they can be deleted.

$next = $body->{next_batch};

matrix_leave_room( ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ta

matrix_leave_room( $user, $room_id_1 );
})->then( sub {
matrix_sync( $user, filter => $filter_id );
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to provide a since argument here? Otherwise, you're just about to overwrite the $next variable, having not actually used it since it was written first on line 153.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ta

})->then( sub {
my ( $body ) = @_;

$next = $body->{next_batch};
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to assert that you definitely did see the newly-left room here, or is that checked already in another test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its tested elsewhere.

I'm not sure how much I want to be complicating tests testing things that are tested elsewhere. I can do either.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's tested elsewhere then no need to add it here, indeed.


Future->done(1);
})->then( sub {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto these

Copy link
Member Author

Choose a reason for hiding this comment

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

Ta

Future->needs_all( map {
matrix_put_room_state( $user2, $room_id_1,
content => { "filler" => $_, membership => "join" },
type => "m.room.member",
state_key => $user2->user_id,
)
} 0 .. 20 );
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to create 21 items of filler state - is that number really necessary? Would the test work with fewer, or even just one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've lowered it and added back a comment.

})->then( sub {
matrix_sync( $user, filter => $filter_id, since => $next );
})->then( sub {
my ( $body ) = @_;

assert_json_keys( $body->{rooms}{leave} , qw( ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably don't want to use assert_json_keys here. What that function does is assert that it's given a JSON object, which contains at least the named keys. It won't mind if there's extra.

Instead you likely want something like:

assert_json_object( my $leave = $body->{rooms}{leave} );
keys %$leave == 0 or die "Expected no rooms in 'leave' state";

Copy link
Member Author

Choose a reason for hiding this comment

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

cough


Future->done(1);
});
};



test "Left rooms appear in the leave section of full state sync",
requires => [ local_user_fixture( with_events => 0 ),
qw( can_sync ) ],
Expand Down