Refactor notification / listeners tests #6297

kraenhansen · 2023-12-04T14:13:59Z

What, How & Why?

This is a refactor of our listener / notification tests, adding missing coverage for some of our collection types and (hopefully) increasing the readability of the tests in the process.

☑️ 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

kraenhansen · 2023-12-04T14:15:37Z

integration-tests/tests/src/typings.d.ts

-declare function setTimeout(cb: (args: any[]) => void, timeout: number): any;
+declare type Timer = number;
+declare function setTimeout(cb: (args: any[]) => void, timeout: number): Timer;
+declare function clearTimeout(timer: Timer): void;


I needed to expose the clearTimeout to use it in the "listener stub" util.

coveralls-official · 2023-12-04T14:45:13Z

coverage: 86.51% (+0.3%) from 86.182%
when pulling 6327b3d on kh/observable-tests
into ca6abdd on main.

kraenhansen · 2023-12-04T19:54:29Z

integration-tests/tests/src/tests/observable.ts

+      // Skipping on React Native because the callback is called one too many times
+      it.skipIf(environment.reactNative, "calls listener", async function (this: RealmContext) {


We were already skipping this test on React Native:

realm-js/integration-tests/tests/src/tests/listeners.ts

Lines 44 to 45 in ca6abdd

// TODO: Figure out why the event is fired twice on React Native

it.skipIf(environment.reactNative, "should work for beforenotify", function (this: RealmContext, done) {

I still don't know why this is happening and it would make sense for us to look into it at some point. I've created #6298 to track this.

Does it happens both on Android and iOS? I mean, the event loop is different and I wonder if it is related to core's scheduler or our code?

Yes, it happens on Android and iOS.

takameyer

Amazing stuff here. Really easy to read and follow. Couple comments, but can be disregarded at will.

takameyer · 2023-12-05T08:11:30Z

integration-tests/tests/src/tests/observable.ts

+////////////////////////////////////////////////////////////////////////////
+
+/**
+ * This suite aims to test observability of Realm, Realm.Object and all collection types.


Bit nit picky, but "observable" to is a term from RxJS and means something different when I read that. Without reading this PR, I'm not sure I would deduce that event handlers were being tested here.

Our listeners are loosely modeled over node.js' event emitters

I choose "observable" as a reference to the very general "observer pattern" in software design: The ability to register listeners (i.e. observers) of an object and have notifications trigger on change, which is probably also what RxJS and lots of other reactive / observable libraries are referencing.

takameyer · 2023-12-05T08:18:00Z

integration-tests/tests/src/tests/observable.ts

+
+function expectRealmNotifications(
+  realm: Realm,
+  eventName: "change" | "schema" | "beforenotify",


I was trying to figure out what the difference between "change" and "beforenotify" is and have realized that this isn't documented. Perhaps we could add this while we are at it.

Yeah - it's not ideal. I'll leave that for a follow up PR. To be completely honest, I don't know the intended use-case for it: Perhaps we should actually just break the API and not expose it.

P.S. You could use RealmEventName here, but it doesn't seem to be exported in index.ts. (Noticed that our enum is also not in index.ts. So if we do expose it, we could also use those in place of the hardcoded ones.)

If I'm not mistaken, I think one of the use cases is to react to potential runtime schema change initiated on the backend? We currently simulate this in our tests via realm._updateSchema().

I created #6300.

kneth · 2023-12-05T08:53:06Z

integration-tests/tests/src/utils/listener-stub.ts

+    ...callbacks,
+    (...args: Args) => {
+      last(...args);
+      // Wait just a sec before resolving, in case another callback fires right after


Is 50 equal to "a sec"? Or should the comment be more accurate?

I should probably update that comment :)

kneth · 2023-12-05T09:00:16Z

integration-tests/tests/src/tests/observable.ts

+////////////////////////////////////////////////////////////////////////////
+
+/**
+ * This suite aims to test observability of Realm, Realm.Object and all collection types.


Our listeners are loosely modeled over node.js' event emitters

kneth · 2023-12-05T10:08:22Z

integration-tests/tests/src/tests/observable.ts

+      // Skipping on React Native because the callback is called one too many times
+      it.skipIf(environment.reactNative, "calls listener", async function (this: RealmContext) {


Does it happens both on Android and iOS? I mean, the event loop is different and I wonder if it is related to core's scheduler or our code?

kneth · 2023-12-05T10:10:05Z

integration-tests/tests/src/tests/observable.ts

+
+      this.realm.write(() => {
+        // collection["bob"].name = "Bobby";
+        // TODO: It seems we cannot trigger a notification when a property value changes.


Can we open an issue about it?

Either that or we should clarify what the intended behaviour is. I'll follow up on this 👍

elle-j

Great refactor!

elle-j · 2023-12-05T09:28:45Z

integration-tests/tests/src/tests/observable.ts

+import { createListenerStub } from "../utils/listener-stub";
+import { createPromiseHandle } from "../utils/promise-handle";
+
+type Observable = Realm | Realm.Object<any> | Realm.Results<any> | Realm.List<any> | Realm.Dictionary | Realm.Set<any>;


Do you see value in taking a similar approach in the sync directory? E.g. for App, User, and SyncSession.

I didn't think of those, but yes - that would be great!

elle-j · 2023-12-05T10:02:07Z

integration-tests/tests/src/tests/observable.ts

+      ...expectedChangeSets.map(
+        (expectedChanges, c) => (realm: Realm, name: string, schema?: Realm.CanonicalObjectSchema[]) => {
+          expect(name).equals(eventName, `Realm change event #${c} name didn't match`);
+          expect(schema).deep.equals(expectedChanges.schema, `Realm change event #${c} schema didn't match`);


The naming of c initially threw me off a bit, thinking it may be misused as I didn't associate the name to an index. But what do you think of moving the part within map(..) to another function to speed up readability? Perhaps something like

...expectedChangeSets.map(createRealmListenerCallback)

If so, then it could also apply to the other expectXNotifications functions.

I tend do gravitate towards inline arrow functions for simple mappings / transformations like these, especially if they're not reused.

elle-j · 2023-12-05T10:25:37Z

integration-tests/tests/src/tests/observable.ts

+
+function expectRealmNotifications(
+  realm: Realm,
+  eventName: "change" | "schema" | "beforenotify",


P.S. You could use RealmEventName here, but it doesn't seem to be exported in index.ts. (Noticed that our enum is also not in index.ts. So if we do expose it, we could also use those in place of the hardcoded ones.)

If I'm not mistaken, I think one of the use cases is to react to potential runtime schema change initiated on the backend? We currently simulate this in our tests via realm._updateSchema().

elle-j · 2023-12-05T10:27:50Z

integration-tests/tests/src/tests/observable.ts

+  // describe("App", () => {});
+


Suggested change

// describe("App", () => {});

elle-j · 2023-12-05T10:31:24Z

integration-tests/tests/src/tests/observable.ts

+          this.realm.create("Person", { name: "Alice" });
+        });
+
+        await completion;


elle-j · 2023-12-05T10:37:12Z

integration-tests/tests/src/tests/observable.ts

+      it("removes listeners", async function (this: RealmContext) {
+        const handle = createPromiseHandle();
+
+        const listener = createListenerStub(handle, () => {
+          this.realm.removeListener("change", listener);
+          setImmediate(() => {
+            this.realm.write(() => {
+              this.realm.create("Person", { name: "Bob" });
+            });
+          });
+        });
+
+        this.realm.addListener("change", listener);
+
+        this.realm.write(() => {
+          this.realm.create("Person", { name: "Alice" });
+        });
+
+        await handle;
+      });


I like the pattern taken with the expectXNotifications functions. Could something similar be applied to the removal tests for expecting the removal?

I agree that the await expectXNotifications reads really good, but I hadn't found a good abstraction for this.
I gave it another go: #6301

elle-j · 2023-12-05T10:59:27Z

integration-tests/tests/src/utils/sequence.ts

+    if (i >= callbacks.length) {
+      throw new Error(`Sequence was called more than ${callbacks.length} times (call #${i})`);


Nit: Maybe either Sequence was called ${callbacks.length} or more times (call #${i}) or Sequence was called more than ${callbacks.length} times (call #${i + 1}) is more correct?

* Observable tests scaffold * Default T for Realm.Dictionary * WIP * WIP * Deleted existing notification tests * Adding documentation to the test utils * Removed import of missing tests * Removed unwanted comments * Skipping "beforenotify" test on React Native * Update listener-stub.ts

kraenhansen self-assigned this Dec 4, 2023

kraenhansen added 5 commits December 4, 2023 15:14

Observable tests scaffold

84e40a0

Default T for Realm.Dictionary

76c76bf

WIP

9a31f1e

WIP

d86fdb2

Deleted existing notification tests

a98b091

kraenhansen force-pushed the kh/observable-tests branch from 94f6910 to 2802530 Compare December 4, 2023 14:14

kraenhansen commented Dec 4, 2023

View reviewed changes

kraenhansen changed the title ~~Refactore notification / listeners tests~~ Refactor notification / listeners tests Dec 4, 2023

kraenhansen requested review from elle-j and takameyer December 4, 2023 19:50

kraenhansen commented Dec 4, 2023

View reviewed changes

kraenhansen added 4 commits December 5, 2023 09:18

Adding documentation to the test utils

eb0d4bf

Removed import of missing tests

73b3051

Removed unwanted comments

e18bbc7

Skipping "beforenotify" test on React Native

79578fa

kraenhansen force-pushed the kh/observable-tests branch from 5e78db6 to 79578fa Compare December 5, 2023 08:18

kraenhansen requested a review from kneth December 5, 2023 08:19

takameyer approved these changes Dec 5, 2023

View reviewed changes

kneth approved these changes Dec 5, 2023

View reviewed changes

Update listener-stub.ts

6327b3d

kraenhansen merged commit 39d441c into main Dec 5, 2023
23 checks passed

kraenhansen deleted the kh/observable-tests branch December 5, 2023 10:56

elle-j approved these changes Dec 5, 2023

View reviewed changes

This was referenced Dec 5, 2023

Export RealmEventName #6300

Merged

Introduced expectListenerRemoval #6301

Merged

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

Refactor notification / listeners tests #6297

Refactor notification / listeners tests #6297

kraenhansen commented Dec 4, 2023

kraenhansen Dec 4, 2023

coveralls-official bot commented Dec 4, 2023 •

edited

Loading

kraenhansen Dec 4, 2023

kneth Dec 5, 2023

kraenhansen Dec 5, 2023

takameyer left a comment

takameyer Dec 5, 2023

kneth Dec 5, 2023

kraenhansen Dec 5, 2023

takameyer Dec 5, 2023

kraenhansen Dec 5, 2023 •

edited

Loading

elle-j Dec 5, 2023

kraenhansen Dec 5, 2023

kneth Dec 5, 2023

kraenhansen Dec 5, 2023

kneth Dec 5, 2023

kneth Dec 5, 2023

kneth Dec 5, 2023

kraenhansen Dec 5, 2023

elle-j left a comment

elle-j Dec 5, 2023

kraenhansen Dec 5, 2023

elle-j Dec 5, 2023 •

edited

Loading

kraenhansen Dec 5, 2023

elle-j Dec 5, 2023

elle-j Dec 5, 2023

elle-j Dec 5, 2023

elle-j Dec 5, 2023

kraenhansen Dec 5, 2023

elle-j Dec 5, 2023

		// Skipping on React Native because the callback is called one too many times
		it.skipIf(environment.reactNative, "calls listener", async function (this: RealmContext) {

	// TODO: Figure out why the event is fired twice on React Native
	it.skipIf(environment.reactNative, "should work for beforenotify", function (this: RealmContext, done) {

		if (i >= callbacks.length) {
		throw new Error(`Sequence was called more than ${callbacks.length} times (call #${i})`);

Refactor notification / listeners tests #6297

Refactor notification / listeners tests #6297

Conversation

kraenhansen commented Dec 4, 2023

What, How & Why?

☑️ ToDos

Choose a reason for hiding this comment

coveralls-official bot commented Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takameyer 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

kraenhansen Dec 5, 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

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

elle-j Dec 5, 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

coveralls-official bot commented Dec 4, 2023 •

edited

Loading

kraenhansen Dec 5, 2023 •

edited

Loading

elle-j Dec 5, 2023 •

edited

Loading