From e62ae956eff74e9f218eda5bc1524144ef74862b Mon Sep 17 00:00:00 2001 From: Denis DelGrosso Date: Wed, 28 Feb 2024 16:00:35 +0000 Subject: [PATCH 1/4] test: improvements to requester pays tests and bucket metadata updates test: improvements to requester pays tests and bucket metadata updates --- system-test/storage.ts | 94 ++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 49 deletions(-) diff --git a/system-test/storage.ts b/system-test/storage.ts index 9ea87526a..2ab52769b 100644 --- a/system-test/storage.ts +++ b/system-test/storage.ts @@ -231,8 +231,10 @@ describe('storage', () => { describe('buckets', () => { // Some bucket update operations have a rate limit. // Introduce a delay between tests to avoid getting an error. - beforeEach(done => { - setTimeout(done, 1000); + beforeEach(async () => { + await new Promise(resolve => + setTimeout(resolve, BUCKET_METADATA_UPDATE_WAIT_TIME) + ); }); it('should get access controls', async () => { @@ -296,6 +298,9 @@ describe('storage', () => { entity: 'allUsers', role: 'READER', }); + await new Promise(resolve => + setTimeout(resolve, BUCKET_METADATA_UPDATE_WAIT_TIME) + ); await bucket.acl.delete({entity: 'allUsers'}); }); @@ -319,6 +324,9 @@ describe('storage', () => { it('should make a bucket private', async () => { try { await bucket.makePublic(); + await new Promise(resolve => + setTimeout(resolve, BUCKET_METADATA_UPDATE_WAIT_TIME) + ); await bucket.makePrivate(); assert.rejects(bucket.acl.get({entity: 'allUsers'}), err => { assert.strictEqual((err as ApiError).code, 404); @@ -1727,40 +1735,31 @@ describe('storage', () => { // // - file.save() // -> file.createWriteStream() - before(() => { + before(async () => { file = bucketNonAllowList.file(generateName()); - return bucket - .enableRequesterPays() - .then(() => bucket.iam.getPolicy()) - .then(data => { - const policy = data[0]; - - // Allow an absolute or relative path (from project root) - // for the key file. - let key2 = process.env.GCN_STORAGE_2ND_PROJECT_KEY; - if (key2 && key2.charAt(0) === '.') { - key2 = `${getDirName()}/../../../${key2}`; - } - - // Get the service account for the "second" account (the - // one that will read the requester pays file). - const clientEmail = JSON.parse( - fs.readFileSync(key2!, 'utf-8') - ).client_email; - - policy.bindings.push({ - role: 'roles/storage.admin', - members: [`serviceAccount:${clientEmail}`], - }); - - return bucket.iam.setPolicy(policy); - }) - .then(() => file.save('abc', USER_PROJECT_OPTIONS)) - .then(() => topic.getMetadata()) - .then(data => { - topicName = data[0].name!; - }); + await bucket.enableRequesterPays(); + const data = await bucket.iam.getPolicy(); + const policy = data[0]; + // Allow an absolute or relative path (from project root) + // for the key file. + let key2 = process.env.GCN_STORAGE_2ND_PROJECT_KEY; + if (key2 && key2.charAt(0) === '.') { + key2 = `${getDirName()}/../../../${key2}`; + } + // Get the service account for the "second" account (the + // one that will read the requester pays file). + const clientEmail = JSON.parse( + fs.readFileSync(key2!, 'utf-8') + ).client_email; + policy.bindings.push({ + role: 'roles/storage.admin', + members: [`serviceAccount:${clientEmail}`], + }); + await bucket.iam.setPolicy(policy); + await file.save('abc', USER_PROJECT_OPTIONS); + const data_2 = await topic.getMetadata(); + topicName = data_2[0].name!; }); // This acts as a test for the following methods: @@ -1788,7 +1787,7 @@ describe('storage', () => { type requesterPaysFunction< T = {} | typeof USER_PROJECT_OPTIONS, R = {} | void, - > = (options: T) => Promise; + > = (options?: T) => Promise; /** * Accepts a function and runs 2 tests - a test where the requester pays @@ -1805,20 +1804,17 @@ describe('storage', () => { const failureMessage = 'Bucket is a requester pays bucket but no user project provided.'; - let expectedError: unknown = null; - - try { - // Should raise an error on requester pays bucket - await testFunction({}); - } catch (e) { - expectedError = e; - } - - assert(expectedError instanceof Error); - assert( - expectedError.message.includes(failureMessage), - `Expected '${expectedError.message}' to include '${failureMessage}'` - ); + await assert.rejects(testFunction(), err => { + console.log(JSON.stringify(err)); + console.log(''); + assert( + (err as Error).message.includes(failureMessage), + `Expected '${ + (err as Error).message + }' to include '${failureMessage}'` + ); + return true; + }); // Validate the desired functionality const results = await testFunction(USER_PROJECT_OPTIONS); From dfac8f7c132fa34ac16f7042197f06d031c72d9b Mon Sep 17 00:00:00 2001 From: Denis DelGrosso Date: Wed, 28 Feb 2024 20:45:34 +0000 Subject: [PATCH 2/4] remove console.log statements --- system-test/storage.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/system-test/storage.ts b/system-test/storage.ts index 2ab52769b..7b4daa6dd 100644 --- a/system-test/storage.ts +++ b/system-test/storage.ts @@ -1805,8 +1805,6 @@ describe('storage', () => { 'Bucket is a requester pays bucket but no user project provided.'; await assert.rejects(testFunction(), err => { - console.log(JSON.stringify(err)); - console.log(''); assert( (err as Error).message.includes(failureMessage), `Expected '${ From 69a9d51bdd3a355d713ebffff155952c75f1975b Mon Sep 17 00:00:00 2001 From: Denis DelGrosso Date: Wed, 28 Feb 2024 21:28:20 +0000 Subject: [PATCH 3/4] adjust wait time slightly --- system-test/storage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system-test/storage.ts b/system-test/storage.ts index 7b4daa6dd..b2c760d5d 100644 --- a/system-test/storage.ts +++ b/system-test/storage.ts @@ -60,7 +60,7 @@ const RUNNING_IN_VPCSC = !!process.env['GOOGLE_CLOUD_TESTS_IN_VPCSC']; const UNIFORM_ACCESS_TIMEOUT = 60 * 1000; // 60s see: https://cloud.google.com/storage/docs/consistency#eventually_consistent_operations const UNIFORM_ACCESS_WAIT_TIME = 5 * 1000; // 5s -const BUCKET_METADATA_UPDATE_WAIT_TIME = 1000; // 1s buckets have a max rate of one metadata update per second. +const BUCKET_METADATA_UPDATE_WAIT_TIME = 1250; // 1.25s buckets have a max rate of one metadata update per second. // block all attempts to chat with the metadata server (kokoro runs on GCE) nock('http://metadata.google.internal') From bccecd1567ff129f7918ed3a067ef60b94b586ca Mon Sep 17 00:00:00 2001 From: Denis DelGrosso Date: Thu, 29 Feb 2024 15:52:38 +0000 Subject: [PATCH 4/4] add retries to failed tests --- system-test/storage.ts | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/system-test/storage.ts b/system-test/storage.ts index b2c760d5d..8802331f3 100644 --- a/system-test/storage.ts +++ b/system-test/storage.ts @@ -68,7 +68,10 @@ nock('http://metadata.google.internal') .replyWithError({code: 'ENOTFOUND'}) .persist(); -describe('storage', () => { +// eslint-disable-next-line prefer-arrow-callback +describe('storage', function () { + this.retries(3); + const USER_ACCOUNT = 'user-spsawchuk@gmail.com'; const TESTS_PREFIX = `storage-tests-${shortUUID()}-`; const RETENTION_DURATION_SECONDS = 10; @@ -110,23 +113,18 @@ describe('storage', () => { }, }; - before(() => { - return bucket - .create() - .then(() => { - return pubsub.createTopic(generateName()); - }) - .then(data => { - topic = data[0]; - return topic.iam.setPolicy({ - bindings: [ - { - role: 'roles/pubsub.editor', - members: ['allUsers'], - }, - ], - }); - }); + before(async () => { + await bucket.create(); + const data = await pubsub.createTopic(generateName()); + topic = data[0]; + await topic.iam.setPolicy({ + bindings: [ + { + role: 'roles/pubsub.editor', + members: ['allUsers'], + }, + ], + }); }); after(() => {