New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(storage-transfer): convert ava tests to mocha #1202

Merged

fhinkel merged 10 commits into master from nareshqlogic-storage-avatomocha

Mar 14, 2019

Contributor

fhinkel commented Mar 8, 2019

See failing tests in #1138

googlebot commented Mar 8, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot added the cla: no label

fhinkel added the kokoro:run label

kokoro-team removed the kokoro:run label

fhinkel added cla: yes kokoro:run and removed cla: no labels

googlebot commented Mar 8, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

kokoro-team removed kokoro:run labels

googlebot commented Mar 8, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot added cla: no and removed cla: yes labels

fhinkel added cla: yes and removed cla: no labels

googlebot commented Mar 8, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

fhinkel added the do not merge label

googlebot commented Mar 8, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot added cla: no and removed cla: yes labels

fhinkel added the kokoro:run label

kokoro-team removed the kokoro:run label

fhinkel added cla: yes and removed cla: no labels

googlebot commented Mar 8, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot commented Mar 8, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot added cla: no and removed cla: yes labels

fhinkel added cla: yes kokoro:run labels

fhinkel added cla: yes kokoro:run and removed cla: no labels

kokoro-team removed the kokoro:run label

googlebot commented Mar 14, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

fhinkel requested a review from JustinBeckwith

March 14, 2019 16:26

fhinkel force-pushed the nareshqlogic-storage-avatomocha branch from 5bf58df to e72fcfa Compare

March 14, 2019 16:33

googlebot commented Mar 14, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot added cla: no and removed cla: yes labels

fhinkel added cla: yes kokoro:run and removed cla: no labels

kokoro-team removed the kokoro:run label

googlebot commented Mar 14, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

fhinkel force-pushed the nareshqlogic-storage-avatomocha branch from e72fcfa to 7bf04bd Compare

March 14, 2019 17:00

googlebot commented Mar 14, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot added cla: no and removed cla: yes labels


          Await for bucket options to be added

654090b

fhinkel force-pushed the nareshqlogic-storage-avatomocha branch from 7bf04bd to 654090b Compare

March 14, 2019 17:37

fhinkel added cla: yes kokoro:run and removed cla: no labels

kokoro-team removed the kokoro:run label

googlebot commented Mar 14, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

JustinBeckwith approved these changes

View reviewed changes

storage-transfer/package.json

-                  "unit-test": "repo-tools test run --cmd ava -- -T 20s --verbose test/*.test.js",
-                  "system-test": "repo-tools test run --cmd ava -- -T 20s --verbose system-test/*.test.js",
-                  "test": "npm run unit-test && npm run system-test"
+                  "unit-test": "mocha test/*.test.js --timeout=10000",

Contributor

JustinBeckwith Mar 14, 2019

So no need to block on this feedback, but I would rather just use mocha --timeout=10000 across the board here. That way if a new test gets added to the test directory, you don't have to worry about it having the right prefix.

Contributor Author

fhinkel Mar 14, 2019

Happy to change it, I'll do it in a separate PR for all the folders.

storage-transfer/system-test/transfer.test.js

    
              test.before(tools.checkCredentials);

              test.before(async () => {

              before(async () => {

                tools.checkCredentials();

Contributor

JustinBeckwith Mar 14, 2019

We have just dropped that line from all the samples, and the console stubbing. It's fine. Should we drop it here too?

storage-transfer/system-test/transfer.test.js

               });
-              test.after.always(async () => {
+              after(async () => {
                 tools.restoreConsole();

Contributor

JustinBeckwith Mar 14, 2019

Same as above, we just dropped this

storage-transfer/system-test/transfer.test.js

               });
-              test.after.always(async () => {
+              after(async () => {
                 tools.restoreConsole();
                 const bucketOne = storage.bucket(firstBucketName);
                 const bucketTwo = storage.bucket(secondBucketName);
                 try {

Contributor

JustinBeckwith Mar 14, 2019

Just for readability, could this be:

// Attempt to do the delete twice, and swallow any errors
for (let i=0; i<2; i++) {
  await bucketOne.deleteFiles({force: true}).catch(console.warn);
}

Contributor Author

fhinkel Mar 14, 2019

Don't you dare adding awaits, that breaks everything. Trust me, we've been there.

storage-transfer/system-test/transfer.test.js

+                  assert.strictEqual(transferJob.description, description);
+                  assert.strictEqual(transferJob.status, 'ENABLED');
+                  assert.strictEqual(
+                    console.log.calledWith('Created transfer job: %s', transferJob.name),

Contributor

JustinBeckwith Mar 14, 2019

Ahhh, I see why the console.log stubbing now.

fhinkel merged commit f44a579 into master

fhinkel deleted the nareshqlogic-storage-avatomocha branch

March 14, 2019 18:58

Contributor

AVaksman commented Apr 3, 2019

towards #1080

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels