-
Notifications
You must be signed in to change notification settings - Fork 943
Refactor IndexedDB tests to use async/await functionality #1409
Conversation
There was a problem hiding this 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.
@caisq, I was originally going to leave out the try-catch blocks, but noticed that it changes the behavior slightly. Catching and using 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. |
There was a problem hiding this 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)
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 It's a little more verbose than what you have but it keeps me from having to do a lot of bookkeeping. |
6a3ffe8
to
21773ab
Compare
@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. |
There was a problem hiding this 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)
@pe8ter Why is it a bad idea to have |
@pe8ter Ah - saw that in https://www.npmjs.com/package/pslint I like that. But we can do that in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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 Here's what the two tests would look like once 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.'
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 approvals obtained (waiting on @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 idealfail('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