Feature/fga/fix sdk integration tests #4568
Conversation
…ot cancelled while testing + fix small warnings
| */ | ||
| class Matrix private constructor(context: Context, matrixConfiguration: MatrixConfiguration) { | ||
| class TestMatrix constructor(context: Context, matrixConfiguration: MatrixConfiguration) { |
There was a problem hiding this comment.
it's in androidTest package so not needed I guess
| @@ -57,11 +55,9 @@ class SpaceCreationTest : InstrumentedTest { | |||
| val topic = "A public space for test" | |||
| var spaceId: String = "" | |||
| commonTestHelper.waitWithLatch { | |||
| GlobalScope.launch { | |||
There was a problem hiding this comment.
Since you are not using GlobalScope anymore, @Suppress("EXPERIMENTAL_API_USAGE") can be removed. There are maybe other places in the PR.
| return Realm.getInstance(monarchy.realmConfiguration).use { | ||
| val token = Realm.getInstance(monarchy.realmConfiguration).use { | ||
| // Makes sure realm is up-to-date as it's used for querying internally on non looper thread. | ||
| it.refresh() |
There was a problem hiding this comment.
Will it fix the notification about init sync not displayed after a clear cache? (not sure there is an existing issue about that)
| } | ||
|
|
||
| fun saveToken(realm: Realm, token: String?) { | ||
| val sync = SyncEntity(token) | ||
| realm.insertOrUpdate(sync) | ||
| if (realm.isInTransaction) { |
There was a problem hiding this comment.
it should always be the case, no? Else shouldn't we do something like crashing the app for instance?
There was a problem hiding this comment.
oh yes sorry, miss from testing
| /** | ||
| * Force foreground for testing | ||
| */ | ||
| internal class TestBackgroundDetectionObserver : BackgroundDetectionObserver { |
| UploadContentWorker(appContext, workerParameters, sessionManager) | ||
| else -> | ||
| // Return null to delegate to the default WorkerFactory. | ||
| null |
There was a problem hiding this comment.
Maybe log a warning for the developers, if they forget to add their new worker instanciation above?
|
Also for big 1d9da6c next time please do separate commits to split renaming of members, and more impactful changes, it will ease the review. Thanks! |
| if (workInfo.state == WorkInfo.State.FAILED) { | ||
| throw RuntimeException("MatrixWorkerFactory is not being set on your worker configuration.\n" + | ||
| "Makes sure to add it to a DelegatingWorkerFactory if you have your own factory or use it directly.\n" + | ||
| "You can grab the instance through the Matrix class.") |
There was a problem hiding this comment.
The message is maybe a bit hard to understand (to me at least) :/. I cannot propose better though, but maybe add a link to the Element code where it's done?
|
|
||
| /** | ||
| * This worker is launched by the factory with the isCreatedByMatrixWorkerFactory flag to true. | ||
| * If the MatrixWorkerFactory is not set up, it will default to the other constructor and it will throw |
There was a problem hiding this comment.
it will not throw but it will fail :)
| @@ -44,7 +44,7 @@ import javax.inject.Inject | |||
| /** | |||
| * This mimics the Matrix class but using TestMatrixComponent internally instead of regular MatrixComponent. | |||
| */ | |||
| class TestMatrix constructor(context: Context, matrixConfiguration: MatrixConfiguration) { | |||
| internal class TestMatrix constructor(context: Context, matrixConfiguration: MatrixConfiguration) { | |||
There was a problem hiding this comment.
CI does not like this change though. I think some other test classes have to be declared internal
This will fix #4546
All of the tests are not passing yet, but we are progressing.
Most notable stuff is introduction of TestMatrix class and MatrixWorkerFactory.