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 /search tests #464

Merged
merged 7 commits into from
Sep 19, 2022
Merged

Add /search tests #464

merged 7 commits into from
Sep 19, 2022

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Sep 15, 2022

Adds the tests from tests/43search.pl

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great!

Two minor questions:

"github.com/matrix-org/complement/runtime"
)

// Note: In contrast to Sytest, we define a filter.rooms on each search request, this allows us to run in parallel.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the point that sytest assumes each test was run in isolation, so that there were no other rooms to search for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, Sytest is creating separate a user and room for each test, so would only receive the results from those rooms.
Since we're only using Alice and a new room for each test, we need to filter on the room.

Comment on lines 112 to 126
must.MatchResponse(t, resp, match.HTTPResponse{
StatusCode: http.StatusOK,
JSON: []match.JSON{
match.JSONKeyPresent(sce + ".count"),
match.JSONKeyPresent(sce + ".results"),
match.JSONKeyEqual(sce+".count", float64(1)),
match.JSONKeyEqual(result0+".room_id", roomID),
match.JSONKeyPresent(result0 + ".content"),
match.JSONKeyPresent(result0 + ".type"),
match.JSONKeyEqual(result0+".content.body", "Message number 4"),
match.JSONKeyEqual(resBefore+".0.content.body", "Message number 3"),
match.JSONKeyEqual(resBefore+".1.content.body", "Message number 2"),
match.JSONKeyEqual(resAfter+".0.content.body", "Message number 5"),
match.JSONKeyEqual(resAfter+".1.content.body", "Message number 6"),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the sytest has some extra checks here. Do we want to carry those over?

  • next_batch from assert_json_keys( $room_events, qw( count results next_batch ) );
  • context from
           my $context = $results->[0]{context};
           assert_json_keys( $context, qw( events_before events_after ));
           my $events_before = $context->{events_before};
           my $events_after = $context->{events_after};
    
           assert_eq( scalar @$events_before, 2, 'events_before' );
           assert_eq( scalar @$events_after, 2, 'events_after' );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, seems like I've removed one next_batch check too much.
For context: We're checking

sce := "search_categories.room_events"
result0 := sce + ".results.0.result"
resBefore := sce + ".results.0.context.events_before"
resAfter := sce + ".results.0.context.events_after"
[...]
match.JSONKeyEqual(resBefore+".0.content.body", "Message number 3"),
match.JSONKeyEqual(resBefore+".1.content.body", "Message number 2"),
match.JSONKeyEqual(resAfter+".0.content.body", "Message number 5"),
match.JSONKeyEqual(resAfter+".1.content.body", "Message number 6"),

which ensures context as well as the other keys (events_before/after) are present.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh sorry, I missed events_before and events_after. It does all start to blur together if you're not careful!

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Brilliant, LGTM!

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