Issue 29402576: Issue 5052 - Adjust filter storage unit tests for API changes (Closed)
|
Created:
April 4, 2017, 12:06 p.m. by Wladimir Palant Modified:
April 5, 2017, 3:48 p.m. Reviewers:
kzar Base URL:
https://hg.adblockplus.org/adblockpluscore Visibility:
Public. |
DescriptionIssue 5052 - Adjust filter storage unit tests for API changes
Patch Set 1 #
Total comments: 11
Patch Set 2 : Removed unused variable #MessagesTotal messages: 5
https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... File test/filterStorage_readwrite.js (left): https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:76: let tempFile2 = IO.resolveFilePath("temp_patterns2.ini"); Working with temporary files is no longer necessary, this test is not accessing any physical files like it used to do. https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... File test/filterStorage_readwrite.js (right): https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:46: FilterStorage.addFilter(Filter.fromText("foobar")); Calling FilterStorage.addFilter() has essentially the same effect as FilterStorage.addSubscription() - it adds a subscription implicitly so the storage isn't completely empty. The additional benefit is that this subscription also has a filter so FilterStorage.importData() won't remove it. https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:77: function canonize(data) This function was merely moved so that it can be used by two tests. https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:311: backupFile.leafName = backupFile.leafName.replace(/\.ini$/, "-backup1.ini"); This variable is unused - I already removed it locally but won't update the review yet in order to avoid unnecessary noise.
https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... File test/filterStorage_readwrite.js (left): https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:76: let tempFile2 = IO.resolveFilePath("temp_patterns2.ini"); On 2017/04/04 12:11:16, Wladimir Palant wrote: > Working with temporary files is no longer necessary, this test is not accessing > any physical files like it used to do. Acknowledged. https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... File test/filterStorage_readwrite.js (right): https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:44: dataFile = IO.resolveFilePath(Prefs.patternsfile); Shouldn't dataFile be assigned inside the setUp function so that it's state is fresh for each test? https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:46: FilterStorage.addFilter(Filter.fromText("foobar")); On 2017/04/04 12:11:16, Wladimir Palant wrote: > Calling FilterStorage.addFilter() has essentially the same effect as > FilterStorage.addSubscription() - it adds a subscription implicitly so the > storage isn't completely empty. The additional benefit is that this subscription > also has a filter so FilterStorage.importData() won't remove it. Acknowledged. https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:77: function canonize(data) On 2017/04/04 12:11:16, Wladimir Palant wrote: > This function was merely moved so that it can be used by two tests. Acknowledged. https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:311: backupFile.leafName = backupFile.leafName.replace(/\.ini$/, "-backup1.ini"); On 2017/04/04 12:11:16, Wladimir Palant wrote: > This variable is unused - I already removed it locally but won't update the > review yet in order to avoid unnecessary noise. If the variable's unused I don't mind it if you delete it here too. Up to you anyway.
https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... File test/filterStorage_readwrite.js (right): https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:44: dataFile = IO.resolveFilePath(Prefs.patternsfile); On 2017/04/05 04:00:15, kzar wrote: > Shouldn't dataFile be assigned inside the setUp function so that it's state is > fresh for each test? Isn't it? We are still inside setUp.
LGTM https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... File test/filterStorage_readwrite.js (right): https://codereview.adblockplus.org/29402576/diff/29402577/test/filterStorage_... test/filterStorage_readwrite.js:44: dataFile = IO.resolveFilePath(Prefs.patternsfile); On 2017/04/05 07:14:24, Wladimir Palant wrote: > On 2017/04/05 04:00:15, kzar wrote: > > Shouldn't dataFile be assigned inside the setUp function so that it's state is > > fresh for each test? > > Isn't it? We are still inside setUp. Argh sorry, so it is. 
|
