From e02139b956a26bb1362be8ad965a9238fe532035 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 24 May 2023 16:41:14 -0400 Subject: [PATCH 01/34] Add a test for testing without mocks Testing without mocks allows us to look at the code how it is functioning right now so that we can evaluate the intended behavior. --- test/no-mocks.ts | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 test/no-mocks.ts diff --git a/test/no-mocks.ts b/test/no-mocks.ts new file mode 100644 index 000000000..a74ea4958 --- /dev/null +++ b/test/no-mocks.ts @@ -0,0 +1,64 @@ +import {describe, it} from 'mocha'; +import {RequestConfig} from '../src/request'; +import * as assert from 'assert'; +import {Datastore} from '../src'; + +describe('NoMocks', () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + + it.only('should encode a request without excludeFromIndexes', done => { + const namespace = datastore.namespace; + const expectedConfig = { + client: 'DatastoreClient', + method: 'commit', + // gaxOpts: {}, + reqOpts: { + mutations: [ + { + upsert: { + key: { + path: [{kind: 'Post', name: 'Post1'}], + partitionId: { + namespaceId: namespace, + }, + }, + properties: {}, + }, + }, + ], + }, + }; + datastore.request_ = (config: RequestConfig) => { + try { + assert.deepStrictEqual(config, expectedConfig); + } catch (e) { + console.log(e); + assert.fail('assertion failed'); + } + done(); + }; + const key = datastore.key(['Post', 'Post1']); + datastore.save({ + key, + data: {}, + }); + }); + + it('should ignore non-existent property in excludeFromIndexes', done => { + datastore.request_ = (config: RequestConfig) => { + console.log(config); + done(); + }; + const key = datastore.key(['Post', 'Post1']); + datastore.save({ + key, + data: {}, + excludeFromIndexes: [ + 'non_exist_property', // this just ignored + 'non_exist_property.*', // should also be ignored + ], + }); + }); +}); From f139770f865f82e20022ce8c4c4c4b98a899f8bb Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 24 May 2023 16:50:26 -0400 Subject: [PATCH 02/34] Do a refactor on the test Objects shared by both tests should be moved into the outer block so that they can be shared. --- test/no-mocks.ts | 60 ++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/test/no-mocks.ts b/test/no-mocks.ts index a74ea4958..53dcecca5 100644 --- a/test/no-mocks.ts +++ b/test/no-mocks.ts @@ -4,16 +4,15 @@ import * as assert from 'assert'; import {Datastore} from '../src'; describe('NoMocks', () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - - it.only('should encode a request without excludeFromIndexes', done => { + describe('using save to evaluate excludeFromIndexes', () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); const namespace = datastore.namespace; const expectedConfig = { client: 'DatastoreClient', method: 'commit', - // gaxOpts: {}, + gaxOpts: {}, reqOpts: { mutations: [ { @@ -30,35 +29,40 @@ describe('NoMocks', () => { ], }, }; - datastore.request_ = (config: RequestConfig) => { + function evaluateConfig(config: RequestConfig) { try { assert.deepStrictEqual(config, expectedConfig); } catch (e) { console.log(e); assert.fail('assertion failed'); } - done(); - }; - const key = datastore.key(['Post', 'Post1']); - datastore.save({ - key, - data: {}, - }); - }); + } - it('should ignore non-existent property in excludeFromIndexes', done => { - datastore.request_ = (config: RequestConfig) => { - console.log(config); - done(); - }; - const key = datastore.key(['Post', 'Post1']); - datastore.save({ - key, - data: {}, - excludeFromIndexes: [ - 'non_exist_property', // this just ignored - 'non_exist_property.*', // should also be ignored - ], + it.only('should encode a request without excludeFromIndexes', done => { + datastore.request_ = (config: RequestConfig) => { + evaluateConfig(config); + done(); + }; + const key = datastore.key(['Post', 'Post1']); + datastore.save({ + key, + data: {}, + }); + }); + it('should ignore non-existent property in excludeFromIndexes', done => { + datastore.request_ = (config: RequestConfig) => { + evaluateConfig(config); + done(); + }; + const key = datastore.key(['Post', 'Post1']); + datastore.save({ + key, + data: {}, + excludeFromIndexes: [ + 'non_exist_property', // this just ignored + 'non_exist_property.*', // should also be ignored + ], + }); }); }); }); From c8dd740e986b165891f8a3e6fedc73ce5681394e Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 24 May 2023 17:11:49 -0400 Subject: [PATCH 03/34] improve the test so that the error bubbles up Now the test fails properly and passes properly by introducing callbacks --- test/no-mocks.ts | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/test/no-mocks.ts b/test/no-mocks.ts index 53dcecca5..41f547bdd 100644 --- a/test/no-mocks.ts +++ b/test/no-mocks.ts @@ -1,5 +1,5 @@ import {describe, it} from 'mocha'; -import {RequestConfig} from '../src/request'; +import {RequestCallback, RequestConfig} from '../src/request'; import * as assert from 'assert'; import {Datastore} from '../src'; @@ -29,33 +29,24 @@ describe('NoMocks', () => { ], }, }; - function evaluateConfig(config: RequestConfig) { + datastore.request_ = (config: RequestConfig, callback: RequestCallback) => { try { assert.deepStrictEqual(config, expectedConfig); - } catch (e) { - console.log(e); - assert.fail('assertion failed'); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); } - } - - it.only('should encode a request without excludeFromIndexes', done => { - datastore.request_ = (config: RequestConfig) => { - evaluateConfig(config); - done(); - }; - const key = datastore.key(['Post', 'Post1']); - datastore.save({ + }; + const key = datastore.key(['Post', 'Post1']); + it.only('should encode a request without excludeFromIndexes', async () => { + const results = await datastore.save({ key, data: {}, }); + assert.deepStrictEqual(results, ['some-data']); }); - it('should ignore non-existent property in excludeFromIndexes', done => { - datastore.request_ = (config: RequestConfig) => { - evaluateConfig(config); - done(); - }; - const key = datastore.key(['Post', 'Post1']); - datastore.save({ + it('should ignore non-existent property in excludeFromIndexes', async () => { + const results = await datastore.save({ key, data: {}, excludeFromIndexes: [ @@ -63,6 +54,7 @@ describe('NoMocks', () => { 'non_exist_property.*', // should also be ignored ], }); + assert.deepStrictEqual(results, ['some-data']); }); }); }); From bdbe4c71ed3003d426d84f11675974f6fb65e560 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 24 May 2023 17:17:38 -0400 Subject: [PATCH 04/34] isolate the latter test right now The failure should be visible to the user and in the current state it is so now by isolating the test it becomes easier to explore solutions that work. --- test/no-mocks.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/no-mocks.ts b/test/no-mocks.ts index 41f547bdd..a55b8aafc 100644 --- a/test/no-mocks.ts +++ b/test/no-mocks.ts @@ -38,14 +38,14 @@ describe('NoMocks', () => { } }; const key = datastore.key(['Post', 'Post1']); - it.only('should encode a request without excludeFromIndexes', async () => { + it('should encode a request without excludeFromIndexes', async () => { const results = await datastore.save({ key, data: {}, }); assert.deepStrictEqual(results, ['some-data']); }); - it('should ignore non-existent property in excludeFromIndexes', async () => { + it.only('should ignore non-existent property in excludeFromIndexes', async () => { const results = await datastore.save({ key, data: {}, From 24975b1d75d4ccf1a63b9a2697c387145f00e364 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 26 May 2023 10:42:53 -0400 Subject: [PATCH 05/34] test fixes indent a describe block, modify tests to make sure they pass and refactor a function out that easily allows for testing modified parameters. --- src/entity.ts | 32 +++++----- test/no-mocks.ts | 148 +++++++++++++++++++++++++++++++++-------------- 2 files changed, 122 insertions(+), 58 deletions(-) diff --git a/src/entity.ts b/src/entity.ts index 3d43406a9..da71c25f3 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -898,22 +898,26 @@ export namespace entity { excludePathFromEntity(entity, newPath); }); } else { - if (hasWildCard && remainderPath === '*') { - const parentEntity = entity.properties![firstPathPart].entityValue; - - if (parentEntity) { - Object.keys(parentEntity.properties).forEach(path => { - const newPath = parentEntity.properties[path].arrayValue - ? path + '[].*' - : path + '.*'; - excludePathFromEntity(parentEntity, newPath); - }); + if (entity.properties![firstPathPart]) { + if (hasWildCard && remainderPath === '*') { + const parentEntity = + entity.properties![firstPathPart].entityValue; + + if (parentEntity) { + Object.keys(parentEntity.properties).forEach(path => { + const newPath = parentEntity.properties[path].arrayValue + ? path + '[].*' + : path + '.*'; + excludePathFromEntity(parentEntity, newPath); + }); + } else { + excludePathFromEntity(entity, firstPathPart); + } } else { - excludePathFromEntity(entity, firstPathPart); + const parentEntity = + entity.properties![firstPathPart].entityValue; + excludePathFromEntity(parentEntity, remainderPath); } - } else { - const parentEntity = entity.properties![firstPathPart].entityValue; - excludePathFromEntity(parentEntity, remainderPath); } } } diff --git a/test/no-mocks.ts b/test/no-mocks.ts index a55b8aafc..4b3c15c4b 100644 --- a/test/no-mocks.ts +++ b/test/no-mocks.ts @@ -5,56 +5,116 @@ import {Datastore} from '../src'; describe('NoMocks', () => { describe('using save to evaluate excludeFromIndexes', () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const expectedConfig = { - client: 'DatastoreClient', - method: 'commit', - gaxOpts: {}, - reqOpts: { - mutations: [ - { - upsert: { - key: { - path: [{kind: 'Post', name: 'Post1'}], - partitionId: { - namespaceId: namespace, + function getExpectedConfig(properties: any, namespace: string | undefined) { + return { + client: 'DatastoreClient', + method: 'commit', + gaxOpts: {}, + reqOpts: { + mutations: [ + { + upsert: { + key: { + path: [{kind: 'Post', name: 'Post1'}], + partitionId: { + namespaceId: namespace, + }, }, + properties, }, - properties: {}, }, - }, - ], - }, - }; - datastore.request_ = (config: RequestConfig, callback: RequestCallback) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const key = datastore.key(['Post', 'Post1']); - it('should encode a request without excludeFromIndexes', async () => { - const results = await datastore.save({ - key, - data: {}, + ], + }, + }; + } + describe('when the property is contained in excludeFromIndexes', () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); + it('should encode a request without excludeFromIndexes', async () => { + const expectedConfig = getExpectedConfig( + {k: {stringValue: 'v'}}, + namespace + ); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save({ + key, + data: {k: 'v'}, + }); + assert.deepStrictEqual(results, ['some-data']); + }); + it('should ignore non-existent property in excludeFromIndexes', async () => { + const expectedConfig = getExpectedConfig( + {k: {stringValue: 'v', excludeFromIndexes: true}}, + namespace + ); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save({ + key, + data: {k: 'v'}, + excludeFromIndexes: ['k', 'k.*'], + }); + assert.deepStrictEqual(results, ['some-data']); }); - assert.deepStrictEqual(results, ['some-data']); }); - it.only('should ignore non-existent property in excludeFromIndexes', async () => { - const results = await datastore.save({ - key, - data: {}, - excludeFromIndexes: [ - 'non_exist_property', // this just ignored - 'non_exist_property.*', // should also be ignored - ], + describe('when the property is not contained in excludeFromIndexes', () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const expectedConfig = getExpectedConfig({}, namespace); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const key = datastore.key(['Post', 'Post1']); + it('should encode a request without excludeFromIndexes', async () => { + const results = await datastore.save({ + key, + data: {}, + }); + assert.deepStrictEqual(results, ['some-data']); + }); + it('should ignore non-existent property in excludeFromIndexes', async () => { + const results = await datastore.save({ + key, + data: {}, + excludeFromIndexes: [ + 'non_exist_property', // this just ignored + 'non_exist_property.*', // should also be ignored + ], + }); + assert.deepStrictEqual(results, ['some-data']); }); - assert.deepStrictEqual(results, ['some-data']); }); }); }); From d129b17ccc9a7c731446dc027e07dcd9d4508a86 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 26 May 2023 11:35:14 -0400 Subject: [PATCH 06/34] Add two more tests for excludeFromIndexes on array Add two more tests that address the array case for excludeIndexesFromPath. Also ensure that the conditions which check for these test cases properly ignore exclude from indexes when an array is used --- src/entity.ts | 10 +++++++-- test/no-mocks.ts | 57 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/src/entity.ts b/src/entity.ts index da71c25f3..45150efe6 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -855,6 +855,7 @@ export namespace entity { if ( firstPathPartIsArray && // check also if the property in question is actually an array value. + typeof entity.properties![firstPathPart] !== 'undefined' && entity.properties![firstPathPart].arrayValue && // check if wildcard is not applied !hasWildCard @@ -879,7 +880,12 @@ export namespace entity { ); } }); - } else if (firstPathPartIsArray && hasWildCard && remainderPath === '*') { + } else if ( + firstPathPartIsArray && + hasWildCard && + remainderPath === '*' && + typeof entity.properties![firstPathPart] !== 'undefined' + ) { const array = entity.properties![firstPathPart].arrayValue; // eslint-disable-next-line @typescript-eslint/no-explicit-any array.values.forEach((value: any) => { @@ -898,7 +904,7 @@ export namespace entity { excludePathFromEntity(entity, newPath); }); } else { - if (entity.properties![firstPathPart]) { + if (typeof entity.properties![firstPathPart] !== 'undefined') { if (hasWildCard && remainderPath === '*') { const parentEntity = entity.properties![firstPathPart].entityValue; diff --git a/test/no-mocks.ts b/test/no-mocks.ts index 4b3c15c4b..0e10fa44d 100644 --- a/test/no-mocks.ts +++ b/test/no-mocks.ts @@ -116,5 +116,62 @@ describe('NoMocks', () => { assert.deepStrictEqual(results, ['some-data']); }); }); + describe('when the property is not contained in excludeFromIndexes and the property is an array', () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); + it('should encode a request when there is no wildcard', async () => { + const expectedConfig = getExpectedConfig( + {k: {stringValue: 'v'}}, + namespace + ); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save({ + key, + data: {k: 'v'}, + excludeFromIndexes: [ + 'non_exist_property[]', // this just ignored + ], + }); + assert.deepStrictEqual(results, ['some-data']); + }); + it('should encode a request when using a wildcard', async () => { + const expectedConfig = getExpectedConfig( + {k: {stringValue: 'v'}}, + namespace + ); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save({ + key, + data: {k: 'v'}, + excludeFromIndexes: [ + 'non_exist_property[].*', // this just ignored + ], + }); + assert.deepStrictEqual(results, ['some-data']); + }); + }); }); }); From 860afcb8bbbda0c5cc215311759929a9a1c9e671 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 26 May 2023 11:59:26 -0400 Subject: [PATCH 07/34] Move the tests over to the index file We should not create a nomocks file because we can just group the tests under a unique describe block of test/index.ts. --- test/index.ts | 182 ++++++++++++++++++++++++++++++++++++++++++++++- test/no-mocks.ts | 177 --------------------------------------------- 2 files changed, 180 insertions(+), 179 deletions(-) delete mode 100644 test/no-mocks.ts diff --git a/test/index.ts b/test/index.ts index 23e61148d..c4fe9f44b 100644 --- a/test/index.ts +++ b/test/index.ts @@ -19,9 +19,10 @@ import * as proxyquire from 'proxyquire'; import {PassThrough, Readable} from 'stream'; import * as ds from '../src'; -import {DatastoreOptions} from '../src'; +import {Datastore, DatastoreOptions} from '../src'; +import {Datastore as OriginalDatastore} from '../src'; import {entity, Entity, EntityProto, EntityObject} from '../src/entity'; -import {RequestConfig} from '../src/request'; +import {RequestCallback, RequestConfig} from '../src/request'; import * as is from 'is'; import * as sinon from 'sinon'; import * as extend from 'extend'; @@ -2150,4 +2151,181 @@ describe('Datastore', () => { assert.strictEqual(key.name, 'Test'); }); }); + + describe('Without using mocks', () => { + const Datastore = OriginalDatastore; + describe('using save to evaluate excludeFromIndexes', () => { + function getExpectedConfig( + properties: any, + namespace: string | undefined + ) { + return { + client: 'DatastoreClient', + method: 'commit', + gaxOpts: {}, + reqOpts: { + mutations: [ + { + upsert: { + key: { + path: [{kind: 'Post', name: 'Post1'}], + partitionId: { + namespaceId: namespace, + }, + }, + properties, + }, + }, + ], + }, + }; + } + describe('when the property is contained in excludeFromIndexes', () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); + it('should encode a request without excludeFromIndexes', async () => { + const expectedConfig = getExpectedConfig( + {k: {stringValue: 'v'}}, + namespace + ); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save({ + key, + data: {k: 'v'}, + }); + assert.deepStrictEqual(results, ['some-data']); + }); + it('should ignore non-existent property in excludeFromIndexes', async () => { + const expectedConfig = getExpectedConfig( + {k: {stringValue: 'v', excludeFromIndexes: true}}, + namespace + ); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save({ + key, + data: {k: 'v'}, + excludeFromIndexes: ['k', 'k.*'], + }); + assert.deepStrictEqual(results, ['some-data']); + }); + }); + describe('when the property is not contained in excludeFromIndexes', () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const expectedConfig = getExpectedConfig({}, namespace); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const key = datastore.key(['Post', 'Post1']); + it('should encode a request without excludeFromIndexes', async () => { + const results = await datastore.save({ + key, + data: {}, + }); + assert.deepStrictEqual(results, ['some-data']); + }); + it('should ignore non-existent property in excludeFromIndexes', async () => { + const results = await datastore.save({ + key, + data: {}, + excludeFromIndexes: [ + 'non_exist_property', // this just ignored + 'non_exist_property.*', // should also be ignored + ], + }); + assert.deepStrictEqual(results, ['some-data']); + }); + }); + describe('when the property is not contained in excludeFromIndexes and the property is an array', () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); + it('should encode a request when there is no wildcard', async () => { + const expectedConfig = getExpectedConfig( + {k: {stringValue: 'v'}}, + namespace + ); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save({ + key, + data: {k: 'v'}, + excludeFromIndexes: [ + 'non_exist_property[]', // this just ignored + ], + }); + assert.deepStrictEqual(results, ['some-data']); + }); + it('should encode a request when using a wildcard', async () => { + const expectedConfig = getExpectedConfig( + {k: {stringValue: 'v'}}, + namespace + ); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save({ + key, + data: {k: 'v'}, + excludeFromIndexes: [ + 'non_exist_property[].*', // this just ignored + ], + }); + assert.deepStrictEqual(results, ['some-data']); + }); + }); + }); + }); }); diff --git a/test/no-mocks.ts b/test/no-mocks.ts deleted file mode 100644 index 0e10fa44d..000000000 --- a/test/no-mocks.ts +++ /dev/null @@ -1,177 +0,0 @@ -import {describe, it} from 'mocha'; -import {RequestCallback, RequestConfig} from '../src/request'; -import * as assert from 'assert'; -import {Datastore} from '../src'; - -describe('NoMocks', () => { - describe('using save to evaluate excludeFromIndexes', () => { - function getExpectedConfig(properties: any, namespace: string | undefined) { - return { - client: 'DatastoreClient', - method: 'commit', - gaxOpts: {}, - reqOpts: { - mutations: [ - { - upsert: { - key: { - path: [{kind: 'Post', name: 'Post1'}], - partitionId: { - namespaceId: namespace, - }, - }, - properties, - }, - }, - ], - }, - }; - } - describe('when the property is contained in excludeFromIndexes', () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - it('should encode a request without excludeFromIndexes', async () => { - const expectedConfig = getExpectedConfig( - {k: {stringValue: 'v'}}, - namespace - ); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save({ - key, - data: {k: 'v'}, - }); - assert.deepStrictEqual(results, ['some-data']); - }); - it('should ignore non-existent property in excludeFromIndexes', async () => { - const expectedConfig = getExpectedConfig( - {k: {stringValue: 'v', excludeFromIndexes: true}}, - namespace - ); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save({ - key, - data: {k: 'v'}, - excludeFromIndexes: ['k', 'k.*'], - }); - assert.deepStrictEqual(results, ['some-data']); - }); - }); - describe('when the property is not contained in excludeFromIndexes', () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const expectedConfig = getExpectedConfig({}, namespace); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const key = datastore.key(['Post', 'Post1']); - it('should encode a request without excludeFromIndexes', async () => { - const results = await datastore.save({ - key, - data: {}, - }); - assert.deepStrictEqual(results, ['some-data']); - }); - it('should ignore non-existent property in excludeFromIndexes', async () => { - const results = await datastore.save({ - key, - data: {}, - excludeFromIndexes: [ - 'non_exist_property', // this just ignored - 'non_exist_property.*', // should also be ignored - ], - }); - assert.deepStrictEqual(results, ['some-data']); - }); - }); - describe('when the property is not contained in excludeFromIndexes and the property is an array', () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - it('should encode a request when there is no wildcard', async () => { - const expectedConfig = getExpectedConfig( - {k: {stringValue: 'v'}}, - namespace - ); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save({ - key, - data: {k: 'v'}, - excludeFromIndexes: [ - 'non_exist_property[]', // this just ignored - ], - }); - assert.deepStrictEqual(results, ['some-data']); - }); - it('should encode a request when using a wildcard', async () => { - const expectedConfig = getExpectedConfig( - {k: {stringValue: 'v'}}, - namespace - ); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save({ - key, - data: {k: 'v'}, - excludeFromIndexes: [ - 'non_exist_property[].*', // this just ignored - ], - }); - assert.deepStrictEqual(results, ['some-data']); - }); - }); - }); -}); From be6b20e0c4a5fe78368f53c5bcd7d6005c858856 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 10:29:00 -0400 Subject: [PATCH 08/34] PR update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove typeof and remove check for string ‘undefined’ --- src/entity.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/entity.ts b/src/entity.ts index 45150efe6..6ee99f4a0 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -855,7 +855,7 @@ export namespace entity { if ( firstPathPartIsArray && // check also if the property in question is actually an array value. - typeof entity.properties![firstPathPart] !== 'undefined' && + entity.properties![firstPathPart] !== undefined && entity.properties![firstPathPart].arrayValue && // check if wildcard is not applied !hasWildCard From f0900e6c2380ae1b23089e42b84ce3d318614414 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 11:14:19 -0400 Subject: [PATCH 09/34] Create a function to refactor the tests Refactor the tests so that they are more readable. There is a lot of repeated code in each test which makes comparisons hard. --- test/index.ts | 113 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 73 insertions(+), 40 deletions(-) diff --git a/test/index.ts b/test/index.ts index 489159cac..225869e60 100644 --- a/test/index.ts +++ b/test/index.ts @@ -21,7 +21,7 @@ import {PassThrough, Readable} from 'stream'; import * as ds from '../src'; import {Datastore, DatastoreOptions} from '../src'; import {Datastore as OriginalDatastore} from '../src'; -import {entity, Entity, EntityProto, EntityObject} from '../src/entity'; +import {entity, Entity, EntityProto, EntityObject, Entities} from '../src/entity'; import {RequestCallback, RequestConfig} from '../src/request'; import * as is from 'is'; import * as sinon from 'sinon'; @@ -2180,15 +2180,48 @@ describe('Datastore', () => { }, }; } - describe('when the property is contained in excludeFromIndexes', () => { + + async function runExcludeFromIndexesTest( + properties: any, + entitiesWithoutKey: any + ) { const datastore = new Datastore({ namespace: `${Date.now()}`, }); const namespace = datastore.namespace; const key = datastore.key(['Post', 'Post1']); + const entities = Object.assign({key}, entitiesWithoutKey); + const expectedConfig = getExpectedConfig(properties, namespace); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save(entities); + assert.deepStrictEqual(results, ['some-data']); + } + describe('when the property is contained in excludeFromIndexes', () => { it('should encode a request without excludeFromIndexes', async () => { + const properties = {k: {stringValue: 'v'}}; + const entitiesWithoutKey = { + data: {k: 'v'}, + }; + await runExcludeFromIndexesTest(properties, entitiesWithoutKey); + }); + it('should ignore non-existent property in excludeFromIndexes', async () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); const expectedConfig = getExpectedConfig( - {k: {stringValue: 'v'}}, + {k: {stringValue: 'v', excludeFromIndexes: true}}, namespace ); datastore.request_ = ( @@ -2205,14 +2238,19 @@ describe('Datastore', () => { const results = await datastore.save({ key, data: {k: 'v'}, + excludeFromIndexes: ['k', 'k.*'], }); assert.deepStrictEqual(results, ['some-data']); }); - it('should ignore non-existent property in excludeFromIndexes', async () => { - const expectedConfig = getExpectedConfig( - {k: {stringValue: 'v', excludeFromIndexes: true}}, - namespace - ); + }); + describe('when the property is not contained in excludeFromIndexes', () => { + it('should encode a request without excludeFromIndexes', async () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); + const expectedConfig = getExpectedConfig({}, namespace); datastore.request_ = ( config: RequestConfig, callback: RequestCallback @@ -2224,33 +2262,6 @@ describe('Datastore', () => { callback(e); } }; - const results = await datastore.save({ - key, - data: {k: 'v'}, - excludeFromIndexes: ['k', 'k.*'], - }); - assert.deepStrictEqual(results, ['some-data']); - }); - }); - describe('when the property is not contained in excludeFromIndexes', () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const expectedConfig = getExpectedConfig({}, namespace); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const key = datastore.key(['Post', 'Post1']); - it('should encode a request without excludeFromIndexes', async () => { const results = await datastore.save({ key, data: {}, @@ -2258,6 +2269,23 @@ describe('Datastore', () => { assert.deepStrictEqual(results, ['some-data']); }); it('should ignore non-existent property in excludeFromIndexes', async () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); + const expectedConfig = getExpectedConfig({}, namespace); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; const results = await datastore.save({ key, data: {}, @@ -2270,12 +2298,12 @@ describe('Datastore', () => { }); }); describe('when the property is not contained in excludeFromIndexes and the property is an array', () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); it('should encode a request when there is no wildcard', async () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); const expectedConfig = getExpectedConfig( {k: {stringValue: 'v'}}, namespace @@ -2301,6 +2329,11 @@ describe('Datastore', () => { assert.deepStrictEqual(results, ['some-data']); }); it('should encode a request when using a wildcard', async () => { + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); const expectedConfig = getExpectedConfig( {k: {stringValue: 'v'}}, namespace From 3e1fa905b6fbef351d0c6fa6ec17a45f5434aa2e Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 11:28:03 -0400 Subject: [PATCH 10/34] All tests for this feature now use same script Replace all tests exercising the feature so that they all use the same test script. This will make the tests way easier to read. --- test/index.ts | 134 ++++++++------------------------------------------ 1 file changed, 20 insertions(+), 114 deletions(-) diff --git a/test/index.ts b/test/index.ts index 225869e60..0a1037c64 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2215,148 +2215,54 @@ describe('Datastore', () => { await runExcludeFromIndexesTest(properties, entitiesWithoutKey); }); it('should ignore non-existent property in excludeFromIndexes', async () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - const expectedConfig = getExpectedConfig( - {k: {stringValue: 'v', excludeFromIndexes: true}}, - namespace - ); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save({ - key, + const properties = {k: {stringValue: 'v', excludeFromIndexes: true}}; + const entitiesWithoutKey = { data: {k: 'v'}, excludeFromIndexes: ['k', 'k.*'], - }); - assert.deepStrictEqual(results, ['some-data']); + }; + await runExcludeFromIndexesTest(properties, entitiesWithoutKey); }); }); describe('when the property is not contained in excludeFromIndexes', () => { it('should encode a request without excludeFromIndexes', async () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - const expectedConfig = getExpectedConfig({}, namespace); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save({ - key, + const properties = {}; + const entitiesWithoutKey = { data: {}, - }); - assert.deepStrictEqual(results, ['some-data']); + }; + await runExcludeFromIndexesTest(properties, entitiesWithoutKey); }); it('should ignore non-existent property in excludeFromIndexes', async () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - const expectedConfig = getExpectedConfig({}, namespace); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save({ - key, + const properties = {}; + const entitiesWithoutKey = { data: {}, excludeFromIndexes: [ 'non_exist_property', // this just ignored 'non_exist_property.*', // should also be ignored ], - }); - assert.deepStrictEqual(results, ['some-data']); + }; + await runExcludeFromIndexesTest(properties, entitiesWithoutKey); }); }); describe('when the property is not contained in excludeFromIndexes and the property is an array', () => { it('should encode a request when there is no wildcard', async () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - const expectedConfig = getExpectedConfig( - {k: {stringValue: 'v'}}, - namespace - ); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save({ - key, + const properties = {k: {stringValue: 'v'}}; + const entitiesWithoutKey = { data: {k: 'v'}, excludeFromIndexes: [ 'non_exist_property[]', // this just ignored ], - }); - assert.deepStrictEqual(results, ['some-data']); + }; + await runExcludeFromIndexesTest(properties, entitiesWithoutKey); }); it('should encode a request when using a wildcard', async () => { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - const expectedConfig = getExpectedConfig( - {k: {stringValue: 'v'}}, - namespace - ); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save({ - key, + const properties = {k: {stringValue: 'v'}}; + const entitiesWithoutKey = { data: {k: 'v'}, excludeFromIndexes: [ 'non_exist_property[].*', // this just ignored ], - }); - assert.deepStrictEqual(results, ['some-data']); + }; + await runExcludeFromIndexesTest(properties, entitiesWithoutKey); }); }); }); From e43a1c36735e16fbd8d71a3daa2b66df0ecf9913 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 13:04:04 -0400 Subject: [PATCH 11/34] Add parameterized testing Add parameterized testing to eliminate repeated code. These tests need to be easier to read for the sake of the reviewer. --- package.json | 1 + test/index.ts | 221 ++++++++++++++++++++++++++------------------------ 2 files changed, 116 insertions(+), 106 deletions(-) diff --git a/package.json b/package.json index fced9bcb8..5a93f76e2 100644 --- a/package.json +++ b/package.json @@ -59,6 +59,7 @@ "@types/node": "^20.4.9", "@types/proxyquire": "^1.3.28", "@types/sinon": "^10.0.0", + "async": "^3.2.4", "c8": "^8.0.1", "gapic-tools": "^0.2.0", "gts": "^5.0.0", diff --git a/test/index.ts b/test/index.ts index 0a1037c64..7d230bfac 100644 --- a/test/index.ts +++ b/test/index.ts @@ -29,6 +29,7 @@ import * as extend from 'extend'; // eslint-disable-next-line @typescript-eslint/no-var-requires const v1 = require('../src/v1/index.js'); +const async = require('async'); // eslint-disable-next-line @typescript-eslint/no-explicit-any const fakeEntityInit: any = { @@ -2154,117 +2155,125 @@ describe('Datastore', () => { describe('Without using mocks', () => { const Datastore = OriginalDatastore; - describe('using save to evaluate excludeFromIndexes', () => { - function getExpectedConfig( - properties: any, - namespace: string | undefined - ) { - return { - client: 'DatastoreClient', - method: 'commit', - gaxOpts: {}, - reqOpts: { - mutations: [ - { - upsert: { - key: { - path: [{kind: 'Post', name: 'Post1'}], - partitionId: { - namespaceId: namespace, - }, + // TODO: Set type for properties. + const onSaveTests = [ + { + // When the property is contained in excludeFromIndexes + // then save should encode request without excludeFromIndexes + properties: {k: {stringValue: 'v'}}, + entitiesWithoutKey: {data: {k: 'v'}}, + }, + { + // When the property is contained in excludeFromIndexes + // then save should ignore non-existent property in excludeFromIndexes + properties: {k: {stringValue: 'v', excludeFromIndexes: true}}, + entitiesWithoutKey: { + data: {k: 'v'}, + excludeFromIndexes: ['k', 'k.*'], + }, + }, + { + // When the property is not contained in excludeFromIndexes + // then save should encode a request without excludeFromIndexes + properties: {}, + entitiesWithoutKey: {data: {}}, + }, + { + // When the property is not contained in excludeFromIndexes + // then save should ignore non-existent property in excludeFromIndexes + properties: {}, + entitiesWithoutKey: { + data: {}, + excludeFromIndexes: [ + 'non_exist_property', // this just ignored + 'non_exist_property.*', // should also be ignored + ], + }, + }, + { + // When the property is not contained in excludeFromIndexes and the property is an array + // then save should encode a request properly when there is no wildcard + properties: {k: {stringValue: 'v'}}, + entitiesWithoutKey: { + data: {k: 'v'}, + excludeFromIndexes: [ + 'non_exist_property[]', // this just ignored + ], + }, + }, + { + // When the property is not contained in excludeFromIndexes and the property is an array + // then save should encode a request properly using a wildcard + properties: {k: {stringValue: 'v'}}, + entitiesWithoutKey: { + data: {k: 'v'}, + excludeFromIndexes: [ + 'non_exist_property[].*', // this just ignored + ], + }, + }, + ]; + + function getExpectedConfig(properties: any, namespace: string | undefined) { + return { + client: 'DatastoreClient', + method: 'commit', + gaxOpts: {}, + reqOpts: { + mutations: [ + { + upsert: { + key: { + path: [{kind: 'Post', name: 'Post1'}], + partitionId: { + namespaceId: namespace, }, - properties, }, + properties, }, - ], - }, - }; - } + }, + ], + }, + }; + } - async function runExcludeFromIndexesTest( - properties: any, - entitiesWithoutKey: any - ) { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - const entities = Object.assign({key}, entitiesWithoutKey); - const expectedConfig = getExpectedConfig(properties, namespace); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save(entities); - assert.deepStrictEqual(results, ['some-data']); - } - describe('when the property is contained in excludeFromIndexes', () => { - it('should encode a request without excludeFromIndexes', async () => { - const properties = {k: {stringValue: 'v'}}; - const entitiesWithoutKey = { - data: {k: 'v'}, - }; - await runExcludeFromIndexesTest(properties, entitiesWithoutKey); - }); - it('should ignore non-existent property in excludeFromIndexes', async () => { - const properties = {k: {stringValue: 'v', excludeFromIndexes: true}}; - const entitiesWithoutKey = { - data: {k: 'v'}, - excludeFromIndexes: ['k', 'k.*'], - }; - await runExcludeFromIndexesTest(properties, entitiesWithoutKey); - }); - }); - describe('when the property is not contained in excludeFromIndexes', () => { - it('should encode a request without excludeFromIndexes', async () => { - const properties = {}; - const entitiesWithoutKey = { - data: {}, - }; - await runExcludeFromIndexesTest(properties, entitiesWithoutKey); - }); - it('should ignore non-existent property in excludeFromIndexes', async () => { - const properties = {}; - const entitiesWithoutKey = { - data: {}, - excludeFromIndexes: [ - 'non_exist_property', // this just ignored - 'non_exist_property.*', // should also be ignored - ], - }; - await runExcludeFromIndexesTest(properties, entitiesWithoutKey); - }); + async function runExcludeFromIndexesTest( + properties: any, + entitiesWithoutKey: any + ) { + const datastore = new Datastore({ + namespace: `${Date.now()}`, }); - describe('when the property is not contained in excludeFromIndexes and the property is an array', () => { - it('should encode a request when there is no wildcard', async () => { - const properties = {k: {stringValue: 'v'}}; - const entitiesWithoutKey = { - data: {k: 'v'}, - excludeFromIndexes: [ - 'non_exist_property[]', // this just ignored - ], - }; - await runExcludeFromIndexesTest(properties, entitiesWithoutKey); - }); - it('should encode a request when using a wildcard', async () => { - const properties = {k: {stringValue: 'v'}}; - const entitiesWithoutKey = { - data: {k: 'v'}, - excludeFromIndexes: [ - 'non_exist_property[].*', // this just ignored - ], - }; - await runExcludeFromIndexesTest(properties, entitiesWithoutKey); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); + const entities = Object.assign({key}, entitiesWithoutKey); + const expectedConfig = getExpectedConfig(properties, namespace); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save(entities); + assert.deepStrictEqual(results, ['some-data']); + } + + async.each( + onSaveTests, + (onSaveTest: {properties: any; entitiesWithoutKey: any}) => { + it('should pass the right properties to upsert on save', async () => { + console.log(`Running with parameters ${onSaveTest}`); + await runExcludeFromIndexesTest( + onSaveTest.properties, + onSaveTest.entitiesWithoutKey + ); }); - }); - }); + } + ); }); }); From b21bd99c3c35e986cd447721e9c44dccaee92dd4 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 13:07:30 -0400 Subject: [PATCH 12/34] Eliminate function and inline code This function is only used once now. We should inline this code in the test. --- test/index.ts | 52 ++++++++++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/test/index.ts b/test/index.ts index 7d230bfac..18f66ec80 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2237,41 +2237,33 @@ describe('Datastore', () => { }; } - async function runExcludeFromIndexesTest( - properties: any, - entitiesWithoutKey: any - ) { - const datastore = new Datastore({ - namespace: `${Date.now()}`, - }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - const entities = Object.assign({key}, entitiesWithoutKey); - const expectedConfig = getExpectedConfig(properties, namespace); - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - const results = await datastore.save(entities); - assert.deepStrictEqual(results, ['some-data']); - } - async.each( onSaveTests, (onSaveTest: {properties: any; entitiesWithoutKey: any}) => { it('should pass the right properties to upsert on save', async () => { console.log(`Running with parameters ${onSaveTest}`); - await runExcludeFromIndexesTest( - onSaveTest.properties, - onSaveTest.entitiesWithoutKey - ); + const properties = onSaveTest.properties; + const entitiesWithoutKey = onSaveTest.entitiesWithoutKey; + const datastore = new Datastore({ + namespace: `${Date.now()}`, + }); + const namespace = datastore.namespace; + const key = datastore.key(['Post', 'Post1']); + const entities = Object.assign({key}, entitiesWithoutKey); + const expectedConfig = getExpectedConfig(properties, namespace); + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + const results = await datastore.save(entities); + assert.deepStrictEqual(results, ['some-data']); }); } ); From 234f5019be8cb21708c90019f5813b5bb64d3df0 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 13:11:23 -0400 Subject: [PATCH 13/34] Add JSON stringify to test title In the test title include the properties passed in so that it is easy to track what passed/failed. --- test/index.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/index.ts b/test/index.ts index 18f66ec80..e4b2a79d5 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2240,8 +2240,9 @@ describe('Datastore', () => { async.each( onSaveTests, (onSaveTest: {properties: any; entitiesWithoutKey: any}) => { - it('should pass the right properties to upsert on save', async () => { - console.log(`Running with parameters ${onSaveTest}`); + it(`should pass the right properties to upsert on save with parameters: ${JSON.stringify( + onSaveTest + )}`, async () => { const properties = onSaveTest.properties; const entitiesWithoutKey = onSaveTest.entitiesWithoutKey; const datastore = new Datastore({ From 17a5f2d104976773b62380c3109c1d91b3dd0e52 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 13:16:37 -0400 Subject: [PATCH 14/34] Organize code so that similar variables are used Similar variables should be used together. Organize the code so that there is less confusion about which variables are related. --- test/index.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/index.ts b/test/index.ts index e4b2a79d5..a30cfd8ae 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2244,14 +2244,13 @@ describe('Datastore', () => { onSaveTest )}`, async () => { const properties = onSaveTest.properties; - const entitiesWithoutKey = onSaveTest.entitiesWithoutKey; const datastore = new Datastore({ namespace: `${Date.now()}`, }); const namespace = datastore.namespace; const key = datastore.key(['Post', 'Post1']); - const entities = Object.assign({key}, entitiesWithoutKey); const expectedConfig = getExpectedConfig(properties, namespace); + // Mock out the request function to compare config passed into it. datastore.request_ = ( config: RequestConfig, callback: RequestCallback @@ -2263,6 +2262,8 @@ describe('Datastore', () => { callback(e); } }; + // Attach key to entities parameter passed in. + const entities = Object.assign({key}, onSaveTest.entitiesWithoutKey); const results = await datastore.save(entities); assert.deepStrictEqual(results, ['some-data']); }); From aedf0c995dc1dba7661700dd31cc554d9b678421 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 13:21:38 -0400 Subject: [PATCH 15/34] inline getExpectedConfig function The getExpectedConfig does not need to be used anymore because its code can just be inlined in the one place it is used. --- test/index.ts | 44 ++++++++++++++++++++------------------------ 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/test/index.ts b/test/index.ts index a30cfd8ae..b0f15732d 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2214,29 +2214,6 @@ describe('Datastore', () => { }, ]; - function getExpectedConfig(properties: any, namespace: string | undefined) { - return { - client: 'DatastoreClient', - method: 'commit', - gaxOpts: {}, - reqOpts: { - mutations: [ - { - upsert: { - key: { - path: [{kind: 'Post', name: 'Post1'}], - partitionId: { - namespaceId: namespace, - }, - }, - properties, - }, - }, - ], - }, - }; - } - async.each( onSaveTests, (onSaveTest: {properties: any; entitiesWithoutKey: any}) => { @@ -2249,7 +2226,26 @@ describe('Datastore', () => { }); const namespace = datastore.namespace; const key = datastore.key(['Post', 'Post1']); - const expectedConfig = getExpectedConfig(properties, namespace); + const expectedConfig = { + client: 'DatastoreClient', + method: 'commit', + gaxOpts: {}, + reqOpts: { + mutations: [ + { + upsert: { + key: { + path: [{kind: 'Post', name: 'Post1'}], + partitionId: { + namespaceId: namespace, + }, + }, + properties, + }, + }, + ], + }, + }; // Mock out the request function to compare config passed into it. datastore.request_ = ( config: RequestConfig, From af1130f3bc0cb275d99c9d10edbccd210984f3a4 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 13:33:18 -0400 Subject: [PATCH 16/34] Separate tests into blocks and inline code Separate the test into blocks and inline some parameters for better test readability. --- test/index.ts | 78 +++++++++++++++++++++++++++------------------------ 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/test/index.ts b/test/index.ts index b0f15732d..f705d03b8 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2220,48 +2220,54 @@ describe('Datastore', () => { it(`should pass the right properties to upsert on save with parameters: ${JSON.stringify( onSaveTest )}`, async () => { - const properties = onSaveTest.properties; const datastore = new Datastore({ namespace: `${Date.now()}`, }); - const namespace = datastore.namespace; - const key = datastore.key(['Post', 'Post1']); - const expectedConfig = { - client: 'DatastoreClient', - method: 'commit', - gaxOpts: {}, - reqOpts: { - mutations: [ - { - upsert: { - key: { - path: [{kind: 'Post', name: 'Post1'}], - partitionId: { - namespaceId: namespace, + { + // This block of code mocks out request_ to check values passed into it. + const expectedConfig = { + client: 'DatastoreClient', + method: 'commit', + gaxOpts: {}, + reqOpts: { + mutations: [ + { + upsert: { + key: { + path: [{kind: 'Post', name: 'Post1'}], + partitionId: { + namespaceId: datastore.namespace, + }, }, + properties: onSaveTest.properties, }, - properties, }, - }, - ], - }, - }; - // Mock out the request function to compare config passed into it. - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - // Attach key to entities parameter passed in. - const entities = Object.assign({key}, onSaveTest.entitiesWithoutKey); - const results = await datastore.save(entities); - assert.deepStrictEqual(results, ['some-data']); + ], + }, + }; + // Mock out the request function to compare config passed into it. + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + } + { + // Attach key to entities parameter passed in and run save with those parameters. + const key = datastore.key(['Post', 'Post1']); + const entities = Object.assign( + {key}, + onSaveTest.entitiesWithoutKey + ); + const results = await datastore.save(entities); + assert.deepStrictEqual(results, ['some-data']); + } }); } ); From b12735933fa2da1e3a7a063299983c153f40c945 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 13:56:31 -0400 Subject: [PATCH 17/34] Add types to parameters passed into async Stronger typing makes the code easier to read. --- test/index.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/index.ts b/test/index.ts index f705d03b8..5458a62e7 100644 --- a/test/index.ts +++ b/test/index.ts @@ -26,6 +26,8 @@ import {RequestCallback, RequestConfig} from '../src/request'; import * as is from 'is'; import * as sinon from 'sinon'; import * as extend from 'extend'; +import * as protos from '../protos/protos'; +import {google} from '../protos/protos'; // eslint-disable-next-line @typescript-eslint/no-var-requires const v1 = require('../src/v1/index.js'); @@ -2155,7 +2157,6 @@ describe('Datastore', () => { describe('Without using mocks', () => { const Datastore = OriginalDatastore; - // TODO: Set type for properties. const onSaveTests = [ { // When the property is contained in excludeFromIndexes @@ -2216,7 +2217,10 @@ describe('Datastore', () => { async.each( onSaveTests, - (onSaveTest: {properties: any; entitiesWithoutKey: any}) => { + (onSaveTest: { + properties: google.datastore.v1.IValue; + entitiesWithoutKey: Entities; + }) => { it(`should pass the right properties to upsert on save with parameters: ${JSON.stringify( onSaveTest )}`, async () => { From 66d0a37fd7b6a93f4c03338822175d6fcd17c023 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 13:58:11 -0400 Subject: [PATCH 18/34] Eliminate unnecessary variable assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don’t assign to the Datastore variable. This is not necessary. Just use the OriginalDatastore variable directly. --- test/index.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/index.ts b/test/index.ts index 5458a62e7..57f4b2e28 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2156,7 +2156,6 @@ describe('Datastore', () => { }); describe('Without using mocks', () => { - const Datastore = OriginalDatastore; const onSaveTests = [ { // When the property is contained in excludeFromIndexes @@ -2224,7 +2223,7 @@ describe('Datastore', () => { it(`should pass the right properties to upsert on save with parameters: ${JSON.stringify( onSaveTest )}`, async () => { - const datastore = new Datastore({ + const datastore = new OriginalDatastore({ namespace: `${Date.now()}`, }); { From 94d7a24dd06ff88a58b977334a1cb2886d0dc3da Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 14:29:34 -0400 Subject: [PATCH 19/34] Ran linter and simplified source changes The diff will be easier to read if we do not add a nested if statement and instead apply a condition on the if and change the else to else if. --- src/entity.ts | 35 +++++++++++++++++------------------ test/index.ts | 8 +++++++- 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/entity.ts b/src/entity.ts index 6ee99f4a0..1b70f8b00 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -904,26 +904,25 @@ export namespace entity { excludePathFromEntity(entity, newPath); }); } else { - if (typeof entity.properties![firstPathPart] !== 'undefined') { - if (hasWildCard && remainderPath === '*') { - const parentEntity = - entity.properties![firstPathPart].entityValue; - - if (parentEntity) { - Object.keys(parentEntity.properties).forEach(path => { - const newPath = parentEntity.properties[path].arrayValue - ? path + '[].*' - : path + '.*'; - excludePathFromEntity(parentEntity, newPath); - }); - } else { - excludePathFromEntity(entity, firstPathPart); - } + if ( + hasWildCard && + remainderPath === '*' && + entity.properties![firstPathPart] !== undefined + ) { + const parentEntity = entity.properties![firstPathPart].entityValue; + if (parentEntity) { + Object.keys(parentEntity.properties).forEach(path => { + const newPath = parentEntity.properties[path].arrayValue + ? path + '[].*' + : path + '.*'; + excludePathFromEntity(parentEntity, newPath); + }); } else { - const parentEntity = - entity.properties![firstPathPart].entityValue; - excludePathFromEntity(parentEntity, remainderPath); + excludePathFromEntity(entity, firstPathPart); } + } else if (entity.properties![firstPathPart] !== undefined) { + const parentEntity = entity.properties![firstPathPart].entityValue; + excludePathFromEntity(parentEntity, remainderPath); } } } diff --git a/test/index.ts b/test/index.ts index 57f4b2e28..96c9408ca 100644 --- a/test/index.ts +++ b/test/index.ts @@ -21,7 +21,13 @@ import {PassThrough, Readable} from 'stream'; import * as ds from '../src'; import {Datastore, DatastoreOptions} from '../src'; import {Datastore as OriginalDatastore} from '../src'; -import {entity, Entity, EntityProto, EntityObject, Entities} from '../src/entity'; +import { + entity, + Entity, + EntityProto, + EntityObject, + Entities, +} from '../src/entity'; import {RequestCallback, RequestConfig} from '../src/request'; import * as is from 'is'; import * as sinon from 'sinon'; From d9da7dae31792d092f2a4c7d9a752678bad5468d Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 14:31:02 -0400 Subject: [PATCH 20/34] Add blank line back for easier diff reading --- src/entity.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/entity.ts b/src/entity.ts index 1b70f8b00..ef82375b6 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -910,6 +910,7 @@ export namespace entity { entity.properties![firstPathPart] !== undefined ) { const parentEntity = entity.properties![firstPathPart].entityValue; + if (parentEntity) { Object.keys(parentEntity.properties).forEach(path => { const newPath = parentEntity.properties[path].arrayValue From 0cd7cbc856eec11d11cc41132b55bf9c0b7fb6ca Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 14:32:08 -0400 Subject: [PATCH 21/34] Try again, eliminate the blank line --- src/entity.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/entity.ts b/src/entity.ts index ef82375b6..73879b586 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -910,7 +910,7 @@ export namespace entity { entity.properties![firstPathPart] !== undefined ) { const parentEntity = entity.properties![firstPathPart].entityValue; - + if (parentEntity) { Object.keys(parentEntity.properties).forEach(path => { const newPath = parentEntity.properties[path].arrayValue From 377770bf4074dcc2c55327ff92d57fde60ae7a4c Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 14:35:05 -0400 Subject: [PATCH 22/34] Additional adjustment to entity first path part Apply the first suggestion in the PR to all examples --- src/entity.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/entity.ts b/src/entity.ts index 73879b586..6b9f49855 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -884,7 +884,7 @@ export namespace entity { firstPathPartIsArray && hasWildCard && remainderPath === '*' && - typeof entity.properties![firstPathPart] !== 'undefined' + entity.properties![firstPathPart] !== undefined ) { const array = entity.properties![firstPathPart].arrayValue; // eslint-disable-next-line @typescript-eslint/no-explicit-any From df3f3769926f62bf1ed3ea92bb5aef85974a1e42 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 15:53:47 -0400 Subject: [PATCH 23/34] Define isFirstPathPartUndefined There is a repeated code fragment for checking if the first path part is undefined that we should refactor. --- src/entity.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/entity.ts b/src/entity.ts index 6b9f49855..96bf14631 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -843,6 +843,8 @@ export namespace entity { const splitPath = path.split(delimiter); const firstPathPart = splitPath.shift()!; const remainderPath = splitPath.join(delimiter).replace(/^(\.|\[\])/, ''); + const isFirstPathPartUndefined = + entity.properties![firstPathPart] !== undefined; if ( !(entity.properties && entity.properties[firstPathPart]) && @@ -855,7 +857,7 @@ export namespace entity { if ( firstPathPartIsArray && // check also if the property in question is actually an array value. - entity.properties![firstPathPart] !== undefined && + isFirstPathPartUndefined && entity.properties![firstPathPart].arrayValue && // check if wildcard is not applied !hasWildCard @@ -884,7 +886,7 @@ export namespace entity { firstPathPartIsArray && hasWildCard && remainderPath === '*' && - entity.properties![firstPathPart] !== undefined + isFirstPathPartUndefined ) { const array = entity.properties![firstPathPart].arrayValue; // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -907,7 +909,7 @@ export namespace entity { if ( hasWildCard && remainderPath === '*' && - entity.properties![firstPathPart] !== undefined + isFirstPathPartUndefined ) { const parentEntity = entity.properties![firstPathPart].entityValue; @@ -921,7 +923,7 @@ export namespace entity { } else { excludePathFromEntity(entity, firstPathPart); } - } else if (entity.properties![firstPathPart] !== undefined) { + } else if (isFirstPathPartUndefined) { const parentEntity = entity.properties![firstPathPart].entityValue; excludePathFromEntity(parentEntity, remainderPath); } From 4f54d9495b7b37fc0a7a7f5002ec068bf91c7bbc Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 15:55:35 -0400 Subject: [PATCH 24/34] Rename the variable to align with what it does --- src/entity.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/entity.ts b/src/entity.ts index 96bf14631..aace92897 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -843,7 +843,7 @@ export namespace entity { const splitPath = path.split(delimiter); const firstPathPart = splitPath.shift()!; const remainderPath = splitPath.join(delimiter).replace(/^(\.|\[\])/, ''); - const isFirstPathPartUndefined = + const isFirstPathPartDefined = entity.properties![firstPathPart] !== undefined; if ( @@ -857,7 +857,7 @@ export namespace entity { if ( firstPathPartIsArray && // check also if the property in question is actually an array value. - isFirstPathPartUndefined && + isFirstPathPartDefined && entity.properties![firstPathPart].arrayValue && // check if wildcard is not applied !hasWildCard @@ -886,7 +886,7 @@ export namespace entity { firstPathPartIsArray && hasWildCard && remainderPath === '*' && - isFirstPathPartUndefined + isFirstPathPartDefined ) { const array = entity.properties![firstPathPart].arrayValue; // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -909,7 +909,7 @@ export namespace entity { if ( hasWildCard && remainderPath === '*' && - isFirstPathPartUndefined + isFirstPathPartDefined ) { const parentEntity = entity.properties![firstPathPart].entityValue; @@ -923,7 +923,7 @@ export namespace entity { } else { excludePathFromEntity(entity, firstPathPart); } - } else if (isFirstPathPartUndefined) { + } else if (isFirstPathPartDefined) { const parentEntity = entity.properties![firstPathPart].entityValue; excludePathFromEntity(parentEntity, remainderPath); } From a101f27120f780a5561e8dfeea3edbaf0cbdd0d6 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 15:56:43 -0400 Subject: [PATCH 25/34] run linter --- src/entity.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/entity.ts b/src/entity.ts index aace92897..3a63d515d 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -906,11 +906,7 @@ export namespace entity { excludePathFromEntity(entity, newPath); }); } else { - if ( - hasWildCard && - remainderPath === '*' && - isFirstPathPartDefined - ) { + if (hasWildCard && remainderPath === '*' && isFirstPathPartDefined) { const parentEntity = entity.properties![firstPathPart].entityValue; if (parentEntity) { From 225374031d4504310ffeb4402a8a64bdf6732116 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 16:03:26 -0400 Subject: [PATCH 26/34] Revert "run linter" This reverts commit a101f27120f780a5561e8dfeea3edbaf0cbdd0d6. --- src/entity.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/entity.ts b/src/entity.ts index 3a63d515d..aace92897 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -906,7 +906,11 @@ export namespace entity { excludePathFromEntity(entity, newPath); }); } else { - if (hasWildCard && remainderPath === '*' && isFirstPathPartDefined) { + if ( + hasWildCard && + remainderPath === '*' && + isFirstPathPartDefined + ) { const parentEntity = entity.properties![firstPathPart].entityValue; if (parentEntity) { From 8db651a6c0d4a37b28ca7bc12da07badd7f1964f Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 16:03:32 -0400 Subject: [PATCH 27/34] Revert "Rename the variable to align with what it does" This reverts commit 4f54d9495b7b37fc0a7a7f5002ec068bf91c7bbc. --- src/entity.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/entity.ts b/src/entity.ts index aace92897..96bf14631 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -843,7 +843,7 @@ export namespace entity { const splitPath = path.split(delimiter); const firstPathPart = splitPath.shift()!; const remainderPath = splitPath.join(delimiter).replace(/^(\.|\[\])/, ''); - const isFirstPathPartDefined = + const isFirstPathPartUndefined = entity.properties![firstPathPart] !== undefined; if ( @@ -857,7 +857,7 @@ export namespace entity { if ( firstPathPartIsArray && // check also if the property in question is actually an array value. - isFirstPathPartDefined && + isFirstPathPartUndefined && entity.properties![firstPathPart].arrayValue && // check if wildcard is not applied !hasWildCard @@ -886,7 +886,7 @@ export namespace entity { firstPathPartIsArray && hasWildCard && remainderPath === '*' && - isFirstPathPartDefined + isFirstPathPartUndefined ) { const array = entity.properties![firstPathPart].arrayValue; // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -909,7 +909,7 @@ export namespace entity { if ( hasWildCard && remainderPath === '*' && - isFirstPathPartDefined + isFirstPathPartUndefined ) { const parentEntity = entity.properties![firstPathPart].entityValue; @@ -923,7 +923,7 @@ export namespace entity { } else { excludePathFromEntity(entity, firstPathPart); } - } else if (isFirstPathPartDefined) { + } else if (isFirstPathPartUndefined) { const parentEntity = entity.properties![firstPathPart].entityValue; excludePathFromEntity(parentEntity, remainderPath); } From b798ec3da29a9f43f654f843711713b19b8c63d3 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 16:03:39 -0400 Subject: [PATCH 28/34] Revert "Define isFirstPathPartUndefined" This reverts commit df3f3769926f62bf1ed3ea92bb5aef85974a1e42. --- src/entity.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/entity.ts b/src/entity.ts index 96bf14631..6b9f49855 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -843,8 +843,6 @@ export namespace entity { const splitPath = path.split(delimiter); const firstPathPart = splitPath.shift()!; const remainderPath = splitPath.join(delimiter).replace(/^(\.|\[\])/, ''); - const isFirstPathPartUndefined = - entity.properties![firstPathPart] !== undefined; if ( !(entity.properties && entity.properties[firstPathPart]) && @@ -857,7 +855,7 @@ export namespace entity { if ( firstPathPartIsArray && // check also if the property in question is actually an array value. - isFirstPathPartUndefined && + entity.properties![firstPathPart] !== undefined && entity.properties![firstPathPart].arrayValue && // check if wildcard is not applied !hasWildCard @@ -886,7 +884,7 @@ export namespace entity { firstPathPartIsArray && hasWildCard && remainderPath === '*' && - isFirstPathPartUndefined + entity.properties![firstPathPart] !== undefined ) { const array = entity.properties![firstPathPart].arrayValue; // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -909,7 +907,7 @@ export namespace entity { if ( hasWildCard && remainderPath === '*' && - isFirstPathPartUndefined + entity.properties![firstPathPart] !== undefined ) { const parentEntity = entity.properties![firstPathPart].entityValue; @@ -923,7 +921,7 @@ export namespace entity { } else { excludePathFromEntity(entity, firstPathPart); } - } else if (isFirstPathPartUndefined) { + } else if (entity.properties![firstPathPart] !== undefined) { const parentEntity = entity.properties![firstPathPart].entityValue; excludePathFromEntity(parentEntity, remainderPath); } From 3133879116c9f77ba081d2d556bbcc0eacccae60 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 16:16:58 -0400 Subject: [PATCH 29/34] Refactor check out for seeing if defined We do four checks to see if the first path part is defined. We should refactor these checks out. --- src/entity.ts | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/entity.ts b/src/entity.ts index 6b9f49855..b4cea0b7c 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -852,10 +852,12 @@ export namespace entity { return; } + const isFirstPathPartDefined = + entity.properties![firstPathPart] !== undefined; if ( firstPathPartIsArray && // check also if the property in question is actually an array value. - entity.properties![firstPathPart] !== undefined && + isFirstPathPartDefined && entity.properties![firstPathPart].arrayValue && // check if wildcard is not applied !hasWildCard @@ -884,7 +886,7 @@ export namespace entity { firstPathPartIsArray && hasWildCard && remainderPath === '*' && - entity.properties![firstPathPart] !== undefined + isFirstPathPartDefined ) { const array = entity.properties![firstPathPart].arrayValue; // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -904,11 +906,7 @@ export namespace entity { excludePathFromEntity(entity, newPath); }); } else { - if ( - hasWildCard && - remainderPath === '*' && - entity.properties![firstPathPart] !== undefined - ) { + if (hasWildCard && remainderPath === '*' && isFirstPathPartDefined) { const parentEntity = entity.properties![firstPathPart].entityValue; if (parentEntity) { @@ -921,7 +919,7 @@ export namespace entity { } else { excludePathFromEntity(entity, firstPathPart); } - } else if (entity.properties![firstPathPart] !== undefined) { + } else if (isFirstPathPartDefined) { const parentEntity = entity.properties![firstPathPart].entityValue; excludePathFromEntity(parentEntity, remainderPath); } From b148c959fcfdcf7bcc35177f295cb94d6da100a9 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 28 Sep 2023 16:25:00 -0400 Subject: [PATCH 30/34] Move comment to more appropriate place The comment should reflect the next line of code. --- src/entity.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/entity.ts b/src/entity.ts index b4cea0b7c..2c939656d 100644 --- a/src/entity.ts +++ b/src/entity.ts @@ -856,8 +856,8 @@ export namespace entity { entity.properties![firstPathPart] !== undefined; if ( firstPathPartIsArray && - // check also if the property in question is actually an array value. isFirstPathPartDefined && + // check also if the property in question is actually an array value. entity.properties![firstPathPart].arrayValue && // check if wildcard is not applied !hasWildCard From d1ff7d6f41f44649200d887d766887041e9e6d5d Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 29 Sep 2023 14:23:16 -0400 Subject: [PATCH 31/34] Replace comments with description Replace comments with description property so that the tests will report the description when running the tests instead of the properties in the description. --- test/index.ts | 223 +++++++++++++++++++++++++------------------------- 1 file changed, 112 insertions(+), 111 deletions(-) diff --git a/test/index.ts b/test/index.ts index 4e13335ec..9ddb6a04d 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2195,124 +2195,125 @@ async.each( }); describe('Without using mocks', () => { - const onSaveTests = [ - { - // When the property is contained in excludeFromIndexes - // then save should encode request without excludeFromIndexes - properties: {k: {stringValue: 'v'}}, - entitiesWithoutKey: {data: {k: 'v'}}, - }, - { - // When the property is contained in excludeFromIndexes - // then save should ignore non-existent property in excludeFromIndexes - properties: {k: {stringValue: 'v', excludeFromIndexes: true}}, - entitiesWithoutKey: { - data: {k: 'v'}, - excludeFromIndexes: ['k', 'k.*'], + describe('onSave tests', () => { + const onSaveTests = [ + { + description: + 'should encode a save request without excludeFromIndexes', + properties: {k: {stringValue: 'v'}}, + entitiesWithoutKey: {data: {k: 'v'}}, }, - }, - { - // When the property is not contained in excludeFromIndexes - // then save should encode a request without excludeFromIndexes - properties: {}, - entitiesWithoutKey: {data: {}}, - }, - { - // When the property is not contained in excludeFromIndexes - // then save should ignore non-existent property in excludeFromIndexes - properties: {}, - entitiesWithoutKey: { - data: {}, - excludeFromIndexes: [ - 'non_exist_property', // this just ignored - 'non_exist_property.*', // should also be ignored - ], + { + description: + 'should add exclude from indexes to property k and ignore excludeFromIndexes with wildcard', + properties: {k: {stringValue: 'v', excludeFromIndexes: true}}, + entitiesWithoutKey: { + data: {k: 'v'}, + excludeFromIndexes: ['k', 'k.*'], + }, }, - }, - { - // When the property is not contained in excludeFromIndexes and the property is an array - // then save should encode a request properly when there is no wildcard - properties: {k: {stringValue: 'v'}}, - entitiesWithoutKey: { - data: {k: 'v'}, - excludeFromIndexes: [ - 'non_exist_property[]', // this just ignored - ], + { + description: + 'should encode a save request without properties and without excludeFromIndexes', + properties: {}, + entitiesWithoutKey: {data: {}}, }, - }, - { - // When the property is not contained in excludeFromIndexes and the property is an array - // then save should encode a request properly using a wildcard - properties: {k: {stringValue: 'v'}}, - entitiesWithoutKey: { - data: {k: 'v'}, - excludeFromIndexes: [ - 'non_exist_property[].*', // this just ignored - ], + { + description: + 'should encode a save request with no properties ignoring excludeFromIndexes for a property not on save data', + properties: {}, + entitiesWithoutKey: { + data: {}, + excludeFromIndexes: [ + 'non_exist_property', // this just ignored + 'non_exist_property.*', // should also be ignored + ], + }, }, - }, - ]; - - async.each( - onSaveTests, - (onSaveTest: { - properties: google.datastore.v1.IValue; - entitiesWithoutKey: Entities; - }) => { - it(`should pass the right properties to upsert on save with parameters: ${JSON.stringify( - onSaveTest - )}`, async () => { - const datastore = new OriginalDatastore({ - namespace: `${Date.now()}`, - }); - { - // This block of code mocks out request_ to check values passed into it. - const expectedConfig = { - client: 'DatastoreClient', - method: 'commit', - gaxOpts: {}, - reqOpts: { - mutations: [ - { - upsert: { - key: { - path: [{kind: 'Post', name: 'Post1'}], - partitionId: { - namespaceId: datastore.namespace, + { + description: + 'should encode a save request with one property ignoring excludeFromIndexes for a property not on save data', + properties: {k: {stringValue: 'v'}}, + entitiesWithoutKey: { + data: {k: 'v'}, + excludeFromIndexes: [ + 'non_exist_property[]', // this just ignored + ], + }, + }, + { + description: + 'should encode a save request with one property ignoring excludeFromIndexes for a property with a wildcard not on save data', + properties: {k: {stringValue: 'v'}}, + entitiesWithoutKey: { + data: {k: 'v'}, + excludeFromIndexes: [ + 'non_exist_property[].*', // this just ignored + ], + }, + }, + ]; + + async.each( + onSaveTests, + (onSaveTest: { + description: string; + properties: google.datastore.v1.IValue; + entitiesWithoutKey: Entities; + }) => { + it(`should pass the right properties to upsert on save with parameters: ${onSaveTest.description}`, async () => { + const datastore = new OriginalDatastore({ + namespace: `${Date.now()}`, + }); + { + // This block of code mocks out request_ to check values passed into it. + const expectedConfig = { + client: 'DatastoreClient', + method: 'commit', + gaxOpts: {}, + reqOpts: { + mutations: [ + { + upsert: { + key: { + path: [{kind: 'Post', name: 'Post1'}], + partitionId: { + namespaceId: datastore.namespace, + }, }, + properties: onSaveTest.properties, }, - properties: onSaveTest.properties, }, - }, - ], - }, - }; - // Mock out the request function to compare config passed into it. - datastore.request_ = ( - config: RequestConfig, - callback: RequestCallback - ) => { - try { - assert.deepStrictEqual(config, expectedConfig); - callback(null, 'some-data'); - } catch (e: any) { - callback(e); - } - }; - } - { - // Attach key to entities parameter passed in and run save with those parameters. - const key = datastore.key(['Post', 'Post1']); - const entities = Object.assign( - {key}, - onSaveTest.entitiesWithoutKey - ); - const results = await datastore.save(entities); - assert.deepStrictEqual(results, ['some-data']); - } - }); - } - ); + ], + }, + }; + // Mock out the request function to compare config passed into it. + datastore.request_ = ( + config: RequestConfig, + callback: RequestCallback + ) => { + try { + assert.deepStrictEqual(config, expectedConfig); + callback(null, 'some-data'); + } catch (e: any) { + callback(e); + } + }; + } + { + // Attach key to entities parameter passed in and run save with those parameters. + const key = datastore.key(['Post', 'Post1']); + const entities = Object.assign( + {key}, + onSaveTest.entitiesWithoutKey + ); + const results = await datastore.save(entities); + assert.deepStrictEqual(results, ['some-data']); + } + }); + } + ); + }); }); describe('multi-db support', () => { From 620ee00894c683dfe71c5f1fd6ddbcbdf8907b4a Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 29 Sep 2023 14:25:02 -0400 Subject: [PATCH 32/34] Eliminate prefix. Only use description --- test/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/index.ts b/test/index.ts index 9ddb6a04d..115c96257 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2261,7 +2261,7 @@ async.each( properties: google.datastore.v1.IValue; entitiesWithoutKey: Entities; }) => { - it(`should pass the right properties to upsert on save with parameters: ${onSaveTest.description}`, async () => { + it(`${onSaveTest.description}`, async () => { const datastore = new OriginalDatastore({ namespace: `${Date.now()}`, }); From e0d770fc6677676d1f541bc1941641b59382375f Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 29 Sep 2023 14:26:22 -0400 Subject: [PATCH 33/34] fix typo --- test/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/index.ts b/test/index.ts index 115c96257..c5a40ef0a 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2195,7 +2195,7 @@ async.each( }); describe('Without using mocks', () => { - describe('onSave tests', () => { + describe('on save tests', () => { const onSaveTests = [ { description: From e1aecb659aae504d86c8e92a2437ebc1cc3e0981 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 29 Sep 2023 14:28:02 -0400 Subject: [PATCH 34/34] lowercase convention --- test/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/index.ts b/test/index.ts index c5a40ef0a..c4171b537 100644 --- a/test/index.ts +++ b/test/index.ts @@ -2194,7 +2194,7 @@ async.each( }); }); - describe('Without using mocks', () => { + describe('without using mocks', () => { describe('on save tests', () => { const onSaveTests = [ {