Skip to content
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

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

Merged
merged 10 commits into from
Mar 14, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1dd0508
refactor(storage-transfer): convert ava tests to mocha
nareshqlogic Feb 11, 2019
02a9435
Refactor code
nareshqlogic Feb 11, 2019
9c1bb2c
Increase timelimit
nareshqlogic Mar 5, 2019
846f982
Do not change await logic in before() during refactoring
fhinkel Mar 8, 2019
57fa580
Reasonable timeoutt
fhinkel Mar 8, 2019
f7acfc4
Don't return promise
fhinkel Mar 8, 2019
70f8064
Change test.after.always to afterEach
fhinkel Mar 14, 2019
715dfb2
Run single test, do not stub console
fhinkel Mar 14, 2019
24b7082
Merge branch 'master' into nareshqlogic-storage-avatomocha
fhinkel Mar 14, 2019
654090b
Await for bucket options to be added
fhinkel Mar 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions storage-transfer/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
"node": ">=8"
},
"scripts": {
"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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"system-test": "mocha system-test/*.test.js --timeout=10000",
"test": "repo-tools test install --cmd=npm -- run unit-test && repo-tools test install --cmd=npm -- run system-test"
},
"dependencies": {
"googleapis": "^38.0.0",
Expand All @@ -23,7 +23,7 @@
},
"devDependencies": {
"@google-cloud/nodejs-repo-tools": "^3.0.0",
"ava": "^0.25.0",
"mocha": "^6.0.0",
"proxyquire": "^2.1.0",
"sinon": "^7.2.7",
"@google-cloud/storage": "^2.3.3",
Expand Down
124 changes: 68 additions & 56 deletions storage-transfer/system-test/transfer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,49 +16,46 @@
/* eslint no-empty: 0 */
'use strict';

const {Storage} = require(`@google-cloud/storage`);
const {Storage} = require('@google-cloud/storage');
const storage = new Storage();
const test = require(`ava`);
const tools = require(`@google-cloud/nodejs-repo-tools`);
const uuid = require(`uuid`);
const assert = require('assert');
const tools = require('@google-cloud/nodejs-repo-tools');
const uuid = require('uuid');

const program = require(`../transfer`);
const program = require('../transfer');

const firstBucketName = `nodejs-docs-samples-test-${uuid.v4()}`;
const secondBucketName = `nodejs-docs-samples-test-${uuid.v4()}`;

let jobName;
const date = `2222/08/11`;
const time = `15:30`;
const description = `this is a test`;
const status = `DISABLED`;
const date = '2222/08/11';
const time = '15:30';
const description = 'this is a test';
const status = 'DISABLED';

test.before(tools.checkCredentials);
test.before(async () => {
before(async () => {
tools.checkCredentials();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tools.stubConsole();

const bucketOptions = {
entity: 'allUsers',
role: storage.acl.WRITER_ROLE,
};
await storage.createBucket(firstBucketName).then(data => {
const bucket = data[0];
return bucket.acl.add(bucketOptions);
});
await storage.createBucket(secondBucketName).then(data => {
const bucket = data[0];
return bucket.acl.add(bucketOptions);
});
const [bucket1] = await storage.createBucket(firstBucketName);
await bucket1.acl.add(bucketOptions);
const [bucket2] = await storage.createBucket(secondBucketName);
await bucket2.acl.add(bucketOptions);
});

test.after.always(async () => {
after(async () => {
tools.restoreConsole();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, we just dropped this

const bucketOne = storage.bucket(firstBucketName);
const bucketTwo = storage.bucket(secondBucketName);
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bucketOne.deleteFiles({force: true});
} catch (err) {} // ignore error
try {
// Intentially, try a second time.
bucketOne.deleteFiles({force: true});
} catch (err) {} // ignore error
try {
Expand All @@ -75,7 +72,7 @@ test.after.always(async () => {
} catch (err) {} // ignore error
});

test.cb.serial(`should create a storage transfer job`, t => {
it('should create a storage transfer job', done => {
const options = {
srcBucket: firstBucketName,
destBucket: secondBucketName,
Expand All @@ -85,65 +82,80 @@ test.cb.serial(`should create a storage transfer job`, t => {
};

program.createTransferJob(options, (err, transferJob) => {
t.ifError(err);
assert.ifError(err);
jobName = transferJob.name;
t.is(transferJob.name.indexOf(`transferJobs/`), 0);
t.is(transferJob.description, description);
t.is(transferJob.status, `ENABLED`);
t.true(
console.log.calledWith(`Created transfer job: %s`, transferJob.name)
assert.strictEqual(transferJob.name.indexOf('transferJobs/'), 0);
assert.strictEqual(transferJob.description, description);
assert.strictEqual(transferJob.status, 'ENABLED');
assert.strictEqual(
console.log.calledWith('Created transfer job: %s', transferJob.name),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

true
);
setTimeout(t.end, 2000);
done();
});
});

test.cb.serial(`should get a transferJob`, t => {
it('should get a transferJob', done => {
program.getTransferJob(jobName, (err, transferJob) => {
t.ifError(err);
t.is(transferJob.name, jobName);
t.is(transferJob.description, description);
t.is(transferJob.status, `ENABLED`);
t.true(console.log.calledWith(`Found transfer job: %s`, transferJob.name));
setTimeout(t.end, 2000);
assert.ifError(err);
assert.strictEqual(transferJob.name, jobName);
assert.strictEqual(transferJob.description, description);
assert.strictEqual(transferJob.status, 'ENABLED');
assert.strictEqual(
console.log.calledWith('Found transfer job: %s', transferJob.name),
true
);
done();
});
});

test.cb.serial(`should update a transferJob`, t => {
it('should update a transferJob', done => {
var options = {
job: jobName,
field: `status`,
field: 'status',
value: status,
};

program.updateTransferJob(options, (err, transferJob) => {
t.ifError(err);
t.is(transferJob.name, jobName);
t.is(transferJob.description, description);
t.is(transferJob.status, status);
t.true(
console.log.calledWith(`Updated transfer job: %s`, transferJob.name)
assert.ifError(err);
assert.strictEqual(transferJob.name, jobName);
assert.strictEqual(transferJob.description, description);
assert.strictEqual(transferJob.status, status);
assert.strictEqual(
console.log.calledWith('Updated transfer job: %s', transferJob.name),
true
);
setTimeout(t.end, 2000);
done();
});
});

test.cb.serial(`should list transferJobs`, t => {
it('should list transferJobs', done => {
program.listTransferJobs((err, transferJobs) => {
t.ifError(err);
t.true(transferJobs.some(transferJob => transferJob.name === jobName));
t.true(
transferJobs.some(transferJob => transferJob.description === description)
assert.ifError(err);
assert.strictEqual(
transferJobs.some(transferJob => transferJob.name === jobName),
true
);
assert.strictEqual(
transferJobs.some(transferJob => transferJob.description === description),
true
);
assert.strictEqual(
transferJobs.some(transferJob => transferJob.status === status),
true
);
assert.strictEqual(
console.log.calledWith('Found %d jobs!', transferJobs.length),
true
);
t.true(transferJobs.some(transferJob => transferJob.status === status));
t.true(console.log.calledWith(`Found %d jobs!`, transferJobs.length));
setTimeout(t.end, 2000);
done();
});
});

test.cb.serial(`should list transferJobs`, t => {
it('should list transferOperations', done => {
program.listTransferOperations(jobName, (err, operations) => {
t.ifError(err);
t.true(Array.isArray(operations));
t.end();
assert.ifError(err);
assert.strictEqual(Array.isArray(operations), true);
done();
});
});
Loading