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 for /context/ with lazy_load_members filter #468

Merged
merged 4 commits into from
Aug 15, 2018

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Jul 20, 2018

@richvdh richvdh requested a review from a team July 23, 2018 10:50
@@ -42,3 +42,56 @@ sub new_User

return $user;
}


# assert that the given members are in the body of a sync response
Copy link
Member

Choose a reason for hiding this comment

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

this feels out of place here. did it need to move out of 15lazy-members.pl?



# assert that the given members are present in a block of state events
sub assert_room_members_in_state {
Copy link
Member

Choose a reason for hiding this comment

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

somehow this name makes me think we're checking the state contains at least these members. Maybe assert_state_room_members_matches ?

Copy link
Member

Choose a reason for hiding this comment

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

this also feels out of place. I'm not really keen on this file being used as a dumping ground for random assertion functions.

Maybe 10apidoc/09synced.pl is a more appropriate home?

sub assert_room_members_in_state {
my ( $events, $member_ids ) = @_;

log_if_fail "members:", $member_ids;
Copy link
Member

Choose a reason for hiding this comment

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

s/"members:"/"expected members:"/?

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.

generally lgtm modulo pedantry about naming and where functions go

@richvdh richvdh assigned ara4n and unassigned richvdh Jul 27, 2018
@richvdh richvdh mentioned this pull request Aug 13, 2018
And update log line to be more specific
@erikjohnston erikjohnston assigned richvdh and unassigned ara4n Aug 15, 2018
@richvdh
Copy link
Member

richvdh commented Aug 15, 2018

\o/

@richvdh richvdh changed the base branch from matthew/remove_redundant_lazy_members to develop August 15, 2018 18:26
@richvdh richvdh merged commit a3d88fa into develop Aug 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