Rework the sync tests #8435
Merged
Rework the sync tests #8435
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tgoyne
force-pushed
the
tg/rework-sync-tests
branch
3 times, most recently
from
656fa80
to
b22a853
Compare
tgoyne
changed the base branch from
master
to
tg/write-transaction-notification
tgoyne
force-pushed
the
tg/write-transaction-notification
branch
from
407d57b
to
a382ae5
Compare
tgoyne
force-pushed
the
tg/rework-sync-tests
branch
from
b22a853
to
a08817f
Compare
dianaafanador3
approved these changes
Dec 18, 2023
There was a problem hiding this comment.
Really great work into making the sync test suite better. just some minor comments. And as a plus, I'm missing a little bit more documentation/comments into our test cases common code, I guess if you take a look at this PR everything makes sense, but if you are someone new this make feel like magic, or you can even miss adding this for a new sync test file.
| @@ -0,0 +1,715 @@ | |||
| //////////////////////////////////////////////////////////////////////////// | |||
| // | |||
| // Copyright 2016 Realm Inc. | |||
There was a problem hiding this comment.
This date doesn't correspond to the date of creation of this file or test at least, in this and the other files created
| @@ -1228,4 +1252,7 @@ public class RealmServer: NSObject { | |||
| } | |||
| } | |||
|
|
|||
| extension String: Error { | |||
| while true { | ||
| let complete = try session.apps[appServerId].sync.progress.get() | ||
| .map { resp in | ||
| guard let resp = resp as? Dictionary<String, Any?> else { return false } |
There was a problem hiding this comment.
can we just have all this conditions in the same guard?
| - parameter type: The type of the object to be queried. | ||
| - parameter query: A query which will be used to modify the existing query. | ||
| */ | ||
| public func updateQuery<T: Object>(toType type: T.Type, where query: (Query<T>) -> Query<Bool>) { |
There was a problem hiding this comment.
why adding this, if the above API can be used for this
| @@ -166,6 +182,20 @@ import Realm.Private | |||
| self.predicate = query?(Query()).predicate ?? NSPredicate(format: "TRUEPREDICATE") | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Same in here, doesn't the above API already can be used for this?
There was a problem hiding this comment.
The comment in the body says why: optional function parameters are always escaping, but this function doesn't escape.
There was a problem hiding this comment.
sorry, didn't check the comment within the code, only the doctoring above
|
|
||
| let realm = try Realm(configuration: config1) | ||
| try realm.write { | ||
| try write { realm in |
There was a problem hiding this comment.
Why not use populateData, if we are checking for count and not for specific data
There was a problem hiding this comment.
The test is checking the number of sections, which depends on the number of unique first names.
tgoyne
force-pushed
the
tg/write-transaction-notification
branch
from
a382ae5
to
bdd98d3
Compare
tgoyne
force-pushed
the
tg/rework-sync-tests
branch
2 times, most recently
from
e2d0e0f
to
f2d95b6
Compare
|
I'm going to take a pass at writing some explanations of wtf the sync tests are doing, because you're absolutely right that there's a lot of complexity that's completely undocumented. |
Don't worry, I think is something I mentioned only as a bonus point |
…hange Core has allowed this for a while, but we had our own validation which made it not work.
The RLMSyncSubscriptionSet was retained until the task was deallocated even after the task completed, which is not strictly incorrect but made a test unreliable.
This compiles because the value can be implicitly converted to an optional, but it isn't actually checking anything. Even if the non-optional has an invalid nil value, it ends up as `.some(invalid)` and will pass the test.
We'll never get a success after an error, so not fulfilling the expectation just pointless waits for a timeout.
This unfortunately turned into a giant unreviewable mess, but I don't really know how to split it up. An overview of the changes: Each XCTestCase subclass now creates a server-side App which is used for most of the tests in that test case. Apps are now never shared between test cases, and do not share Mongo collections with other apps. Together this means that state from one test case should never bleed over to other test cases. This required rearranging many of the tests, as we had test cases with both PBS and FLX tests. While doing this I also discovered several tests which were just plain in the wrong test case due to files having multiple multi-thousand-line test cases and people thought they were adding tests to the one defined at the top. I split these files into multiple files, which I think makes things much more managable but unfortunately results in the diff being unhelpful. Each test case now explicitly defines which set of classes it uses, and only Rules for those classes is created in the server app. This cuts the time required to create apps roughly in half and helps offset the fact that we're now creating more apps. It also gets rid of the weird things like the hardcoded list of flx-compatible types in RealmServer. Creating an app now waits for the initial sync to complete. With PBS this is required to avoid some very strange bugs. With FLX it mostly ensures that if this times out for some reason we get a test failure there, rather than later in some very confusing place in the middle of a test. Client reset tests now use the API endpoint for FLX in addition to PBS. This makes them dramatically faster (several seconds instead of 30+). FLX tests now consistently follow the pattern of using one of the object fields as a partition key to query on rather than querying for all objects of a type. Some tests already did this, while others tried to clear the data first (which did not always work if the server was in the middle of processing old requests), and some just plain broke if tests were run in the wrong order. In the very early days of sync, opening the same Realm URL twice required two different processes, so all our sync tests spawned child processes. That hasn't been true for a very long time, but the tests stuck around and some more were written in that style due to mimicking the existing tests. I've ported almost all of them over to operating in a single process, which makes them both simpler and much faster (5s to .5s in many cases). The tests are now run with developer mode off. This was initially required due to the change where opening with a class subset is now considered a breaking change in developer mode, but now that test cases explicitly specify their types that isn't a problem any more. However, it does let us once again test subscriptions failing due to an unqueryable field, and that test revealed that we were using the wrong error domain for that error. I added some new helper functions for the things I discovered I was going to have to change in literally hundreds of places. Creating a temporary user is now just `self.createUser()` rather than separate steps of creating credentials and logging in. `self.name` is now used as the tag value for partitions and user names and such rather than `#function` or `NSStringFromSelector(_cmd)`, which makes it so that it doesn't have to be explicitly passed into helper functions. There were a number of places where this previously was done incorrectly and `#function` was used inside helper functions, which didn't achieve the desired effect.
tgoyne
force-pushed
the
tg/rework-sync-tests
branch
from
f2d95b6
to
710d6b6
Compare
tgoyne
force-pushed
the
tg/write-transaction-notification
branch
from
bdd98d3
to
535097a
Compare
tgoyne
changed the base branch from
tg/write-transaction-notification
to
master
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This unfortunately turned into a giant unreviewable mess, but I don't really know how to split it up. An overview of the changes:
Each XCTestCase subclass now creates a server-side App which is used for most of the tests in that test case. Apps are now never shared between test cases, and do not share Mongo collections with other apps. Together this means that state from one test case should never bleed over to other test cases.
This required rearranging many of the tests, as we had test cases with both PBS and FLX tests. While doing this I also discovered several tests which were just plain in the wrong test case due to files having multiple multi-thousand-line test cases and people thought they were adding tests to the one defined at the top. I split these files into multiple files, which I think makes things much more managable but unfortunately results in the diff being unhelpful.
Each test case now explicitly defines which set of classes it uses, and only Rules for those classes is created in the server app. This cuts the time required to create apps roughly in half and helps offset the fact that we're now creating more apps. It also gets rid of the weird things like the hardcoded list of flx-compatible types in RealmServer.
Creating an app now waits for the initial sync to complete. With PBS this is required to avoid some very strange bugs. With FLX it mostly ensures that if this times out for some reason we get a test failure there, rather than later in some very confusing place in the middle of a test.
Client reset tests now use the API endpoint for FLX in addition to PBS. This makes them dramatically faster (several seconds instead of 30+).
FLX tests now consistently follow the pattern of using one of the object fields as a partition key to query on rather than querying for all objects of a type. Some tests already did this, while others tried to clear the data first (which did not always work if the server was in the middle of processing old requests), and some just plain broke if tests were run in the wrong order.
In the very early days of sync, opening the same Realm URL twice required two different processes, so all our sync tests spawned child processes. That hasn't been true for a very long time, but the tests stuck around and some more were written in that style due to mimicking the existing tests. I've ported almost all of them over to operating in a single process, which makes them both simpler and much faster (5s to .5s in many cases).
The tests are now run with developer mode off. This was initially required due to the change where opening with a class subset is now considered a breaking change in developer mode, but now that test cases explicitly specify their types that isn't a problem any more. However, it does let us once again test subscriptions failing due to an unqueryable field, and that test revealed that we were using the wrong error domain for that error.
I added some new helper functions for the things I discovered I was going to have to change in literally hundreds of places. Creating a temporary user is now just
self.createUser()rather than separate steps of creating credentials and logging in.self.nameis now used as the tag value for partitions and user names and such rather than#functionorNSStringFromSelector(_cmd), which makes it so that it doesn't have to be explicitly passed into helper functions. There were a number of places where this previously was done incorrectly and#functionwas used inside helper functions, which didn't achieve the desired effect.