Allow failing of individual transactions #3052
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/individualfailures
branch
from
6ca4910 to
8b257f9
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/individualfailures
branch
from
16db9ba to
2d869e5
Compare
Binary Size ReportAffected SDKs
Test Logs |
| ); | ||
| } else { | ||
| return super.runTransaction(action, mode, transactionOperation); | ||
| if (this.injectFailures[action as PersistenceAction] === undefined) { |
There was a problem hiding this comment.
Consider saving the result of this.injectFailures[action as PersistenceAction] to a local variable instead of accessing it twice via this long dictionary access syntax.
| } | ||
| if (this.injectFailures[action as PersistenceAction]) { | ||
| return Promise.reject( | ||
| new IndexedDbTransactionError(new Error('Simulated retryable error')) |
There was a problem hiding this comment.
Consider including the value of action in the error message. This may assist with debugging to figure out which action caused the exception to be thrown.
There was a problem hiding this comment.
Good idea. Future me will thank you as I am debugging test assertions.
| ); | ||
| } else { | ||
| return super.runTransaction(action, mode, transactionOperation); | ||
| if (this.injectFailures[action as PersistenceAction] === undefined) { |
There was a problem hiding this comment.
If this block of code is an identical copy/paste from above, then consider moving it to a helper function.
There was a problem hiding this comment.
It's not identical because I only updated one of the two blocks to use "else if". But it should be identical... Moved it to a helper function.
| ); | ||
| } else { | ||
| return super.runTransaction(action, mode, transactionOperation); | ||
| if (this.injectFailures[action as PersistenceAction] === undefined) { |
There was a problem hiding this comment.
It looks like the Android and iOS spec test framework will need to be updated to handle this change in the generated JSON. Are you planning to do that as well?
There was a problem hiding this comment.
Yes, we need to make those changes there too. For now, the "recovery" tests are marked no-ios and no-android. We won't need to update the runner until we remove these tags and implement LevelDB/SQLite recovery.
There was a problem hiding this comment.
Ahh you're right. I missed the no-ios and no-android tags... even though I was looking for them 🤔
| throw fail('Failure mode not specified for action: ' + action); | ||
| } else if (shouldFail) { | ||
| throw new IndexedDbTransactionError( | ||
| new Error('Simulated retryable error' + action) |
There was a problem hiding this comment.
nit: Add a space or colon after "error" so that the action text is not mashed into the word "error".
There was a problem hiding this comment.
Not a nit but a crucial typo fix :) Done
| ); | ||
| } else { | ||
| return super.runTransaction(action, mode, transactionOperation); | ||
| if (this.injectFailures[action as PersistenceAction] === undefined) { |
There was a problem hiding this comment.
Ahh you're right. I missed the no-ios and no-android tags... even though I was looking for them 🤔
Disclaimer: This is pretty ugly... feedback appreciated :)
The idea of the PR is to expand the new "failDatabase" spec test feature to allow the tests to specify which transaction action to fail. This allows individualized failures for operations that spawn multiple actions. Currently, we only test scenarios where the first transaction fails. This will allows us to test what happens if the first transaction succeeds and the second one fails.