Add tests for QBS GeoSpatial #6223

takameyer · 2023-10-31T12:44:11Z

What, How & Why?

Expanded previous tests with a useFlexibleSync flag
Set id to _id as it is required for QBS
Added special logic to initialize app, auth and sync data
Updated all utility test methods to be async and peform sub/unsub for qbs
- Also removed foreach as the callback is completely ignored by mocha when set to async
Disabled duplicate tests

Possible Followup

A skipped exception test for Impossible Box is possibly able to be reenabled, but must ask core for exception
Ignored altitude in local queries throws in QBS. Perhaps they should be unified in their behavior between core and baas.

☑️ ToDos

📝 Changelog entry
📝 Compatibility label is updated or copied from previous entry
📝 Update COMPATIBILITY.md
🚦 Tests
📦 Updated internal package version in consuming package.jsons (if updating internal packages)
📱 Check the React Native/other sample apps work if necessary
💥 Breaking label has been applied or is not necessary

* Expanded previous tests with a `useFlexibleSync` flag * Added special logic to initialize app, auth and sync data * Updated all utility test methods to be async and peform sub/unsub for qbs * Disabled duplicate tests

kneth

It is good to see that our geospatial queries now can be used with flexible sync.

kneth · 2023-11-01T10:57:45Z

integration-tests/tests/src/tests/queries.ts

+        expectedLength,
+        `Expected length ${expectedLength} for query: ${queryString} ${JSON.stringify(queryArgs)}`,
+      );
+      if (useFlexibleSync) {


Could it make sense to turn it into a small function? I mean, it is repeated many places.

I do like to repeat myself in tests, but I can see that this is replicated maybe a bit too much.

elle-j · 2023-11-01T09:38:51Z

integration-tests/tests/src/tests/queries.ts

+      if (useFlexibleSync) {
+        await filtered.subscribe({ behavior: WaitForSync.Always });
+      }


I'm thinking it may be best to add a unique name as part of the subscription options as well, so that the removal of the subscription specifically targets this subscription. In this case, my comment would also apply to the functions expectQueryResultValues() and expectQueryResultValuesByName().

But with this "new" subscription mechanism it relieves the need for this. I think for testing it's okay to just unsubscribe from the original model, no?
I do agree that in actual application code, one should be using names to track their subscriptions.

I think the reason in this case to also use a name would be if the same realm is used at the same the by some other test and that test relies on a subscription with the same query. Then that subscription would be removed. But I'm definitely unsure if this scenario (same realm, parallel test, same query) is possible or likely in our tests.

We can cross that bridge when we come to it. I think we will start having many problems in many areas if we start adding parallelizations to our test framework (which then would need this type of optimization, including realm file paths, etc).

elle-j · 2023-11-01T09:41:27Z

integration-tests/tests/src/tests/queries.ts

+      if (useFlexibleSync) {
+        await filtered.subscribe({ behavior: WaitForSync.Always });
+      }
+      expect(filtered.length).equal(


Usage of expect().equal() and expect().to.equal() are being mixed in the test. Could we pick just one of them? 🙂

I didn't write this, but if I did, I would have chosen expect().to.equal()

Ditto to.equal()

elle-j · 2023-11-01T10:06:58Z

integration-tests/tests/src/tests/queries.ts

+        useFlexibleSync && authenticateUserBefore();
+        const realmConfig = (useFlexibleSync
+          ? { schema: [PointOfInterest, MyGeoPoint.schema], sync: { flexible: true } }
+          : { schema: [PointOfInterest, MyGeoPoint.schema] }) as unknown as OpenRealmConfiguration;


Why was the as unknown as.. needed here? Even though the sync is not taking a user here, it looks like the type system should not have an issue with that since OpenRealmConfiguration allows Partial<SyncConfiguration>. So maybe that's not the issue here?

I was getting type errors and this was my quick and dirty solution without digging into why this Partial wasn't taken by our internal hooks. However, I could take some extra time to make this clearer.

Alrighty, yeah probably don't have to spend too much time on it tho 👍

elle-j · 2023-11-01T10:19:13Z

integration-tests/tests/src/tests/queries.ts

-          center: { latitude: -25, longitude: -32.34 },
-          distance: 0.5,
-        };
+        async function geoTest(


Looks like async shouldn't be needed here. The Promise return type is inferred in this function.

Suggested change

async function geoTest(

function geoTest(

elle-j · 2023-11-01T10:44:10Z

integration-tests/tests/src/tests/queries.ts

+          if (useFlexibleSync) {
+            const fullConfig = {
+              ...realmConfig,
+              sync: { ...realmConfig.sync, user: this.user },
+            } as unknown as Realm.ConfigurationWithSync;
+            await this.realm.syncSession?.uploadAllLocalChanges();
+            objects.unsubscribe();
+            this.realm.close();
+            Realm.deleteFile(fullConfig);
+            this.realm = await Realm.open(fullConfig);


Could you add some inline comments to why this beforeEach needs to unsubscribe, close and delete the realm, and the reopen it, please 🙂

Also, perhaps an expect() would be good after the last line (after reopening the realm again) to verify the state is what you expect (and helps the reader understand what should be expected). I assume e.g. you'd expect the length of this.realm.objects(PointOfInterest) to be 0 since there are no subscriptions.

We could also extract this into a reusable hook. I will add some comments, as this did take a fair amount of trickery to get right.

integration-tests/tests/src/tests/queries.ts

elle-j · 2023-11-01T11:33:58Z

integration-tests/tests/src/tests/queries.ts

For the tests, perhaps only using flexible sync would be enough? Rather than doing both local and synced tests. Was there a reason for adding rather than replacing?

I agree. Refactoring now.

Cool, this will definitely simplify things as well 🙂

* Flexible sync only for geospatial * Convenience function for tear down * Add error case for altitude * Refactor altitude test to box, since circle is erroneous from baas

coveralls-official · 2023-11-02T12:08:37Z

coverage: 85.397% (-0.008%) from 85.405% when pulling b751de1 on andrew/qbs_geospatial_tests into 33835a8 on main.

elle-j

Looks great 💯

elle-j · 2023-11-02T12:10:54Z

integration-tests/tests/src/tests/queries.ts

@@ -231,23 +196,25 @@ const expectQueryException = (
 * For example: (r, "Obj", "intCol" [[3, 4], "intCol > 2]) => querying "intCol > 2" should
 * return results whose elements' intCol property values are 3 and 4.
 */
-const expectQueryResultValues = (
+const expectQueryResultValues = async (


Suggested change

const expectQueryResultValues = async (

const expectQueryResultValues = (

elle-j · 2023-11-02T12:11:16Z

integration-tests/tests/src/tests/queries.ts

-      expectedResults,
-      `
+  return Promise.all(
+    queryResultPairs.map(async ([expectedResults, queryString, ...queryArgs]) => {


Suggested change

queryResultPairs.map(async ([expectedResults, queryString, ...queryArgs]) => {

queryResultPairs.map(([expectedResults, queryString, ...queryArgs]) => {

elle-j · 2023-11-02T12:12:08Z

integration-tests/tests/src/tests/queries.ts

-      expectedResults,
-      `
+  return Promise.all(
+    queryResultPairs.map(async ([expectedResults, queryString, ...queryArgs]) => {


Suggested change

queryResultPairs.map(async ([expectedResults, queryString, ...queryArgs]) => {

queryResultPairs.map(([expectedResults, queryString, ...queryArgs]) => {

integration-tests/tests/src/tests/sync/geospatial.ts

Co-authored-by: LJ <[email protected]>

Add tests for QBS GeoSpatial

105d616

* Expanded previous tests with a `useFlexibleSync` flag * Added special logic to initialize app, auth and sync data * Updated all utility test methods to be async and peform sub/unsub for qbs * Disabled duplicate tests

github-actions bot assigned takameyer Oct 31, 2023

takameyer requested review from kneth and elle-j October 31, 2023 12:45

kneth approved these changes Nov 1, 2023

View reviewed changes

elle-j reviewed Nov 1, 2023

View reviewed changes

PR comments

417caec

* Flexible sync only for geospatial * Convenience function for tear down * Add error case for altitude * Refactor altitude test to box, since circle is erroneous from baas

takameyer force-pushed the andrew/qbs_geospatial_tests branch from 654ca31 to 417caec Compare November 2, 2023 12:00

elle-j approved these changes Nov 2, 2023

View reviewed changes

takameyer and others added 2 commits November 2, 2023 13:28

Apply suggestions from code review

a2e6f07

Co-authored-by: LJ <[email protected]>

Last updates

b751de1

takameyer merged commit d315db1 into main Nov 30, 2023
22 of 27 checks passed

takameyer deleted the andrew/qbs_geospatial_tests branch November 30, 2023 08:45

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for QBS GeoSpatial #6223

Add tests for QBS GeoSpatial #6223

takameyer commented Oct 31, 2023 •

edited

Loading

kneth left a comment

kneth Nov 1, 2023

takameyer Nov 2, 2023

elle-j Nov 1, 2023

takameyer Nov 2, 2023

elle-j Nov 2, 2023

takameyer Nov 2, 2023

elle-j Nov 1, 2023 •

edited

Loading

takameyer Nov 2, 2023

elle-j Nov 2, 2023

elle-j Nov 1, 2023

takameyer Nov 2, 2023

elle-j Nov 2, 2023

elle-j Nov 1, 2023

elle-j Nov 1, 2023

takameyer Nov 2, 2023

elle-j Nov 2, 2023

elle-j Nov 1, 2023

takameyer Nov 2, 2023

elle-j Nov 2, 2023

coveralls-official bot commented Nov 2, 2023 •

edited

Loading

elle-j left a comment

elle-j Nov 2, 2023

elle-j Nov 2, 2023

elle-j Nov 2, 2023

	const expectQueryResultValues = async (
	const expectQueryResultValues = (

	queryResultPairs.map(async ([expectedResults, queryString, ...queryArgs]) => {
	queryResultPairs.map(([expectedResults, queryString, ...queryArgs]) => {

Add tests for QBS GeoSpatial #6223

Add tests for QBS GeoSpatial #6223

Conversation

takameyer commented Oct 31, 2023 • edited Loading

What, How & Why?

Possible Followup

☑️ ToDos

kneth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elle-j Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls-official bot commented Nov 2, 2023 • edited Loading

elle-j left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takameyer commented Oct 31, 2023 •

edited

Loading

elle-j Nov 1, 2023 •

edited

Loading

coveralls-official bot commented Nov 2, 2023 •

edited

Loading