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

Allow search results to appear in multiple orders #479

Merged
merged 2 commits into from
Sep 27, 2022
Merged

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.

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
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":[]}}}

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search_works_across_an_upgraded_room_and_its_predecessor is flaky
4 participants