Issue 5203041586249728: Issue 2040 - Adapted tests for chrome.storage.local (Closed)
|
Created:
Feb. 26, 2015, 12:12 p.m. by Sebastian Noack Modified:
April 20, 2015, 3:40 p.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 2040 - Adapted tests for chrome.storage.local
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed comments #MessagesTotal messages: 4
http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... File chrome/content/tests/prefs.js (left): http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:100: equal(prefExists("notificationdata"), false, "User-defined pref has been removed"); Do we really need to make sure that the item is removed from the backend storage, if the pref gets reverted to an empty object? I removed that test and the respective logic from the Prefs implementation in the platform module because: 1. There is no simple way to compare objects. The old code merely compared there JSON representation, which only works relaibly if one of the objects is empty. However, the new implementation doesn't need to decode/encode JSON anymore. 2. At least in the platform module, "notificationdata" and "stats_total" are the only prefs using objects. And both are never reverted to an empty object.
http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... File chrome/content/tests/prefs.js (left): http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:100: equal(prefExists("notificationdata"), false, "User-defined pref has been removed"); On 2015/02/26 12:52:38, Sebastian Noack wrote: > Do we really need to make sure that the item is removed from the backend > storage, if the pref gets reverted to an empty object? The point of this test isn't really ensuring that the preference is removed, rather making sure that it is created even if it didn't exist before. As such, it is a prerequisite for the following checks. http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... File chrome/content/tests/prefs.js (right): http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:18: if ("chrome" in window) Looks like this test doesn't consider Safari. Is there already an issue to get the Safari implementation tested as well? http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:21: let key = "pref:" + name; Nit: no smart tabs please. http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:22: chrome.storage.local.get([key], function(items) According to documentation, passing in key without wrapping it in an array should work as well. http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:39: let key = "pref:" + name; Nit: no smart tabs please. http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:40: chrome.storage.local.get([key], function(items) As above, passing in key without wrapping it in an array should work as well.
http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... File chrome/content/tests/prefs.js (left): http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:100: equal(prefExists("notificationdata"), false, "User-defined pref has been removed"); On 2015/02/26 14:43:11, Wladimir Palant wrote: > On 2015/02/26 12:52:38, Sebastian Noack wrote: > > Do we really need to make sure that the item is removed from the backend > > storage, if the pref gets reverted to an empty object? > > The point of this test isn't really ensuring that the preference is removed, > rather making sure that it is created even if it didn't exist before. As such, > it is a prerequisite for the following checks. So I suppose removing this line (and the same below) is fine? http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... File chrome/content/tests/prefs.js (right): http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:18: if ("chrome" in window) On 2015/02/26 14:43:11, Wladimir Palant wrote: > Looks like this test doesn't consider Safari. Is there already an issue to get > the Safari implementation tested as well? Tests are currently and ever since pretty much broken on Safari. The largest issue there is that we can't access the background page there, at least if want to get rid of the messaging proxy. Also some extension APIs that can be accessed from extension pages on Chrome, can only be accessed from the background page on Safari. So eventually, if we want to run tests on Safari, we have to run the tests in the background page, showing the results somehow on a page. I have the feeling that this is getting difficult to impossible with QUnit. http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:21: let key = "pref:" + name; On 2015/02/26 14:43:11, Wladimir Palant wrote: > Nit: no smart tabs please. Just regular tabs. I'm still not a fan of spaces. So I manually switch from tabs to spaces when working on our files, and apparently forgot once. ;) http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:22: chrome.storage.local.get([key], function(items) On 2015/02/26 14:43:11, Wladimir Palant wrote: > According to documentation, passing in key without wrapping it in an array > should work as well. Yes, it does, but I find it weird. Since you always get an object of key-value-pairs in the callback, it's kinda inconsistent when passing an integral. But I don't mind, changed it. http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:39: let key = "pref:" + name; On 2015/02/26 14:43:11, Wladimir Palant wrote: > Nit: no smart tabs please. Done, here as well. http://codereview.adblockplus.org/5203041586249728/diff/5629499534213120/chro... chrome/content/tests/prefs.js:40: chrome.storage.local.get([key], function(items) On 2015/02/26 14:43:11, Wladimir Palant wrote: > As above, passing in key without wrapping it in an array should work as well. Done, here as wel.
LGTM 
|
