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

Integration tests should wait until the room is ready #1516

Merged
merged 2 commits into from
Jul 8, 2022

Conversation

Anderas
Copy link
Contributor

@Anderas Anderas commented Jul 8, 2022

Some of the flaky integration tests are failing because they manually start a session and try to get a room immediately. The problem with that approach is not only that the room may not be created at that point, but even if it is, not all state may be properly updated in the room summary.

The (partial) solution to this is to start sessions and then listen to global notifications that announce room creation or sync completeion. It is not 100% reliable, and requires a bit of manual delay, but it seems to be more reliable than the current setup.

Additionally I decided to enable the entire suite of MXCryptoTests and disable in code those that do not pass. This makes it more obvious when reading the test code and automatically adds any new tests to the test plan.

@Anderas Anderas requested review from a team and pixlwave and removed request for a team July 8, 2022 13:52
Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment on lines 3441 to 3442
- (void)restartSession:(MXSession *)session
waitingForRoomId:(NSString *)roomId
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is a little out of on this and the 2 calls to it.


// Even when sync completed, there are actually still a few async updates that happen (i.e. the notification
// fires too early), so have to add some small arbitrary delay.
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 1 * NSEC_PER_SEC), dispatch_get_main_queue(), ^{
Copy link
Member

Choose a reason for hiding this comment

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

Kind of annoying to have to stall for a whole second, but can see why it is needed 🙁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea it is indeed. I would fix the notification, but then have no test to proove that I did not break something else, so this should be passing first before any changes to production code.

Comment on lines 3452 to 3453
observer = [[NSNotificationCenter defaultCenter] addObserverForName:kMXSessionNewRoomNotification
object:nil
Copy link
Member

Choose a reason for hiding this comment

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

Indentation here also.

@pixlwave
Copy link
Member

pixlwave commented Jul 8, 2022

Oh, but missing a changelog, I forgot to add that to the review.

@Anderas Anderas merged commit da888f1 into develop Jul 8, 2022
@Anderas Anderas deleted the andy/fix_tests branch July 8, 2022 17:01
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