Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Refactor IndexedDB tests to use async/await functionality #1409

Merged
merged 1 commit into from
Nov 22, 2018

Conversation

dsanders11
Copy link
Contributor

@dsanders11 dsanders11 commented Nov 20, 2018

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 Reviewable

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@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.

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

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
Copy link

pe8ter commented Nov 20, 2018

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
Copy link
Contributor Author

@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.

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Collaborator

caisq commented Nov 20, 2018

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

@caisq
Copy link
Collaborator

caisq commented Nov 20, 2018

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

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

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: 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
Copy link
Contributor Author

dsanders11 commented Nov 22, 2018

@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.'
});

Copy link
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

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

@caisq caisq merged commit 2da747a into tensorflow:master Nov 22, 2018
@dsanders11 dsanders11 deleted the async-indexeddb-test branch August 23, 2024 06:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants