Issue 4223 - Adapt synchronizer tests to work in adblockpluscore repository

Issue 4223 - Adapt synchronizer tests to work in adblockpluscore repository

[Wladimir Palant, 2016-10-05 08:12:01]

The diff is moderately useful unfortunately, too much had to be changed. The
original test was running in pseudo-synchronous fashion, this had to be changed
to using promises. As a consequence, loops has to be converted into runTest
functions calling themselves recursively - this is ugly, I want to make each
former loop iteration a separate test. The "virtual time" concept could be
simplified considerably compared to what we have in adblockplustests - we know
that there is only one timer, so we can wait for the outstanding requests to be
done and advance the "time" to trigger that timer. Note: I didn't touch the test
logic at all here, only changed the way it is expressed.

[Wladimir Palant, 2016-10-05 09:26:06]

Done, no more looping within tests - the former loops are test groups now.

[kzar, 2016-10-05 11:36:51]

Found this a little hard to review, but had a go.

https://codereview.adblockplus.org/29355872/diff/29355873/test/_common.js
File test/_common.js (right):

https://codereview.adblockplus.org/29355872/diff/29355873/test/_common.js#new...
test/_common.js:34: log: () => undefined,
I think suppressing logging for all tests might be a bad idea. Couldn't we miss
a problem that way? (Also I find it kind of useful to log stuff to figure out
why my tests aren't passing :p.) Perhaps we should only do this for the tests
that need it?

https://codereview.adblockplus.org/29355872/diff/29355873/test/_common.js#new...
test/_common.js:105: globals: Object.assign({}, globals, options.globals),
(Cool, never heard of Object.assign() before.)

https://codereview.adblockplus.org/29355872/diff/29355950/test/synchronizer.js
File test/synchronizer.js (right):

https://codereview.adblockplus.org/29355872/diff/29355950/test/synchronizer.j...
test/synchronizer.js:82: function XMLHttpRequest()
Seems kind of wasteful to define XMLHttpRequest and some of this other stuff
inside setUp for every test?

https://codereview.adblockplus.org/29355872/diff/29355950/test/synchronizer.j...
test/synchronizer.js:353: this.registerHandler("/subscription", metadata =>
I guess we could avoid declaring all the module variables at the top like `let
Filter = null;` too if we assigned them to the `this` Object from the setUp
functions. I didn't realise that was an option before, not sure if it's worth
changing that everywhere now however.

[Wladimir Palant, 2016-10-05 12:12:50]

> Found this a little hard to review,

Not exactly unexpected, this is a complex test - which is why I am definitely
not changing the logic right now.

https://codereview.adblockplus.org/29355872/diff/29355873/test/_common.js
File test/_common.js (right):

https://codereview.adblockplus.org/29355872/diff/29355873/test/_common.js#new...
test/_common.js:34: log: () => undefined,
On 2016/10/05 11:36:51, kzar wrote:
> I think suppressing logging for all tests might be a bad idea. Couldn't we
miss
> a problem that way?

Not really. An exception in a test should still be displayed, and other than
that - we are intentionally causing error conditions in tests, so our code
logging that error is expected and not useful at all.

> Also I find it kind of useful to log stuff to figure out
> why my tests aren't passing :p.

Yes, if you mess with code inside the sandbox you might want to activate logging
again - I do the same thing right now but that's a temporary measure. Maybe
something to have as a command line switch.

https://codereview.adblockplus.org/29355872/diff/29355950/test/synchronizer.js
File test/synchronizer.js (right):

https://codereview.adblockplus.org/29355872/diff/29355950/test/synchronizer.j...
test/synchronizer.js:82: function XMLHttpRequest()
On 2016/10/05 11:36:51, kzar wrote:
> Seems kind of wasteful to define XMLHttpRequest and some of this other stuff
> inside setUp for every test?

Not really - we want a clean environment for every test. But the code is quite
massive and unstructured indeed, the next change is going to move it into
_common.js.

https://codereview.adblockplus.org/29355872/diff/29355950/test/synchronizer.j...
test/synchronizer.js:353: this.registerHandler("/subscription", metadata =>
On 2016/10/05 11:36:51, kzar wrote:
> I guess we could avoid declaring all the module variables at the top like `let
> Filter = null;` too if we assigned them to the `this` Object from the setUp
> functions. I didn't realise that was an option before, not sure if it's worth
> changing that everywhere now however.

Not during the initial migration - the fewer changes the better. But we should
definitely consider better options that avoid having global variables.

[kzar, 2016-10-05 12:18:16]

LGTM since as you say we don't want to mess with this file too much before it's
migrated.

https://codereview.adblockplus.org/29355872/diff/29355873/test/_common.js
File test/_common.js (right):

https://codereview.adblockplus.org/29355872/diff/29355873/test/_common.js#new...
test/_common.js:34: log: () => undefined,
On 2016/10/05 12:12:51, Wladimir Palant wrote:
> Maybe something to have as a command line switch.

Yea, a switch to enable logging would be pretty useful.

https://codereview.adblockplus.org/29355872/diff/29355950/test/synchronizer.js
File test/synchronizer.js (right):

https://codereview.adblockplus.org/29355872/diff/29355950/test/synchronizer.j...
test/synchronizer.js:82: function XMLHttpRequest()
On 2016/10/05 12:12:51, Wladimir Palant wrote:
> On 2016/10/05 11:36:51, kzar wrote:
> > Seems kind of wasteful to define XMLHttpRequest and some of this other stuff
> > inside setUp for every test?
> 
> Not really - we want a clean environment for every test. But the code is quite
> massive and unstructured indeed, the next change is going to move it into
> _common.js.

Fair enough.