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 and update tests for new behaviour with empty backups #519

Merged
merged 6 commits into from
Nov 5, 2018

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Oct 30, 2018

No description provided.

@uhoreg
Copy link
Member Author

uhoreg commented Oct 30, 2018

corresponds to matrix-org/synapse#4123

@uhoreg
Copy link
Member Author

uhoreg commented Oct 30, 2018

The tests in the corresponding synapse PR run fine, so I'm not sure why they're failing here, and the test results seem to be empty. Unless it was because I did a force-push to the synapse branch while these tests were running... 🤷‍♂️

@uhoreg uhoreg requested a review from a team October 30, 2018 18:58
@richvdh
Copy link
Member

richvdh commented Oct 30, 2018

Aborting test run due to failure to start test server

-- for some reason, synapse didn't start.

@matrixbot retest this please


Future->done(1);
matrix_get_backup_key( $user, '', '', 'bogusversion');
})->main::expect_http_404;
Copy link
Member

Choose a reason for hiding this comment

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

can you use expect_m_not_found, to check the error code as well as the http code?

matrix_get_key_backup_info( $user )->then( sub {
my ( $content ) = @_;

log_if_fail "Content", $content;
Copy link
Member

Choose a reason for hiding this comment

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

could we have a more descriptive description than "Content" ? (likewise below)

$version = $content->{version};

matrix_get_backup_key( $user, '!notaroom', 'notassession', $version);
})->main::expect_http_4xx
Copy link
Member

Choose a reason for hiding this comment

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

why is this a 4xx rather than something more specific? Also, please can we have a comment to say what we are testing here?


$version = $content->{version};

matrix_get_backup_key( $user, '!notaroom', 'notassession', $version);
Copy link
Member

Choose a reason for hiding this comment

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

missing a space before the )

@@ -393,9 +435,19 @@ =head2 matrix_get_backup_key
sub matrix_get_backup_key {
my ( $user, $room_id, $session_id, $version ) = @_;

my $uri;

if ($session_id) {
Copy link
Member

Choose a reason for hiding this comment

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

probably better to check for definedness rather than truthiness (if ( defined $session_id )) to distinguish between an absent param and an empty one.

Also, spaces inside parens please. (I don't make the rules...)

matrix_get_backup_key( $user, '!notaroom', 'notassession', $version);
})->main::expect_http_4xx
->then( sub {
matrix_get_backup_key( $user, '!notaroom', '', $version);
Copy link
Member

Choose a reason for hiding this comment

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

Again, it would be useful to have some more words to explain what we are testing and what we expect the result to be.

@uhoreg uhoreg requested a review from a team November 1, 2018 04:44
@turt2live turt2live requested review from a team and removed request for a team November 1, 2018 04:58
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.

lgtm apart from the nits below. Could you fix them, then merge?

@@ -384,18 +435,28 @@ sub matrix_backup_keys {

=head2 matrix_get_backup_key

matrix_get_backup_key( $user, $room_id, $session_id, $version )
matrix_get_backup_key( $user, $version, $room_id, $session_id )

Send keys to a given key backup version
Copy link
Member

Choose a reason for hiding this comment

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

this looks wrong. could you fix it while you're here?


assert_deeply_eq( $content, { "rooms" => {} } );

Future->done(1);
Copy link
Member

Choose a reason for hiding this comment

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

we don't need this

@uhoreg uhoreg merged commit 546e580 into develop Nov 5, 2018
@uhoreg uhoreg deleted the uhoreg/e2e_backup_fix_404 branch November 5, 2018 22:59
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.

2 participants