-
Notifications
You must be signed in to change notification settings - Fork 88
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
Updates Dart tests to use new notification API #3354
Conversation
✅ Deploy Preview for device-sdk ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks for working on this, @MongoCaleb! I have a couple suggestions for the updated test. And a couple of things beyond my in-line comments:
- We may need to update the "Monitor sync progress", in the
manage_sync_session_test.dart
file. You may have already looked at this, but wanted to make sure.- It looks like we're already using the correct API for the monitor progress section of the Manage Sync Sessions page. However, we need to confirm that the content around the example describes the behavior and API correctly. Looks like this section was updated for v2.0.0 and probably doesn't account for the changes in the latest version.
- Can you add a Release Notes section to the PR? You're doing release notes this time, so it should make your life a bit easier later. 😄
}); | ||
// :snippet-end: | ||
expect(realm.isClosed, false); | ||
expect(transferred, transferable); | ||
expect(transferred, greaterThanOrEqualTo(0)); | ||
expect(progress, greaterThanOrEqualTo(-1)); |
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.
This expect statement will always evaluate to true because we set progress
to -1
on line 74. So, sync progress could be nothing or it could be complete. I think we'd want to test that the sync is complete, so should be looking for greater than -1
, not equal to.
I think the await
on realm open will wait for sync to finish, but I'm not certain of the default behavior for the Flutter SDK. Either way, we'll need to make sure we're waiting for sync to finish so that we can get the expected result.
Alternatively, we could change the type and value of progress
. Within the if
block on line 80, we could set progress
to "Transfer complete" or something. Then assert equality between the expected string and what is actually in progress
.
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.
General comment above: I believe the page content around each of the code snippets is correct. However, the one you link (Monitor Progress) shows using getProgressStream. This is not deprecated, so I think it's good to leave in here. Do you think it should be replaced with the other way to do it (as shown in this PR)?
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.
Per the new info from Kaspar, we're good to have both.
Can we add a mention of getProgressStream
to the section where the onProgressCallback
example is used? We don't talk about the fact that they're really the same thing anywhere. It would be nice to have info about when devs might use one instead of the other. But at least acknowledging that there are two ways to add progress notifications would be great.
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.
Also, thanks for digging into this! I had no idea there were two ways to do this. 😆
progress = syncProgress.progressEstimate; | ||
// Percent complete == progress * 100 | ||
if (syncProgress.progressEstimate == 1.0) { | ||
//transfer is complete |
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.
//transfer is complete | |
// Transfer is complete |
Note: we are keeping information about both
|
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.
Thanks for your work on this, @MongoCaleb! LGTM.
I have a couple of comments you should consider before merging, but they're not blocking comments.
This one in particular could help devs navigate this feature a bit: #3354 (comment)
Future<void> addSubscriptions(Realm realm, String searchByPrefix) async { | ||
final query = realm.query<Tricycle>(r'name BEGINSWITH $0', ["a"]); | ||
if (realm.subscriptions.find(query) == null) { | ||
realm.subscriptions.update((mutableSubscriptions) => mutableSubscriptions.add(query)); | ||
} | ||
await realm.subscriptions.waitForSynchronization(); | ||
} | ||
|
||
Future<Configuration> subscribeForAtlasAddedData(App app, {int itemsCount = 100}) async { | ||
final productNamePrefix = "a"; | ||
final user1 = await app.logIn(Credentials.anonymous(reuseCredentials: false)); | ||
final config1 = Configuration.flexibleSync(user1, [Tricycle.schema]); | ||
final realm1 = Realm(config1); | ||
await addSubscriptions(realm1, productNamePrefix); | ||
realm1.close(); | ||
|
||
final user2 = await app.logIn(Credentials.anonymous(reuseCredentials: false)); | ||
final config2 = Configuration.flexibleSync(user2, [Tricycle.schema]); | ||
final realm2 = Realm(config2); | ||
|
||
await addSubscriptions(realm2, productNamePrefix); | ||
|
||
final trikes = realm2.all<Tricycle>(); | ||
realm2.write(() { | ||
realm2.deleteMany(trikes); | ||
realm2.add(Tricycle(1001, "a1001")); | ||
realm2.add(Tricycle(2002, "a2002")); | ||
realm2.add(Tricycle(3003, "a3003")); | ||
}); | ||
|
||
await realm2.syncSession.waitForUpload(); | ||
await realm2.syncSession.waitForDownload(); | ||
realm2.close(); | ||
return config1; | ||
} |
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.
Is there a reason to have these functions outside of the "Track upload progress" test that starts on line 106? They're only for that test, right?
Readability for Commit Hash: a2936a0 You can see any previous Readability scores (if they exist) by looking Readability scores for changed documents:
For Grade Level, aim for 8 or below. For Reading Ease scores, aim for 60 or above:
For help improving readability, try Hemingway App. |
@krollins-mdb added notes. |
✨ Staging URL: https://preview-mongodbmongodb.gatsbyjs.io/realm/master/ 🪵 Logs |
Pull Request Info - SDK Docs Consolidation
Jira ticket: hhttps://jira.mongodb.org/browse/DOCSP-40843
Staging Links
Release Notes
dart fix
to clean up all code examples and then re-bluehawk them all.