Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Target specific thread name to unflake redaction tests #11662

Merged
merged 3 commits into from
Sep 28, 2023

Conversation

andybalaam
Copy link
Member

@andybalaam andybalaam commented Sep 25, 2023

Ensure that when we ask to open a thread called "Root2", we don't accidentally open a thread called "Root" that has "2 unread replies" in it.

Part of element-hq/element-web#25449


This change is marked as an internal change (Task), so will not be included in the changelog.

@andybalaam andybalaam added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Sep 25, 2023
@andybalaam andybalaam marked this pull request as ready for review September 25, 2023 11:44
@andybalaam andybalaam requested a review from a team as a code owner September 25, 2023 11:44
@andybalaam andybalaam requested review from dbkr and richvdh September 25, 2023 11:44
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.

don't quite get this. If the problem is that we match "Root2" when we look for "Root", won't we also match "SecondRoot"?

@andybalaam
Copy link
Member Author

don't quite get this. If the problem is that we match "Root2" when we look for "Root", won't we also match "SecondRoot"?

That is a very good question. The symptom I saw was that we appeared to match "Root" when we searched for "Root2".

I ran the test 10 times after the change and it works, but I agree that it would be very nice to know why.

@andybalaam
Copy link
Member Author

don't quite get this. If the problem is that we match "Root2" when we look for "Root", won't we also match "SecondRoot"?

That is a very good question. The symptom I saw was that we appeared to match "Root" when we searched for "Root2".

I ran the test 10 times after the change and it works, but I agree that it would be very nice to know why.

Thanks to @germain-gg we figured out that the cy.contains call is matching a thread root called "Root", followed by the string "2 replies". I think the correct fix is to stop using contains and be more specific, so I will try that.

@andybalaam andybalaam disabled auto-merge September 27, 2023 11:17
@andybalaam andybalaam changed the title Disambiguate thread root names to unflake redaction tests Target specific thread name to unflake redaction tests Sep 28, 2023
@andybalaam andybalaam requested a review from richvdh September 28, 2023 12:16
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.

I wouldn't have minded a comment explaining the need for caution here, but sure

@andybalaam andybalaam added this pull request to the merge queue Sep 28, 2023
Merged via the queue into develop with commit 58afa90 Sep 28, 2023
27 checks passed
@andybalaam andybalaam deleted the andybalaam/fix-flakes-in-redaction-tests branch September 28, 2023 13:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants