From 86d0b816701135b322556c8573a448e80c85026c Mon Sep 17 00:00:00 2001 From: Denis DelGrosso <85250797+ddelgrosso1@users.noreply.github.com> Date: Wed, 6 Mar 2024 09:55:39 -0500 Subject: [PATCH] test: improvements to requester pays tests and bucket metadata updates (#2414) * test: improvements to requester pays tests and bucket metadata updates test: improvements to requester pays tests and bucket metadata updates * remove console.log statements * adjust wait time slightly * add retries to failed tests --- system-test/storage.ts | 128 +++++++++++++++++++---------------------- 1 file changed, 60 insertions(+), 68 deletions(-) diff --git a/system-test/storage.ts b/system-test/storage.ts index 9ea87526a..8802331f3 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') @@ -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(() => { @@ -231,8 +229,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 +296,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 +322,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 +1733,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 +1785,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 +1802,15 @@ 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 => { + 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);