Add test coverage for `lib/table.js` #178

rmeritz · 2020-06-04T17:54:00Z

Add 100% test coverage
Follow existing pattern of more "integration" style test where each operation that has its own test file and the file tests the in results of interacting with the fake API directly (as opposed to a true unit test that would mock out Tables's interactions with Record and Query)
The select, update, and create methods were already pretty extensively tested using hard coded return values in the mock API express app. I added tests for error cases using the existing "handler override" functionality. Since the "handler override" is only resets on tearDownAsync all cleanup and setup needs to be down in these test files on each test. This probably makes the test slower. I could instead add some mechanism to reset the handler after each run.

- Is there a better way to rest the 'handler override' than switching to beforeEach/afterEach?

jugglinmike · 2020-06-04T18:06:27Z

Taking a look now!

jugglinmike

This looks great, Rebecca! I especially appreciate the effort you've made to use the existing infrastructure while preserving the readability of your tests. (I can't find any online resources describing the "mystery guest" problem, though...)

Anyway, I've made a few specific suggestions. Here are some ideas for a few more tests:

forEach - no results are found
forEach - an error is thrown in the first callback
forEach - an error is thrown in the second callback
eachPage - when there are multiple pages

As a general extension to the HTTP error tests: could you verify that each statusCode you choose is being defined on the error object? That seems important for the developers who are debugging problems.

jugglinmike · 2020-06-04T18:14:37Z

test/create.test.js

+                },
+                {typecast: true}
+            )
+            .catch(function(err) {


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

That seems useful for future debugging. Done.

jugglinmike · 2020-06-04T18:45:57Z

test/find.test.js

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


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.

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.

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

jugglinmike · 2020-06-04T18:46:44Z

test/find.test.js

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


(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.)

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

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

test/list.test.js

jugglinmike · 2020-06-04T18:54:06Z

test/list.test.js

+                {opts: 'arepassedalong'},
+                function(record) {
+                    expect(record.getId()).toBe('recordA');
+                    expect(record.get('Name')).toBe('Rebecca');


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?

Makes sense. Done.

jugglinmike · 2020-06-04T18:57:43Z

test/list.test.js

+                    fields: {Name: 'Rebecca'},
+                    createdTime: '2020-04-20T16:20:00.000Z',
+                },
+            ],


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

Makes sense. Done.

jugglinmike · 2020-06-04T19:04:51Z

test/list.test.js

+            .base('app123')
+            .table('Table')
+            .forEach(
+                {opts: 'arepassedalong'},


This is a bit distracting for this particular test

Yep. Removed!

jugglinmike · 2020-06-04T19:10:35Z

test/select.test.js

+            .table('Table')
+            .select()
+            .eachPage(function page(records) {
+                records.forEach(function(record) {


This will also pass for any number of records. Could you verify records.length?

jugglinmike · 2020-06-04T19:15:18Z

test/select.test.js

+            airtable
+                .base('app123')
+                .table('Table')
+                .select({maxRecords: 'should not be a string'});


This will pass if the calls to base or table throw an error. Unlikely, sure, but still worth taking those parts of the chain outside of the assertion function.

I added matching of the specific error messages. I think this doesn't exactly address you point. But it does make it more clear that the errors I'm seen being throw are the ones I'm trying to throw. Is this a good enough way to address the issue?

Yeah, that works for me

rmeritz · 2020-06-04T19:29:32Z

Thanks for all the feedback.

(I can't find any online resources describing the "mystery guest" problem, though...)

Btw, I think this is the original source of the term: http://xunitpatterns.com/Obscure%20Test.html#Mystery%20Guest

rmeritz · 2020-06-04T19:47:49Z

As a general extension to the HTTP error tests: could you verify that each statusCode you choose is being defined on the error object? That seems important for the developers who are debugging problems.

That makes sense. Done.

- Add tests with more than one record - Add test with no records - Add iteration counter to ensure all expectations are run

rmeritz · 2020-06-04T21:00:40Z

I think it makes sense to skip the eachPage - when there are multiple pages test for now because that starts to dive into the details of Query internals in a serious way. I think we can cover it when we do tests for lib/query.js.

To test that errors are thrown in tests. This way if tests unexpectly fufill the promise they immediately error instead of just timeing out.

rmeritz · 2020-06-05T17:32:39Z

forEach - an error is thrown in the first callback
forEach - an error is thrown in the second callback

I'm skipping adding these tests because the code doesn't handle a error be thrown in the callback at all it just bubbled up to the global event handler. I might want to change the behavior so that such an error is handled, but this is for an already deprecated function, so I don't think it makes sense to address that behavior change.

jugglinmike

Wondrous!

Please note that in this context, my approval carries even less weight than it normally does. We'll need someone at Airtable to merge, perhaps @kasrak.

rmeritz · 2020-06-10T21:01:14Z

@kasrak - A lot of the test changes are interconnected, so for record tests I ended up branching of this branch. I have another branch ready for lib/records.js that I will open a PR for once this is merged: bocoup/airtable.js@table-tests-rmeritz...bocoup:record-tests-rmeritz (Probably will start something similar for query.js too, but will try and do some unrelated tests first).

kasrak

Thanks!

rmeritz added 6 commits June 3, 2020 11:35

Add tests initial tests for table.js

445a442

Add Table#select tests

49ad1af

Add table tests for handling of error cases

02521f1

- Is there a better way to rest the 'handler override' than switching to beforeEach/afterEach?

Add tests to Table#list

45d8a13

Add tests for Table#forEach

a27c134

Remove unused fetchNextPage from select tests

a0376dc

jugglinmike suggested changes Jun 4, 2020

View reviewed changes

rmeritz added 3 commits June 4, 2020 12:47

Improve error tests by adding status and msg check

7dea2b0

Improve forEach tests

1fd852b

- Add tests with more than one record - Add test with no records - Add iteration counter to ensure all expectations are run

Ensure testing correct errors in select test

5ae49bf

Use then with an error statement instead of catch

0550425

To test that errors are thrown in tests. This way if tests unexpectly fufill the promise they immediately error instead of just timeing out.

jugglinmike approved these changes Jun 9, 2020

View reviewed changes

kasrak approved these changes Jun 10, 2020

View reviewed changes

kasrak merged commit cb41a4f into Airtable:master Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test coverage for `lib/table.js` #178

Add test coverage for `lib/table.js` #178

rmeritz commented Jun 4, 2020

jugglinmike commented Jun 4, 2020

jugglinmike left a comment

jugglinmike Jun 4, 2020

rmeritz Jun 4, 2020

jugglinmike Jun 4, 2020

rmeritz Jun 4, 2020

jugglinmike Jun 4, 2020

jugglinmike Jun 4, 2020

rmeritz Jun 4, 2020

rmeritz Jun 4, 2020

jugglinmike Jun 4, 2020

rmeritz Jun 4, 2020

jugglinmike Jun 4, 2020

rmeritz Jun 4, 2020

jugglinmike Jun 4, 2020

rmeritz Jun 4, 2020

jugglinmike Jun 4, 2020

rmeritz Jun 4, 2020

jugglinmike Jun 4, 2020

rmeritz Jun 4, 2020

jugglinmike Jun 4, 2020

rmeritz commented Jun 4, 2020

rmeritz commented Jun 4, 2020

rmeritz commented Jun 4, 2020 •

edited

Loading

rmeritz commented Jun 5, 2020

jugglinmike left a comment

rmeritz commented Jun 10, 2020

kasrak left a comment

Add test coverage for lib/table.js #178

Add test coverage for lib/table.js #178

Conversation

rmeritz commented Jun 4, 2020

jugglinmike commented Jun 4, 2020

jugglinmike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmeritz commented Jun 4, 2020

rmeritz commented Jun 4, 2020

rmeritz commented Jun 4, 2020 • edited Loading

rmeritz commented Jun 5, 2020

jugglinmike left a comment

Choose a reason for hiding this comment

rmeritz commented Jun 10, 2020

kasrak left a comment

Choose a reason for hiding this comment

Add test coverage for `lib/table.js` #178

Add test coverage for `lib/table.js` #178

rmeritz commented Jun 4, 2020 •

edited

Loading