Issue 4223 - Adapt filter storage read/write tests to work in adblockpluscore repository

Issue 4223 - Adapt filter storage read/write tests to work in adblockpluscore repository

[Wladimir Palant, 2016-10-05 20:10:23]

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
File test/filterStorage_readwrite.js (right):

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:149:
FilterStorage.addSubscription(subscription);
This was calling our public API in the original test, hence testing below that
the subscription was definitely added correctly. Now the subscription is being
added explicitly (public API isn't part of adblockpluscore) but I thought that
leaving the checks won't hurt - the point of this code is making sure that
external subscriptions aren't being saved, so we better be sure that we have an
external subscription in the data.

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:205: test.deepEqual(subscription.defaults, null,
"Default types");
I had to modify this, the fallback for legacy groups is gone with
https://issues.adblockplus.org/ticket/3660 but this test wasn't adapted for some
reason. Default title is no more, and there are no implied defaults either.

[kzar, 2016-10-06 05:13:38]

https://codereview.adblockplus.org/29356024/diff/29356025/README.md
File README.md (right):

https://codereview.adblockplus.org/29356024/diff/29356025/README.md#newcode16
README.md:16: in the `test` directory of the repository.
Mind mentioning that you can also give `npm test` a list of paths should you
want to run a subset of the tests?

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
File test/filterStorage_readwrite.js (right):

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:50: let datapath = path.resolve(__dirname,
"data", "patterns.ini");
Did you forget to add the data/patterns.ini file?

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:65: let listener = function(action)
Seems like this logic is nearly identical in saveFilters, perhaps reuse it?

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:149:
FilterStorage.addSubscription(subscription);
On 2016/10/05 20:10:22, Wladimir Palant wrote:
> This was calling our public API in the original test, hence testing below that
> the subscription was definitely added correctly. Now the subscription is being
> added explicitly (public API isn't part of adblockpluscore) but I thought that
> leaving the checks won't hurt - the point of this code is making sure that
> external subscriptions aren't being saved, so we better be sure that we have
an
> external subscription in the data.

Acknowledged.

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:203: test.equal(subscription.url, url,
"Subscription ID");
Won't `url` always be "~eh~" by the time the test is run?

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:205: test.deepEqual(subscription.defaults, null,
"Default types");
On 2016/10/05 20:10:22, Wladimir Palant wrote:
> I had to modify this, the fallback for legacy groups is gone with
> https://issues.adblockplus.org/ticket/3660 but this test wasn't adapted for
some
> reason. Default title is no more, and there are no implied defaults either.

Acknowledged.

[Wladimir Palant, 2016-10-06 07:13:59]

https://codereview.adblockplus.org/29356024/diff/29356025/README.md
File README.md (right):

https://codereview.adblockplus.org/29356024/diff/29356025/README.md#newcode16
README.md:16: in the `test` directory of the repository.
On 2016/10/06 05:13:37, kzar wrote:
> Mind mentioning that you can also give `npm test` a list of paths should you
> want to run a subset of the tests?

Done.

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
File test/filterStorage_readwrite.js (right):

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:50: let datapath = path.resolve(__dirname,
"data", "patterns.ini");
On 2016/10/06 05:13:38, kzar wrote:
> Did you forget to add the data/patterns.ini file?

No, I copied the files in a separate commit - what you have here is merely a
diff from the original files. The patterns.ini we use for tests wasn't modified.

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:65: let listener = function(action)
On 2016/10/06 05:13:38, kzar wrote:
> Seems like this logic is nearly identical in saveFilters, perhaps reuse it?

Reusing isn't quite trivial but with the new FilterNotifier API this is no
longer an issue.

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:203: test.equal(subscription.url, url,
"Subscription ID");
On 2016/10/06 05:13:37, kzar wrote:
> Won't `url` always be "~eh~" by the time the test is run?

Good question. I checked and this isn't the case - we actually run this test
with all three variants for `url`. However, changing `let url` into `var url`
will produce the behavior you expected.

[kzar, 2016-10-06 08:06:47]

LGTM

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
File test/filterStorage_readwrite.js (right):

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:50: let datapath = path.resolve(__dirname,
"data", "patterns.ini");
On 2016/10/06 07:13:59, Wladimir Palant wrote:
> On 2016/10/06 05:13:38, kzar wrote:
> > Did you forget to add the data/patterns.ini file?
> 
> No, I copied the files in a separate commit - what you have here is merely a
> diff from the original files. The patterns.ini we use for tests wasn't
modified.

Acknowledged.

https://codereview.adblockplus.org/29356024/diff/29356025/test/filterStorage_...
test/filterStorage_readwrite.js:203: test.equal(subscription.url, url,
"Subscription ID");
On 2016/10/06 07:13:59, Wladimir Palant wrote:
> On 2016/10/06 05:13:37, kzar wrote:
> > Won't `url` always be "~eh~" by the time the test is run?
> 
> Good question. I checked and this isn't the case - we actually run this test
> with all three variants for `url`. However, changing `let url` into `var url`
> will produce the behavior you expected.

Ah of course.