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

Add test coverage for lib/table.js #178

Merged
merged 10 commits into from
Jun 10, 2020
Merged

Add test coverage for lib/table.js #178

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
445a442
Add tests initial tests for table.js
rmeritz Jun 3, 2020
49ad1af
Add Table#select tests
rmeritz Jun 3, 2020
02521f1
Add table tests for handling of error cases
rmeritz Jun 3, 2020
45d8a13
Add tests to Table#list
rmeritz Jun 4, 2020
a27c134
Add tests for Table#forEach
rmeritz Jun 4, 2020
a0376dc
Remove unused fetchNextPage from select tests
rmeritz Jun 4, 2020
7dea2b0
Improve error tests by adding status and msg check
rmeritz Jun 4, 2020
1fd852b
Improve forEach tests
rmeritz Jun 4, 2020
5ae49bf
Ensure testing correct errors in select test
rmeritz Jun 4, 2020
0550425
Use then with an error statement instead of catch
rmeritz Jun 4, 2020
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
1 change: 0 additions & 1 deletion lib/table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
// istanbul ignore file
'use strict';

var isArray = require('lodash/isArray');
Expand Down
29 changes: 27 additions & 2 deletions test/create.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,18 @@ var testHelpers = require('./test_helpers');

describe('record creation', function() {
var airtable;
var testExpressApp;
var teardownAsync;

beforeAll(function() {
beforeEach(function() {
return testHelpers.getMockEnvironmentAsync().then(function(env) {
airtable = env.airtable;
testExpressApp = env.testExpressApp;
teardownAsync = env.teardownAsync;
});
});

afterAll(function() {
afterEach(function() {
return teardownAsync();
});

Expand Down Expand Up @@ -51,6 +53,29 @@ describe('record creation', function() {
);
});

it('can throw an error if create fails', function(done) {
testExpressApp.set('handler override', function(req, res) {
res.status(402).json({
error: {message: 'foo bar'},
});
});

return airtable
.base('app123')
.table('Table')
.create(
{
foo: 'boo',
bar: 'yar',
},
{typecast: true}
)
.catch(function(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test will time out if the promise returned by the library is fulfilled. To make reporting a little faster and more descriptive, we could do something like...

-  .catch(function(err) {
+  .then(function() {
+      throw new Error('Promise fulfilled unexpectedly');
+    }, function(err) {
       expect(err).not.toBeNull();
-      done();
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems useful for future debugging. Done.

expect(err).not.toBeNull();
done();
});
});

it('can add the "typecast" parameter when creating one record', function() {
return airtable
.base('app123')
Expand Down
23 changes: 21 additions & 2 deletions test/delete.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,18 @@ var testHelpers = require('./test_helpers');

describe('record deletion', function() {
var airtable;
var testExpressApp;
var teardownAsync;

beforeAll(function() {
beforeEach(function() {
return testHelpers.getMockEnvironmentAsync().then(function(env) {
airtable = env.airtable;
testExpressApp = env.testExpressApp;
teardownAsync = env.teardownAsync;
});
});

afterAll(function() {
afterEach(function() {
return teardownAsync();
});

Expand Down Expand Up @@ -50,6 +52,23 @@ describe('record deletion', function() {
});
});

it('can throw an error if delete fails', function(done) {
testExpressApp.set('handler override', function(req, res) {
res.status(402).json({
error: {message: 'foo bar'},
});
});

return airtable
.base('app123')
.table('Table')
.destroy(['rec123', 'rec456'])
.catch(function(err) {
expect(err).not.toBeNull();
done();
});
});

it('can delete multiple records and call a callback', function(done) {
airtable
.base('app123')
Expand Down
44 changes: 44 additions & 0 deletions test/find.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict';

var testHelpers = require('./test_helpers');

describe('record retrival', function() {
var airtable;
var testExpressApp;
var teardownAsync;

beforeAll(function() {
return testHelpers.getMockEnvironmentAsync().then(function(env) {
airtable = env.airtable;
testExpressApp = env.testExpressApp;
teardownAsync = env.teardownAsync;
});
});

afterAll(function() {
return teardownAsync();
});

it('can find one record', function() {
var recordId = 'record1';

testExpressApp.set('handler override', function(req, res) {
expect(req.method).toBe('GET');
expect(req.url).toBe('/v0/app123/Table/record1?');
Copy link
Contributor

Choose a reason for hiding this comment

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

(The trailing ? is a little fragile. Ideally the library shouldn't add it, but it doesn't matter if it's there or not. There are ways we could address that, but we wouldn't need them if the library didn't do this. Probably best to accept this fragility and plan to change the library later.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be fine switching to toMatch https://jestjs.io/docs/en/expect.html#tomatchregexporstring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm switch to match seem unideal too. I'm just going to leave it as is for now.

res.json({
id: req.params.recordId,
fields: {Name: 'Rebecca'},
createdTime: '2020-04-20T16:20:00.000Z',
Copy link
Contributor

Choose a reason for hiding this comment

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

What is createdTime? It seemed like something that we should test, but there doesn't appear to be any reference to it in the application code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is returned by the API so I thought I would include it to make the tests more realistic, but there aren't any methods provided by the library to access it. (Except through the Record#_rawJson which is clearly meant to be private.

I could see an argument to removing it because maybe it obscures the test more by providing extraneous information. But I also see an argument of leaving it in because I imagine it might be exposed in the future and it provides a realistic understanding of the format of the json returned from the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend toward the "extraneous information" interpretation, but I'm happy to go with your preference here, particularly because you're following precedence.

});
});

return airtable
.base('app123')
.table('Table')
.find(recordId)
.then(function(foundRecord) {
expect(foundRecord.id).toBe(recordId);
expect(foundRecord.get('Name')).toBe('Rebecca');
});
});
});
229 changes: 229 additions & 0 deletions test/list.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
'use strict';

var testHelpers = require('./test_helpers');

describe('list records', function() {
var airtable;
var testExpressApp;
var teardownAsync;

beforeAll(function() {
return testHelpers.getMockEnvironmentAsync().then(function(env) {
airtable = env.airtable;
testExpressApp = env.testExpressApp;
teardownAsync = env.teardownAsync;
});
});

afterAll(function() {
return teardownAsync();
});

it('lists records with a limit and offset without opts', function(done) {
testExpressApp.set('handler override', function(req, res) {
expect(req.method).toBe('GET');
expect(req.url).toBe('/v0/app123/Table/?limit=50&offset=offset000');
res.json({
records: [
{
id: 'recordA',
fields: {Name: 'Rebecca'},
createdTime: '2020-04-20T16:20:00.000Z',
},
],
offset: 'offsetABC',
});
});

return airtable
.base('app123')
.table('Table')
.list(50, 'offset000', function(err, records, offset) {
expect(err).toBeNull();
expect(records.length).toBe(1);
expect(records[0].getId()).toBe('recordA');
expect(records[0].get('Name')).toBe('Rebecca');
expect(offset).toBe('offsetABC');
done();
});
});

it('lists records with a limit, offset, and opts', function(done) {
testExpressApp.set('handler override', function(req, res) {
expect(req.method).toBe('GET');
expect(req.url).toBe(
'/v0/app123/Table/?limit=50&offset=offset000&otherOptions=getpassedalong'
);
res.json({
records: [
{
id: 'recordA',
fields: {Name: 'Rebecca'},
createdTime: '2020-04-20T16:20:00.000Z',
},
],
offset: 'offsetABC',
});
});

return airtable
.base('app123')
.table('Table')
.list(50, 'offset000', {otherOptions: 'getpassedalong'}, function(
err,
records,
offset
) {
expect(err).toBeNull();
expect(records.length).toBe(1);
expect(records[0].getId()).toBe('recordA');
expect(records[0].get('Name')).toBe('Rebecca');
expect(offset).toBe('offsetABC');
done();
});
});

it('can throw an error if list fails', function(done) {
testExpressApp.set('handler override', function(req, res) {
res.status(402).json({
error: {message: 'foo bar'},
});
});

return airtable
.base('app123')
.table('Table')
.list(50, 'offset000', function(err, records, offset) {
expect(err).not.toBeNull();
expect(records).toBeUndefined();
expect(offset).toBeUndefined();
done();
});
});

it('iterates through each record with opts', function(done) {
var json = {
records: [
{
id: 'recordA',
fields: {Name: 'Rebecca'},
createdTime: '2020-04-20T16:20:00.000Z',
},
],
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a second record so we can verify that the library is truly iterating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Done.

};

testExpressApp.set('handler override', function(req, res) {
expect(req.method).toBe('GET');
expect(req.url).toBe('/v0/app123/Table/?limit=100&opts=arepassedalong');
res.json(json);
});

return airtable
.base('app123')
.table('Table')
.forEach(
{opts: 'arepassedalong'},
rmeritz marked this conversation as resolved.
Show resolved Hide resolved
function(record) {
expect(record.getId()).toBe('recordA');
expect(record.get('Name')).toBe('Rebecca');
Copy link
Contributor

Choose a reason for hiding this comment

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

This test will pass if the callback is never invoked. Could you increment a counter in this function and then verify its value in the second callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Done.

},
function() {
done();
}
);
});

it('iterates through each record without opts', function(done) {
var json = {
records: [
{
id: 'recordA',
fields: {Name: 'Rebecca'},
createdTime: '2020-04-20T16:20:00.000Z',
},
],
};

testExpressApp.set('handler override', function(req, res) {
expect(req.method).toBe('GET');
expect(req.url).toBe('/v0/app123/Table/?limit=100');
res.json(json);
});

return airtable
.base('app123')
.table('Table')
.forEach(
function(record) {
expect(record.getId()).toBe('recordA');
expect(record.get('Name')).toBe('Rebecca');
},
function() {
done();
}
);
});

it('can throw an error if forEach fails', function(done) {
testExpressApp.set('handler override', function(req, res) {
res.status(402).json({
error: {message: 'foo bar'},
});
});

return airtable
.base('app123')
.table('Table')
.forEach(
{opts: 'arepassedalong'},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit distracting for this particular test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Removed!

function() {},
function(err) {
expect(err).not.toBeNull();
done();
}
);
});

it('iterates through each record and handles pagination', function(done) {
var json = {
records: [
{
id: 'recordA',
fields: {Name: 'Rebecca'},
createdTime: '2020-04-20T16:20:00.000Z',
},
],
offset: 'offset123',
};

testExpressApp.set('handler override', function(req, res) {
expect(req.method).toBe('GET');
expect(req.url).toBe('/v0/app123/Table/?limit=100&opts=arepassedalong');
res.json(json);
});

return airtable
.base('app123')
.table('Table')
.forEach(
{opts: 'arepassedalong'},
function(record) {
expect(record.getId()).toBe('recordA');
expect(record.get('Name')).toBe('Rebecca');

// Reset API response to handle the paginated call
testExpressApp.set('handler override', function(req, res) {
expect(req.method).toBe('GET');
expect(req.url).toBe(
'/v0/app123/Table/?limit=100&offset=offset123&opts=arepassedalong'
);
delete json.offset;
res.json(json);
});
},
function() {
done();
}
);
});
});
Loading