Dapp visibility save/load tests #4150
Conversation
|
Would be nice to have tests that actually call the |
ngotchac
added
A5-grumble 🔥
|
Yes, was not meant to be a comprehensive suite test at all. (Just the slice where issues were found previously). |
|
Extended read/write tests (non-previously encountered bugs, pre-emptive) |
jacogr
added
A0-pleasereview 🤓
| let globalFetch; | ||
|
|
||
| function stubGlobals () { | ||
| globalContractsGet = Contracts.get; |
There was a problem hiding this comment.
Why not use a sinon sandbox here?
There was a problem hiding this comment.
Don't use it anywhere. Stubs are called and restored in one place setup right up front, so no need to go down the unknown road.
There was a problem hiding this comment.
it's using another properly documented, tested feature of a library that we already use instead of a homegrown solution which fails if stubGlobals gets called twice without a restoreGlobals in between.
|
|
||
| global.fetch = (url) => { | ||
| switch (url) { | ||
| case '/api/apps': |
There was a problem hiding this comment.
You might want to use sinon stubs here.
There was a problem hiding this comment.
Not yet. First ensure nothjng else is called, hence the default case with logging. At the same time need to see how the local tests develop, would need a variety of returns.
(Wish I could do everything at once for the full store, however just had to do the tests to verify an issue. Anybody with time and a non-overfull queue can jump in.)
There was a problem hiding this comment.
In this case and in general, I don't have strong opinions about sinon stubs so do whatever you prefer.
| let api; | ||
| let store; | ||
|
|
||
| function create () { |
There was a problem hiding this comment.
[tiny grumble] Would prefer a more meaningful name here.
Why did you make api and store global?
There was a problem hiding this comment.
Alternatives are to moving it in the describe block & then using putting create in there as well. Or using a beforeEach to set it with results, which means we need to call multiple times or always assign when created. Overall, since it local to the spec, this is concise. (api however is not really used yet, however as the tests expand, will be - see other store implemehtations.)
|
|
||
| it('disables visibility', () => { | ||
| store.hideApp(APPID_DAPPREG); | ||
| store.writeDisplayApps(); |
There was a problem hiding this comment.
This isn't needed write ? The write to localStorage should be automatic right?
ngotchac
added
A6-mustntgrumble 💦
Tests to verify save/load works as expected
(Now able to replicate https://github.com/ethcore/parity/issues/4147 via testing, if it does occur)