Skip to content

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Oct 5, 2018

No description provided.

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Wooo, tests! Sorry that I'm not a fan of interdependent tests :(


assert_json_keys( $content, "is_verified" );

assert_json_keys( $content, "session_data" );
Copy link
Member

Choose a reason for hiding this comment

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

You can do:

assert_json_keys( $content, qw( first_message_index forwarded_count is_verified session_data ) )

is_verified => JSON::false,
session_data => "anopaquestring",
}
);
Copy link
Member

Choose a reason for hiding this comment

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

weird indentation

content => {
algorithm => "m.megolm_backup.v1",
auth_data => "anopaquestring",
}
Copy link
Member

Choose a reason for hiding this comment

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

I think its going to be beneficial if we create matrix_create_backup_version etc functions. There's already a fair bit of duplication and it'll make it easier to write more backup tests.

@@ -0,0 +1,387 @@
my $fixture = local_user_fixture();

my $current_version; # FIXME: is there a better way of passing the backup version between tests?
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if each test was a standalone test, rather than there being implicit dependencies (it makes it harder to reason about each test and change later on).

I think once the APIs are split out into separate functions that will reduce boiler plate enough to make it feasible to just start from scratch for each test. There are also multi_test if you want to do multiple tests in one run through.

assert_json_keys( $content, "session_data" );

$content->{first_message_index} == 1 or
die "Expected first message index to match submitted data";
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to use assert_eq( $content->{first_message_index}, 1, ".." ) as it will have better logging on failures (in terms of logging what the actual values were?)

@dbkr
Copy link
Member Author

dbkr commented Oct 12, 2018

This should be better! ptal :)

@dbkr dbkr assigned erikjohnston and unassigned dbkr Oct 12, 2018
@dbkr dbkr merged commit f69c17e into develop Oct 15, 2018
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.

3 participants