From 010804a917dad5e651cb0fc86e74b4e655798d53 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Tue, 20 Apr 2021 18:18:32 -0700 Subject: [PATCH] simpler test harness --- .../abstract-edge-removal-test.ts | 60 ++-- .../integration/graph/edge-removal/helpers.ts | 271 ++++++++---------- .../integration/graph/edge-removal/setup.ts | 10 +- 3 files changed, 161 insertions(+), 180 deletions(-) diff --git a/packages/record-data/tests/integration/graph/edge-removal/abstract-edge-removal-test.ts b/packages/record-data/tests/integration/graph/edge-removal/abstract-edge-removal-test.ts index 921fe925d16..16c7cd93ebd 100644 --- a/packages/record-data/tests/integration/graph/edge-removal/abstract-edge-removal-test.ts +++ b/packages/record-data/tests/integration/graph/edge-removal/abstract-edge-removal-test.ts @@ -94,8 +94,17 @@ module('Integration | Graph | Nodes', function(hooks) { * * Ergo we expect no entries removed (`removed: false`) and for no caches * to have been deleted (`cleared: false`) + * + * However: for a newly created record any form of rollback, unload or persisted delete + * will result in it being destroyed and cleared */ - await testFinalState(this, testState, config, { removed: false, cleared: false }, assert); + await testFinalState( + this, + testState, + config, + { removed: !!config.useCreate, cleared: !!config.useCreate, implicitCleared: !!config.useCreate }, + assert + ); }); } @@ -124,33 +133,34 @@ module('Integration | Graph | Nodes', function(hooks) { await settled(); /** - * For unload, we treat it as a deletion for sync relationships and + * For unload, we treat it as a persisted deletion for new records and for sync relationships and * as no-change for async relationships. * - * Ergo for async we expect no entries removed (`removed: false`) and for no caches - * to have been deleted (`cleared: false`) - * - * And for sync we expect entries removed (`removed: true`) but for no caches to have been - * deleted. (`cleared: false`) - * - * HOWEVER: for async/sync implicit (`inverseNull`) we expect relationships to be removed but not cleared - * AND for async implicit hasMany relationships we expect the relationships to be cleared as well. + * For local-changes of implicit hasMany relationships we expect the relationships to be cleared as well, + * this special case is handled within ./helpers.ts and is something we can ideally delete as a behavior + * in the future. * - * FURTHER: for newly created records we expect the inverse to be cleaned up (chris) but for the relationships + * For newly created records we expect the inverse to be cleaned up (chris) but for the relationships * for the newly created record to be fully intact. There's no particularly good reason for this other than * we've counted on the record's destroyed state removing these objects from the graph. The inverse relationship * state containers will have removed any retained info about the newly created record. * + * Finally, we expect that even though the relationships on `john` could have been removed in the `sync` case + * that they won't be removed in either case from local and only if from remote if dirtyLocal or useCreate is true. + * The relationships in this case will still be removed from chris. We are possibly retaining these relationships + * despite transitioning the record to an `empty` state in the off chance we need to rematerialize the record. + * Likely for most cases this is just a bug. + * * If this is confusing that's exactly why we've now added this test suite. People depend on this weirdly * observable behavior, so we want to know when it changes. */ - await testFinalState( - this, - testState, - config, - { removed: config.useCreate ? true : !_config.async, cleared: !_config.async && !config.inverseNull }, - assert - ); + + // we remove if the record was new or if the relationship was sync (client side delete semantics) + let removed = config.useCreate || !config.async; + // we clear sync non-implicit relationships (client side delete semantics) + let cleared = !config.async && !config.inverseNull; + + await testFinalState(this, testState, config, { removed, cleared, implicitCleared: true }, assert); }); } @@ -181,20 +191,22 @@ module('Integration | Graph | Nodes', function(hooks) { * For persisted deletions, we expect the cache to have removed all entries for * the deleted record from both explicit and implicit inverses. * - * Ergo we expect entries removed (`removed: true`) and for no caches - * to have been deleted (`cleared: false`) + * Ergo we expect entries removed (`removed: true`) and for all caches + * to have been deleted (`cleared: true`) * - * For unclear reasons, currently async hasMany relationships are emptied - * but not cleared prior to dematerialization after a persisted delete. + * For unclear reasons, currently sync hasMany relationships are emptied + * but not cleared prior to dematerialization after a persisted delete + * only when there is dirty local state. (`cleared: false`) while the + * implicit caches are still cleared. * * This could be either an intentional or unintentional bug caused by the need * to be able to sometimes resurrect a many array during unload. */ let cleared = true; - if (config.relType === 'hasMany' && !config.async) { + if (config.relType === 'hasMany' && !config.async && config.dirtyLocal) { cleared = false; } - await testFinalState(this, testState, config, { removed: true, cleared }, assert); + await testFinalState(this, testState, config, { removed: true, cleared, implicitCleared: true }, assert); }); } diff --git a/packages/record-data/tests/integration/graph/edge-removal/helpers.ts b/packages/record-data/tests/integration/graph/edge-removal/helpers.ts index e9482c9013d..d071cf07c60 100644 --- a/packages/record-data/tests/integration/graph/edge-removal/helpers.ts +++ b/packages/record-data/tests/integration/graph/edge-removal/helpers.ts @@ -59,6 +59,8 @@ interface ExpectedTestOutcomes { removed: boolean; // whether the test expects the relationship cache for `john` to have been cleared cleared: boolean; + // whether the test expects the implicit relationship cache for `john` to have been cleared, defaults to `cleared` + implicitCleared?: boolean; } /** @@ -70,6 +72,8 @@ interface TestState { john: UserRecord; chrisIdentifier: StableRecordIdentifier; johnIdentifier: StableRecordIdentifier; + chrisInverseKey: string; + johnInverseKey: string; } export async function setInitialState(context: Context, config: TestConfig, assert): Promise { @@ -156,53 +160,28 @@ export async function setInitialState(context: Context, config: TestConfig, asse const chrisState = stateOf(chrisBestFriend); const johnState = stateOf(johnBestFriend); - if (!isMany) { - assert.strictEqual( - chrisState.remote, - config.dirtyLocal || config.useCreate ? null : johnIdentifier, - config.dirtyLocal || config.useCreate - ? 'PreCond: Chris has no best friend (remote)' - : 'PreCond: Chris has John as a best friend (remote)' - ); - assert.strictEqual( - chrisState.local, - config.useCreate && config.inverseNull ? null : johnIdentifier, - config.useCreate && config.inverseNull - ? 'PreCond: Chris has no best friend (local)' - : 'PreCond: Chris has John as a best friend (local)' - ); - assert.strictEqual( - johnState.remote, - config.dirtyLocal || config.useCreate ? null : chrisIdentifier, - config.dirtyLocal || config.useCreate - ? 'PreCond: John has no best friend (remote)' - : 'PreCond: John has Chris as a best friend (remote)' - ); - assert.strictEqual(johnState.local, chrisIdentifier, 'PreCond: John has Chris as a best friend (local)'); - } else { - assert.deepEqual( - chrisState.remote, - config.dirtyLocal || config.useCreate ? [] : [johnIdentifier], - config.dirtyLocal || config.useCreate - ? 'PreCond: Chris has no best friend (remote)' - : 'PreCond: Chris has John as a best friend (remote)' - ); - assert.deepEqual( - chrisState.local, - config.useCreate && config.inverseNull ? [] : [johnIdentifier], - config.useCreate && config.inverseNull - ? 'PreCond: Chris has no best friend (local)' - : 'PreCond: Chris has John as a best friend (local)' - ); - assert.deepEqual( - johnState.remote, - config.dirtyLocal || config.useCreate ? [] : [chrisIdentifier], - config.dirtyLocal || config.useCreate - ? 'PreCond: John has no best friend (remote)' - : 'PreCond: John has Chris as a best friend (remote)' - ); - assert.deepEqual(johnState.local, [chrisIdentifier], 'PreCond: John has Chris as a best friend (local)'); - } + assert.deepEqual( + chrisState.remote, + config.dirtyLocal || config.useCreate ? [] : [johnIdentifier], + config.dirtyLocal || config.useCreate + ? 'PreCond: Chris has no best friend (remote)' + : 'PreCond: Chris has John as a best friend (remote)' + ); + assert.deepEqual( + chrisState.local, + config.useCreate && config.inverseNull ? [] : [johnIdentifier], + config.useCreate && config.inverseNull + ? 'PreCond: Chris has no best friend (local)' + : 'PreCond: Chris has John as a best friend (local)' + ); + assert.deepEqual( + johnState.remote, + config.dirtyLocal || config.useCreate ? [] : [chrisIdentifier], + config.dirtyLocal || config.useCreate + ? 'PreCond: John has no best friend (remote)' + : 'PreCond: John has Chris as a best friend (remote)' + ); + assert.deepEqual(johnState.local, [chrisIdentifier], 'PreCond: John has Chris as a best friend (local)'); if (config.inverseNull) { const chrisImplicits = graph.getImplicit(chrisIdentifier); @@ -259,7 +238,14 @@ export async function setInitialState(context: Context, config: TestConfig, asse assert.false(graph.implicit.has(johnIdentifier), 'PreCond: no implicits for john'); } - return { chris, john, chrisIdentifier, johnIdentifier }; + return { + chris, + john, + chrisIdentifier, + johnIdentifier, + chrisInverseKey: chrisBestFriend.inverseKey, + johnInverseKey: johnBestFriend.inverseKey, + }; } export async function testFinalState( @@ -271,139 +257,120 @@ export async function testFinalState( ) { const { graph } = context; const { chrisIdentifier, johnIdentifier } = testState; - const isMany = config.relType === 'hasMany'; const chrisBestFriend = graph.get(chrisIdentifier).get('bestFriends'); const chrisState = stateOf(chrisBestFriend); - /* - with an implicit unload deletion we remove from `chris` (the remaining record) - but don't remove from `john` (the unloaded record). - - This is true for both the implicit and non-implicit relationship cache. - - EXCEPT for has-many implicit we still remove. - - TODO Not removing from `john` / not clearing `john` is likely a bug we should fix. - */ - const isImplicitUnloadDeletion = config.isUnloadAsDelete && !config.async && config.inverseNull && !config.useCreate; - const removeFromJohn = statuses.removed && !isImplicitUnloadDeletion; - const isLocalChangeToImplicitManyArray = - isMany && config.dirtyLocal && config.inverseNull && config.isUnloadAsDelete && !config.useCreate; - const shouldRetainNewUnloadedRelationship = config.useCreate && config.isUnloadAsDelete; - - if (!isMany) { - assert.strictEqual( - chrisState.remote, - isImplicitUnloadDeletion || config.dirtyLocal || config.useCreate || statuses.removed ? null : johnIdentifier, - isImplicitUnloadDeletion || config.dirtyLocal || config.useCreate || statuses.removed - ? 'Result: Chris has no best friend (remote)' - : 'Result: Chris has John as a best friend (remote)' - ); - assert.strictEqual( - chrisState.local, - isImplicitUnloadDeletion || config.useCreate || statuses.removed ? null : johnIdentifier, - isImplicitUnloadDeletion || config.useCreate || statuses.removed - ? 'Result: Chris has no best friend (local)' - : 'Result: Chris has John as a best friend (local)' - ); - } else { - assert.deepEqual( - chrisState.remote, - isImplicitUnloadDeletion || config.dirtyLocal || config.useCreate || statuses.removed ? [] : [johnIdentifier], - isImplicitUnloadDeletion || config.dirtyLocal || config.useCreate || statuses.removed - ? 'Result: Chris has no best friend (remote)' - : 'Result: Chris has John as a best friend (remote)' - ); - assert.deepEqual( - chrisState.local, - isImplicitUnloadDeletion || config.useCreate || statuses.removed ? [] : [johnIdentifier], - isImplicitUnloadDeletion || config.useCreate || statuses.removed - ? 'Result: Chris has no best friend (local)' - : 'Result: Chris has John as a best friend (local)' - ); - } + // this one specific case gets it's own WAT + // this is something ideally a refactor should do away with. + const isUnloadOfImplictAsyncHasManyWithLocalChange = + config.isUnloadAsDelete && config.dirtyLocal && config.async && config.relType === 'hasMany' && config.inverseNull; + + // related to above another WAT that refactor should cleanup + // in this case we don't care if sync/async + const isUnloadOfImplictHasManyWithLocalChange = + config.isUnloadAsDelete && config.dirtyLocal && config.relType === 'hasMany' && config.inverseNull; + + // in the dirtyLocal and useCreate case there is no remote data + const chrisRemoteRemoved = config.dirtyLocal || config.useCreate || statuses.removed; + const chrisLocalRemoved = statuses.removed && !isUnloadOfImplictAsyncHasManyWithLocalChange; + + // for the isUnloadAsDelete case we don't remove unless dirtyLocal or useCreate + // this may be a bug but likely is related to retaining info for rematerialization. + // as the RecordData is in an empty state but not destroyed. + const johnRemoteRemoved = config.dirtyLocal || config.useCreate || (!config.isUnloadAsDelete && statuses.removed); + const johnLocalRemoved = !config.isUnloadAsDelete && statuses.removed; + const johnCleared = statuses.cleared || isUnloadOfImplictHasManyWithLocalChange; + + const _removed = config.isUnloadAsDelete ? statuses.cleared && statuses.removed : statuses.removed; + // in the dirtyLocal and useCreate case there is no remote data + const chrisImplicitRemoteRemoved = config.dirtyLocal || config.useCreate || _removed; + const chrisImplicitLocalRemoved = _removed || isUnloadOfImplictHasManyWithLocalChange; + const johnImplicitsCleared = statuses.implicitCleared || statuses.cleared; + // in the dirtyLocal and useCreate case there is no remote data + const johnImplicitRemoteRemoved = config.dirtyLocal || config.useCreate || statuses.removed; + const johnImplicitLocalRemoved = statuses.removed; + + const OUTCOMES = { + chrisRemoteRemoved, + chrisLocalRemoved, + johnCleared, + johnRemoteRemoved, + johnLocalRemoved, + chrisImplicitRemoteRemoved, + chrisImplicitLocalRemoved, + johnImplicitsCleared, + johnImplicitRemoteRemoved, + johnImplicitLocalRemoved, + }; - // for a newly created record any form of rollback, unload or persisted delete will result in it being destroyed and cleared - // for an existing record, if we expect removal and the relationship is async also expect the relationships cache to have - // been pro-actively cleared. - if (statuses.cleared || (config.useCreate && !config.isUnloadAsDelete)) { + assert.deepEqual( + chrisState.remote, + OUTCOMES.chrisRemoteRemoved ? [] : [johnIdentifier], + OUTCOMES.chrisRemoteRemoved + ? 'Result: Chris has no best friend (remote)' + : 'Result: Chris has John as a best friend (remote)' + ); + assert.deepEqual( + chrisState.local, + OUTCOMES.chrisLocalRemoved ? [] : [johnIdentifier], + OUTCOMES.chrisLocalRemoved + ? 'Result: Chris has no best friend (local)' + : 'Result: Chris has John as a best friend (local)' + ); + + if (OUTCOMES.johnCleared) { assert.false(graph.identifiers.has(johnIdentifier), 'Result: Relationships for John were cleared from the cache'); } else { const johnBestFriend = graph.get(johnIdentifier).get('bestFriends'); const johnState = stateOf(johnBestFriend); - if (!isMany) { - assert.strictEqual( - johnState.remote, - config.dirtyLocal || config.useCreate || removeFromJohn ? null : chrisIdentifier, - config.dirtyLocal || config.useCreate || removeFromJohn - ? 'Result: John has no best friend (remote)' - : 'Result: John has Chris as a best friend (remote)' - ); - assert.strictEqual( - johnState.local, - !shouldRetainNewUnloadedRelationship && removeFromJohn ? null : chrisIdentifier, - !shouldRetainNewUnloadedRelationship && removeFromJohn - ? 'Result: John has no best friend (local)' - : 'Result: John has Chris as a best friend (local)' - ); - } else { - assert.deepEqual( - johnState.remote, - config.dirtyLocal || config.useCreate || removeFromJohn ? [] : [chrisIdentifier], - config.dirtyLocal || config.useCreate || removeFromJohn - ? 'Result: John has no best friend (remote)' - : 'Result: John has Chris as a best friend (remote)' - ); - assert.deepEqual( - johnState.local, - !shouldRetainNewUnloadedRelationship && (isLocalChangeToImplicitManyArray || removeFromJohn) - ? [] - : [chrisIdentifier], - !shouldRetainNewUnloadedRelationship && (isLocalChangeToImplicitManyArray || removeFromJohn) - ? 'Result: John has no best friend (local)' - : 'Result: John has Chris as a best friend (local)' - ); - } + assert.deepEqual( + johnState.remote, + OUTCOMES.johnRemoteRemoved ? [] : [chrisIdentifier], + OUTCOMES.johnRemoteRemoved + ? 'Result: John has no best friend (remote)' + : 'Result: John has Chris as a best friend (remote)' + ); + assert.deepEqual( + johnState.local, + OUTCOMES.johnLocalRemoved ? [] : [chrisIdentifier], + OUTCOMES.johnLocalRemoved + ? 'Result: John has no best friend (local)' + : 'Result: John has Chris as a best friend (local)' + ); } if (config.inverseNull) { - const johnBestFriend = graph.get(johnIdentifier).get('bestFriends'); const chrisImplicits = graph.getImplicit(chrisIdentifier); assert.strictEqual(Object.keys(chrisImplicits).length, 1, 'Result: Chris has one implicit relationship key'); - const chrisImplicitFriend = chrisImplicits[chrisBestFriend.inverseKey] as Relationship; + const chrisImplicitFriend = chrisImplicits[testState.chrisInverseKey] as Relationship; assert.ok(chrisImplicitFriend, 'Result: Chris has an implicit relationship for best friend'); const chrisImplicitState = stateOf(chrisImplicitFriend); - // implicits on chris are managed by john, so removeFromJohn should remove here + assert.deepEqual( chrisImplicitState.remote, - config.dirtyLocal || config.useCreate || removeFromJohn ? [] : [johnIdentifier], - config.dirtyLocal || config.useCreate || removeFromJohn + OUTCOMES.chrisImplicitRemoteRemoved ? [] : [johnIdentifier], + OUTCOMES.chrisImplicitRemoteRemoved ? 'Result: Chris has no implicit best friend (remote)' : 'Result: John implicitly has Chris as a best friend (remote)' ); - // but in the newly created case we don't cleanup this implicit assert.deepEqual( chrisImplicitState.local, - !shouldRetainNewUnloadedRelationship && (config.useCreate || isLocalChangeToImplicitManyArray || removeFromJohn) - ? [] - : [johnIdentifier], - !shouldRetainNewUnloadedRelationship && (config.useCreate || isLocalChangeToImplicitManyArray || removeFromJohn) - ? 'Result: Chris has no implicit best friend (remote)' + OUTCOMES.chrisImplicitLocalRemoved ? [] : [johnIdentifier], + OUTCOMES.chrisImplicitLocalRemoved + ? 'Result: Chris has no implicit best friend (local)' : 'Result: John implicitly has Chris as a best friend (local)' ); - // implicits on john are managed by chris, so with inverseNull and useCreate: true - // the implicit on john will be empty since chris should have no state. - if (config.useCreate || statuses.cleared) { + if (OUTCOMES.johnImplicitsCleared) { assert.false(graph.implicit.has(johnIdentifier), 'implicit cache for john has been removed'); } else { const johnImplicits = graph.getImplicit(johnIdentifier); - const johnImplicitFriend = johnImplicits[johnBestFriend.inverseKey] as Relationship; + const johnImplicitFriend = johnImplicits[testState.johnInverseKey] as Relationship; assert.strictEqual( Object.keys(johnImplicits).length, 1, @@ -414,15 +381,15 @@ export async function testFinalState( assert.deepEqual( johnImplicitState.remote, - isImplicitUnloadDeletion || config.dirtyLocal || config.useCreate || statuses.removed ? [] : [chrisIdentifier], - isImplicitUnloadDeletion || config.dirtyLocal || config.useCreate || statuses.removed + OUTCOMES.johnImplicitRemoteRemoved ? [] : [chrisIdentifier], + OUTCOMES.johnImplicitRemoteRemoved ? 'Result: John has no implicit best friend (remote)' : 'Result: Chris implicitly has John as a best friend (remote)' ); assert.deepEqual( johnImplicitState.local, - isImplicitUnloadDeletion || statuses.removed ? [] : [chrisIdentifier], - isImplicitUnloadDeletion || statuses.removed + OUTCOMES.johnImplicitLocalRemoved ? [] : [chrisIdentifier], + OUTCOMES.johnImplicitLocalRemoved ? 'Result: John has no implicit best friend (local)' : 'Result: Chris implicitly has John as a best friend (local)' ); diff --git a/packages/record-data/tests/integration/graph/edge-removal/setup.ts b/packages/record-data/tests/integration/graph/edge-removal/setup.ts index 0eb1879021f..20ddf6d8cce 100644 --- a/packages/record-data/tests/integration/graph/edge-removal/setup.ts +++ b/packages/record-data/tests/integration/graph/edge-removal/setup.ts @@ -27,6 +27,7 @@ class AbstractMap { has(identifier: StableRecordIdentifier) { let recordData = recordDataFor(identifier) as RecordData; + // debugger; return !!recordData && !!recordData[this.prop]; } } @@ -50,7 +51,7 @@ class AbstractGraph { if (!recordData || !recordData.__relationships) { let relationships = this.cachedRelationships.get(identifier); if (relationships) { - return relationships; + throw new Error(`accessed destroyed relationships within graph`); } } let relationships = recordData._relationships; @@ -64,7 +65,7 @@ class AbstractGraph { if (!recordData || !recordData.__implicitRelationships) { let relationships = this.cachedImplicits.get(identifier); if (relationships) { - return relationships; + throw new Error(`accessed destroyed implicit relationships within graph`); } } let relationships = recordData._implicitRelationships; @@ -84,8 +85,9 @@ function isBelongsTo(rel: Relationship): rel is BelongsToRelationship { export function stateOf(rel: Relationship) { let local, remote; if (isBelongsTo(rel)) { - local = rel.inverseRecordData ? rel.inverseRecordData.identifier : null; - remote = rel.canonicalState ? rel.canonicalState.identifier : null; + // we cast these to array form to make the tests more legible + local = rel.inverseRecordData && rel.inverseRecordData.identifier ? [rel.inverseRecordData.identifier] : []; + remote = rel.canonicalState && rel.canonicalState.identifier ? [rel.canonicalState.identifier] : []; } else { local = rel.members.list.map(m => (m ? m.identifier : null)); remote = rel.canonicalMembers.list.map(m => (m ? m.identifier : null));