Repository navigation
KeyringController: improve unit test coverage - #693
Conversation
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Hey @jpuri, sorry for the delay in reviewing this. I need to take a closer look but for now I've just added comments for style-related things. @Gudahtt You might want to take a look at this too.
Just a note about the TODOs. I am not a big fan of adding TODOs to code because I feel like it creates noise when reading through the code. Would you mind filing issues around problems you see? You can copy/paste the comments you've left if you like. That should allow us to have discussions about these things in a dedicated place and give visibility for @Gudahtt as he works through the keyring stuff.
|
Test coverage in KeyringController after PR:
|
Gudahtt
left a comment
Gudahtt
left a comment
There was a problem hiding this comment.
LGTM! Still lots of room for improvement but this seems like a clear step forward.
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Noticed one small thing, but it is minor. Looks good to me regardless.
|
I have done a small update, can you plz re-approve @adonesky1 , @mcmire , @Gudahtt |
Adding more unit test cases for keyring controller.