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

test to ensure spurious state doesn't leak into incr sync responses when LLing #487

Merged
merged 6 commits into from
Sep 18, 2018

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Aug 25, 2018

@ara4n
Copy link
Member Author

ara4n commented Aug 25, 2018

@matrixbot retest this please

@ara4n ara4n requested a review from a team August 25, 2018 10:41
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.

looks plausible modulo some wibbling about the interface to the utility function

@@ -205,7 +205,34 @@ sub await_sync_presence_contains {
}


push @EXPORT, qw( assert_room_members assert_state_room_members_matches );
push @EXPORT, qw( assert_room_members assert_state_room_members_matches assert_room_state);
Copy link
Member

Choose a reason for hiding this comment

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

nice to have: generally it seems helpful to have a push @EXPORT next to each func being exported, rather than one big one at the top

Copy link
Member Author

Choose a reason for hiding this comment

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

the codebase is very inconsistent on this, but have moved them.

push @EXPORT, qw( assert_room_members assert_state_room_members_matches );
push @EXPORT, qw( assert_room_members assert_state_room_members_matches assert_room_state);

# assert that the state body of a sync response is made up of the given state types.
Copy link
Member

Choose a reason for hiding this comment

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

nice to have: generally we seem to be using POD for this sort of thing. It'd be nice to be consistent, even if it's slightly more verbose.


# assert that the state body of a sync response is made up of the given state types.
sub assert_room_state {
my ( $body, $room_id, $state_types ) = @_;
Copy link
Member

Choose a reason for hiding this comment

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

some docs on what state_types should look like would be helpful, to save people reverse-engineering it.

push @EXPORT, qw( assert_room_members assert_state_room_members_matches assert_room_state);

# assert that the state body of a sync response is made up of the given state types.
sub assert_room_state {
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be assert_room_state_types_match or something, because (a) it only checks for the types, not the whole state, and (b) it checks that they match exactly (as opposed to, for example, being a superset of the desired list)

Copy link
Member

Choose a reason for hiding this comment

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

Also, should it be assert_state_types_match and take a state block, ie pull lines 214-215 up to the caller, making this equivalent to assert_state_room_members_matches rather than assert_room_members? That feels like it would be more intuitive to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

unconvinced about passing in a random list of state events rather than a sync body response, but whatever; have done so

@@ -258,7 +267,6 @@
});
};


Copy link
Member

Choose a reason for hiding this comment

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

nit: this is inconsistent...

@ara4n
Copy link
Member Author

ara4n commented Aug 28, 2018

@richvdh ptal

@ara4n ara4n assigned ara4n and unassigned ara4n Aug 28, 2018

log_if_fail "Room", $room;
$state_types is an arrayref of arrayrefs, each a tuple of type & state_key.
Copy link
Member

Choose a reason for hiding this comment

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

an example might have been helpful.

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

@ara4n
Copy link
Member Author

ara4n commented Sep 18, 2018

retest this please

@ara4n ara4n merged commit e2ea627 into develop Sep 18, 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.

2 participants