-
Notifications
You must be signed in to change notification settings - Fork 165
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
Updated core to 13.26.0 #3502
Updated core to 13.26.0 #3502
Conversation
Pull Request Test Coverage Report for Build 7640121693
💛 - Coveralls |
@nirinchev this should fix both errors that came out from CI
|
app ??= DefaultApp; | ||
app.Handle.SetFakeSyncRouteForTesting(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about this - I know it's just for tests, but if this is called with one of our integration apps, it would sets its sync route to a fake value, even though a real value should exist. I don't know if sync will refresh that route at some point, but I worry that as it stands, it's possible that running this test first, then running some integration tests that use the app, those will fail due to the route being misconfigured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmh, I understand. Good news is that all the tests are passing now, including sync tests, so I suppose it's not a problem "yet".
If we want to be more robust for the tests we could:
- Set the fake value only if the sync route is currently empty/null
- Set it to null/empty again at the end of the test (if modified)
- Maybe create infrastructure in core to create those fake synced realm more "organically"?
I am not sure if it's worth to do this though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is the only holdup and it's only for tests, what do you think about merging this and creating a follow up ticket so we come back to it when the core investigation is finished?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
Co-authored-by: Nikola Irinchev <[email protected]>
Description
Fixes #3482