Skip to content

Allow search results to appear in multiple orders#479

Merged
richvdh merged 2 commits intomainfrom
rav/fix_search_test
Sep 27, 2022
Merged

Allow search results to appear in multiple orders#479
richvdh merged 2 commits intomainfrom
rav/fix_search_test

Conversation

@richvdh
Copy link
Member

@richvdh richvdh commented Sep 27, 2022

Fixes #478, I hope, though there may be other tests with the same problem.

@richvdh
Allow search results to appear in multiple orders
a98d9e4 
Fixes #478, I hope, though there
may be other tests with the same problem.
@richvdh richvdh requested review from a team as code owners September 27, 2022 11:10
squahtx
squahtx approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@squahtx squahtx left a comment

Choose a reason for hiding this comment

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

lgtm

@S7evinK
Copy link
Contributor

S7evinK commented Sep 27, 2022

@richvdh RE your comment

The problem here is that the complement test is assuming that the search results will be returned in a particular order, which I don't think is correct (and certainly the sytest that it was ported from did not make that assumption).

Looking at the deleted Sytest code, it definitely did that assumption.

@S7evinK
Copy link
Contributor

S7evinK commented Sep 27, 2022

Aha.. Looking closer.. It might be related to the different message body? message 1 vs message before upgrade?

@richvdh
Copy link
Member Author

richvdh commented Sep 27, 2022

Looking at the deleted Sytest code, it definitely did that assumption

I don't think so. Look at https://github.com/matrix-org/sytest/blob/bebf431edf7311d5147139f44dab97131a784d29/tests/43search.pl#L287: it sets $result to the first "result" whose event_id matches $event_id_one, and then proceeds to do checks on $result. That means it will pass even if "message 1" appears second in the result list. It then goes on to do something similar for "message 2".

@S7evinK
Copy link
Contributor

S7evinK commented Sep 27, 2022

Oh, true, didn't read that right.

@kegsay
Copy link
Member

kegsay commented Sep 27, 2022

Though CI seems to be failing:

TestSearch/parallel/Search_works_across_an_upgraded_room_and_its_predecessor (1.09s)
      apidoc_search_test.go:208: SendEventSynced waiting for event ID $5_V3NGmyDWKZwYZLCdmopuvHpzes6ao4ILTml5aQAGo
      apidoc_search_test.go:223: SendEventSynced waiting for event ID $1xoBlX0wz7xtwUeiit0K72msQ0cC8wOIvsFe17e_Xck
      apidoc_search_test.go:250: MatchResponse JSONCheckOff: unexpected item  - http://localhost:49342/_matrix/client/v3/search => {"search_categories":{"room_events":{"results":[{"rank":0.5,"result":{"type":"m.room.message","room_id":"!zgnaljHeflTPgVGwXI:hs1","sender":"@alice:hs1","content":{"body":"Message before upgrade","msgtype":"m.text"},"origin_server_ts":1664277514508,"unsigned":{"age":733},"event_id":"$5_V3NGmyDWKZwYZLCdmopuvHpzes6ao4ILTml5aQAGo","user_id":"@alice:hs1","age":733},"context":{}},{"rank":0.5,"result":{"type":"m.room.message","room_id":"!ZrmSMkfiHQuBjbdUuD:hs1","sender":"@alice:hs1","content":{"body":"Message after upgrade","msgtype":"m.text"},"origin_server_ts":1664277515138,"unsigned":{"age":103},"event_id":"$1xoBlX0wz7xtwUeiit0K72msQ0cC8wOIvsFe17e_Xck","user_id":"@alice:hs1","age":103},"context":{}}],"count":2,"highlights":[]}}}

kegsay
kegsay reviewed Sep 27, 2022
View reviewed changes
@richvdh richvdh merged commit c04aeb6 into main Sep 27, 2022
@richvdh richvdh deleted the rav/fix_search_test branch September 27, 2022 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kegsay kegsay kegsay approved these changes

+1 more reviewer

@squahtx squahtx squahtx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Search_works_across_an_upgraded_room_and_its_predecessor is flaky

4 participants

@richvdh @S7evinK @kegsay @squahtx