RJS-2397: Collections in mixed - synced tests

[papafe]

This PR adds tests for synced collections in mixed

☑️ ToDos

• 🚦 Tests

[kneth]

If not used, it should be safe to remove it

[papafe]

I agree, I'll remove it

[kneth]

Thank you.

Do you plan to add more tests?

[kraenhansen]

If actual is the wrong type (the database is broken), then this might be asserting the wrong thing and we cannot rely on the type of inserted since that's has the unmanaged value 🤔 I believe this was why the test functions were separate. Also it allows for more detailed testing and multiple tests per type.

[papafe]

Makes sense. I'll reinstantiate the local test functions that were used in the tests and keep this for the mixed testing

[kraenhansen]

Did you address this? Perhaps I misunderstood the action you would take, based on my first comment.
The current use of this still feel wrong to me, as the assertion performed depends on the type of the actual, if the implementation is broken to the point where actual has the wrong value, we're not ensured that we'll be choosing the right assertion below.

[papafe]

I think I misunderstood your comment in the beginning
So... I agree with your concern and I agree this is super suboptimal. The reason why it was done like that is that in the tests that were already there the realm gets closed and opened again so if one of the tested values is an object then we can't access it anymore. To be honest I'm not sure how to solve this in a clean way. I'm open for suggestions though 😁
I think we can discuss this on slack

[papafe]

I think I've fixed this. I've made the testing a little bit more complicated, but now the default tester actually uses the inserted value as the truth value

[kneth]

Realm.clearTestState() and Realm.shutdown() are closing all open Realms: https://github.com/realm/realm-js/blob/main/packages/realm/src/Realm.ts#L159-L191

[papafe]

From what I can understand from the body of the function, then just calling Realm.clearTestState would be enough here, right?

If so, maybe it's also not necessary in the hook that's already there?

realm-js/integration-tests/tests/src/hooks/open-realm-before.ts

Lines 31 to 74 in 6269081

	export function openRealmHook(config: OpenRealmConfiguration = {}) {
	return async function openRealmHandler(this: Partial<RealmContext> & UserContext & Mocha.Context): Promise<void> {
	this.longTimeout();
	if (this.realm) {
	throw new Error("Unexpected realm on context, use only one openRealmBefore per test");
	} else {
	const { realm, config: actualConfig } = await openRealm(config, this.user as unknown as User);
	this.realm = realm;
	this.closeRealm = async ({
	clearTestState = true,
	deleteFile = true,
	reopen = false,
	}: Partial<CloseRealmOptions>) => {
	if (this.realm && !this.realm.isClosed) {
	this.realm.close();
	}
	// Get rid of the Realm in any case
	delete this.realm;
	if (deleteFile) {
	Realm.deleteFile(actualConfig);
	}
	if (clearTestState) {
	Realm.clearTestState();
	}
	if (reopen) {
	const { realm } = await openRealm(actualConfig, this.user as unknown as User);
	this.realm = realm;
	}
	};
	}
	};
	}
	/**
	* Close the Realm instance on the current `this` context
	*
	* @param this Mocha `this` context
	*/
	export function closeThisRealm(this: RealmContext & Mocha.Context): void {
	if (this.closeRealm) {
	this.closeRealm({ clearTestState: true, deleteFile: true });
	}
	// Clearing the test state to ensure the sync session gets completely reset and nothing is cached between tests
	Realm.clearTestState();

[papafe]

For me the work on the test is "practically" done. The roundtrip tests are all passing, no issues with those.
The issue is for the "mixed collections" tests, that even though pass when running each on their own, exhibit some strange behaviour when run together, so I need to investigate why we get this flakyness.
But general comments about the tests/structure is appreciated 😁

[papafe]

There is another thread in this PR that is not visible here because it's on an outdated version of this file, regarding if here using clearTestState is enough.

[papafe]

I think I can't use OpenRealmConfiguration here because it would create a circular reference and TS didn't seem to appreciate that

[papafe]

@elle-j @kraenhansen @kneth I've removed all the flaky tests and the changes I've done with those. I will add them in a separate PR, but for now I wanted to have at least the basic tests merged

[elle-j]

Nice to get some roundtrip tests in 🥳 Left some ideas 👍

[elle-j]

Nice 🥳

[kraenhansen]

Since the code infers the expected type from the shape of the inserted POJO, it establishes implicit expectations from the calling code in the behavior of the function. This is especially problematic because inserted is unknown and therefore accepts any values in combination with the final else block, which just compares the string representation. In my opinion it's too easy for the calling code to receive a changed that will render this default tester function useless and as such I fear this will be a source of subtle bugs in the testing code that are hard to discover. Additionally, since this puts restrictions on the shape of the inserted values, it's potentially harder for us to test combinations of these and types we add in the future.

Do you see any way of passing the tester function explicitly from the tests?
Although potentially more verbose.

[papafe]

I understand your concerns.
I suppose it depends what you mean by passing the tester function explicitly from the tests. The methods that are used to define the tests accept a tester function, and this is the one that is used when no function is passed.
If you mean that we should pass a more specific tester function for each test then... We can do it, but it's just much more complicated, because this method not only considers all the possible mixed types (I think at least), but also collections, so it does a recursive comparison if needed.

[kraenhansen]

pass a more specific tester function for each test

Exactly.

does a recursive comparison if needed

Just an idea on the top of my head: We could implement a higher-order function to create dictionary tester functions with a specific tester applied on items.

const createDictionaryTester = (itemTester: ValueTester) => (actual: unknown, inserted: unknown) {
  expectRealmDictionary(actual);
  if (typeof inserted !== "object" || inserted === null) {
    throw new Error("Expected an object of values");
  }
  for (const [insertedKey, insertedValue] of Object.entries(inserted)) {
    itemTester(actual[insertedKey], insertedValue);
  }
}

To test a nested dictionary { foo: { bar: ["baz"] } } this be chained like:

tester: createDictionaryTester(createDictionaryTester(stringTester))

[papafe]

I think there are a couple of things to consider:

• The values that are being used the tests look something like [1, true, "string", ...., object, innerList, innerDicti]. S
  Thus the testing methods wouldn't look particularly pretty.
• It gets even more messy in the other PR with additional tests. In that PR I don't have a single mixed value that I'm trying to roundtrip like here, but I do a lot of modifications to the list/dictionary in order to verify it all works correctly, so I can't have one specific testing function.

Overall I tried to make a testing function that would be generic enough to work with different kind of tests, even though it is not maybe the most specific or strict. I think that if we go the more strict route we either need to simplify the test cases (mostly considering the additional PR) or we need to greatly complicate our tester.

[kraenhansen]

🤔 Yeah. I agree, there are tradeoffs here, which complicates the decision.
I wanted to mention my reservations, but I won't object to this merging as is.