Refactor IndexedDB tests to use async/await functionality

[dsanders11]

Description

No functional change here, just cleaning up the tests with the cleaner await style.

BTW, keep an eye on DefinitelyTyped/DefinitelyTyped#30646. The current Jasmine version has support for expectAsync(...).toBeRejectedWith() which would make some of these expected failure tests cleaner (rather than a less than ideal fail('Unexpected success')), but they haven't been typed yet so the TS compile fails.

---

For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md

---

This change is 

[caisq]

Thanks for doing this, @dsanders11 !! See my comments below.

Reviewable status: 0 of 1 approvals obtained (waiting on @dsanders11)

---

src/io/indexed_db_test.ts, line 118 at r1 (raw file):

} catch (err) {

You can drop this try-catch block. If there are errors, it'll cause the it to fail anyway.

---

src/io/indexed_db_test.ts, line 161 at r1 (raw file):

} catch (err) {

Same as above. Drop the try-catch block.

---

src/io/indexed_db_test.ts, line 172 at r1 (raw file):

catch (err) {

No action required: this try-catch below should be kept, as it serves to examine the error message.

---

src/io/indexed_db_test.ts, line 203 at r1 (raw file):

} catch (err) {

Same as above. Drop the try-catch block.

---

src/io/indexed_db_test.ts, line 224 at r1 (raw file):

} catch (err) {

Same as above. Drop the try-catch block.

---

src/io/indexed_db_test.ts, line 258 at r1 (raw file):

 } catch (err) {

Same as above. Drop the try-catch block.

---

src/io/indexed_db_test.ts, line 280 at r1 (raw file):

    } catch (err) {
 

Same as above. Drop the try-catch block.

---

src/io/indexed_db_test.ts, line 305 at r1 (raw file):

} catch (err) {

Same as above. Drop the try-catch block.

[dsanders11]

@caisq, I was originally going to leave out the try-catch blocks, but noticed that it changes the behavior slightly. Catching and using fail(err.stack) will show the full stack if it fails (including the test filename, etc), as opposed to no try-catch block failing will only show the stack of where the error occurred in non-test code, so indexed_db.ts. Due to this behavior change, I opted to keep the fail(err.stack) so that there were zero changes in behavior/output in this PR. Not showing a stack including the test filename makes it harder to immediately know where the error is.

To illustrate, the first is the current fail behavior, the second is without the try-catch block.

Failed: Error: Cannot find model with path 'aQuxModel' in IndexedDB.
    at IDBRequest.getInfoRequest.onsuccess (src/io/indexed_db.ts:310:26 <- src/io/indexed_db.js:257:51)
    at <Jasmine>
    at Object.<anonymous> (src/io/indexed_db_test.ts:331:6 <- src/io/indexed_db_test.js:311:21)
    at step (src/io/indexed_db_test.js:32:23)
    at Object.throw (src/io/indexed_db_test.js:13:53)
    at rejected (src/io/indexed_db_test.js:5:65)

Error: Cannot find model with path 'aQuxModel' in IndexedDB.
    at IDBRequest.getInfoRequest.onsuccess (src/io/indexed_db.ts:310:26 <- src/io/indexed_db.js:257:51)

I think there's probably a change that can be made to the Karma/Jasmine config that would make the default stacktrace include the test file name, but I'm not familiar enough there to go digging.

If the change in error output is acceptable I can make the requested changes.

[caisq]

My vote is that the change in the failure message is acceptable. If someone's trying debug a test and want to see the full stack, they can insert try-catch blocks temporarily.

Reviewable status: 0 of 1 approvals obtained (waiting on @dsanders11)

[pe8ter]

A pattern that I use (until the Jasmine typings go in) is to assign the caught error to a local variable and test the expectation outside the catch block. I then use a custom lint rule to ban calls to expect with catch blocks (see shameless self-promotion).

It's a little more verbose than what you have but it keeps me from having to do a lot of bookkeeping.

[dsanders11]

@caisq, alright, I made those changes and force-pushed to update. This should be ready to merge. I'm not familiar with the Reviewable stuff, but tried to mark them as done on there.

[caisq]

@dsanders11 For future reference, no need for squash or force push. We squash all the commits when merging PRs.

Reviewable is pretty nice. I think everyone have access to it. If so, I recommend trying it out. It has better change display and commenting UI than the default GitHub one.

Reviewable status: 0 of 1 approvals obtained (waiting on @dsanders11)

[caisq]

@pe8ter Why is it a bad idea to have expect assertions in catch blocks`? I don't fully understand. Can you elaborate?

[caisq]

@pe8ter Ah - saw that in https://www.npmjs.com/package/pslint

I like that. But we can do that in a separate PR.

[caisq]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsanders11)

---

src/io/indexed_db_test.ts, line 167 at r2 (raw file):

      expect(err.message)
          .toEqual(

Actually, @dsanders11 , can you fix this by storing err in a variable and asserting the value of the variable outside this catch block? Same for another test below.

We can consider the linter that @pe8ter mentioned later.

[dsanders11]

@caisq, I'd prefer to leave it as-is at the moment. Reason being, as I mentioned in the comment at the start of this PR, there's a better way to do this in Jasmine, it just doesn't currently compile in TypeScript because it's not typed in the @types/jasmine package. Since that's being fixed, re-writing that test as you requested would only last for a couple weeks before the new method of writing the test is available and can be used. No point in re-writing it twice.

Here's what the two tests would look like once expectAsync is available:

expectAsync(handler.load).toBeRejectedWith({
    message: 'Cannot find model with path \'NonexistentModel\' in IndexedDB.'
});

And

expectAsync(async () => {
    await new BrowserIndexedDBManager().removeModel('nonexistent');
}).toBeRejectedWith({
    message: 'Cannot find model with path \'nonexistent\' in IndexedDB.'
});

[caisq]

Reviewable status: 0 of 1 approvals obtained (waiting on @dsanders11)