Issue 6728 - Remove Mercurial dependency

Issue 6728 - Removes Mercurial dependency

[mathias, 2018-06-13 06:04:50]

https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py#newc...
cms/sources.py:238: return randint(1, 10)
I think this should be a way higher range. The likeliness of hitting the same
number in two consecutive builds is probably too high otherwise.

[Vasily Kuznetsov, 2018-06-13 13:35:24]

Hi Jon,

Looks pretty good, love the deleted code :D

I'm thinking that perhaps we need to add a test for using the cache directory
(since we're messing with it, so it would be good to check that it still works).
Also there are a couple of inline comments (see below).

Cheers,
Vasily

https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py#newc...
cms/sources.py:238: return randint(1, 10)
On 2018/06/13 06:04:50, mathias wrote:
> I think this should be a way higher range. The likeliness of hitting the same
> number in two consecutive builds is probably too high otherwise.

Yeah, a range of 10 is too small. This would give us 10% chance of an accidental
collision. Something like a longint range (2^32) would be better.

https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py
File tests/conftest.py (left):

https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py#o...
tests/conftest.py:16: subprocess.check_call(['hg', 'init', site_dir])
Woohoo! All of this stuff GONE. Could even delete the copying if we could
arrange for the cache to be stored in /tmp or something like this (just an idea,
doesn't have to be in this review).

https://codereview.adblockplus.org/29805580/diff/29805586/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29805580/diff/29805586/tests/test_page_out...
tests/test_page_outputs.py:58: return
Maybe this could just be replaced with `else:`.

[Jon Sonesen, 2018-06-14 19:24:07]

On 2018/06/13 13:35:24, Vasily Kuznetsov wrote:
> Hi Jon,
> 
> Looks pretty good, love the deleted code :D
> 
> I'm thinking that perhaps we need to add a test for using the cache directory
> (since we're messing with it, so it would be good to check that it still
works).
> Also there are a couple of inline comments (see below).
> 
> Cheers,
> Vasily
> 
> https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py
> File cms/sources.py (right):
> 
>
https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py#newc...
> cms/sources.py:238: return randint(1, 10)
> On 2018/06/13 06:04:50, mathias wrote:
> > I think this should be a way higher range. The likeliness of hitting the
same
> > number in two consecutive builds is probably too high otherwise.
> 
> Yeah, a range of 10 is too small. This would give us 10% chance of an
accidental
> collision. Something like a longint range (2^32) would be better.
> 
> https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py
> File tests/conftest.py (left):
> 
>
https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py#o...
> tests/conftest.py:16: subprocess.check_call(['hg', 'init', site_dir])
> Woohoo! All of this stuff GONE. Could even delete the copying if we could
> arrange for the cache to be stored in /tmp or something like this (just an
idea,
> doesn't have to be in this review).
> 
>
https://codereview.adblockplus.org/29805580/diff/29805586/tests/test_page_out...
> File tests/test_page_outputs.py (right):
> 
>
https://codereview.adblockplus.org/29805580/diff/29805586/tests/test_page_out...
> tests/test_page_outputs.py:58: return
> Maybe this could just be replaced with `else:`.

Cool, I will add a test for this. Thanks for the feedback

[Jon Sonesen, 2018-06-14 19:24:15]

https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29805580/diff/29805586/cms/sources.py#newc...
cms/sources.py:238: return randint(1, 10)
On 2018/06/13 13:35:23, Vasily Kuznetsov wrote:
> On 2018/06/13 06:04:50, mathias wrote:
> > I think this should be a way higher range. The likeliness of hitting the
same
> > number in two consecutive builds is probably too high otherwise.
> 
> Yeah, a range of 10 is too small. This would give us 10% chance of an
accidental
> collision. Something like a longint range (2^32) would be better.

My bad, meant to increase the size here, it was just a quick placeholder for
testing

https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py
File tests/conftest.py (left):

https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py#o...
tests/conftest.py:16: subprocess.check_call(['hg', 'init', site_dir])
On 2018/06/13 13:35:23, Vasily Kuznetsov wrote:
> Woohoo! All of this stuff GONE. Could even delete the copying if we could
> arrange for the cache to be stored in /tmp or something like this (just an
idea,
> doesn't have to be in this review).

Yeah that's a good idea, I reckon we should do this in a different commit/issue

https://codereview.adblockplus.org/29805580/diff/29805586/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29805580/diff/29805586/tests/test_page_out...
tests/test_page_outputs.py:58: return
On 2018/06/13 13:35:23, Vasily Kuznetsov wrote:
> Maybe this could just be replaced with `else:`.

Heh, yeah for sure whoops

[Vasily Kuznetsov, 2018-07-03 08:50:24]

Hi Jon,

Looks good, just some small adjustments.

Cheers,
Vasily

https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py
File tests/conftest.py (left):

https://codereview.adblockplus.org/29805580/diff/29805586/tests/conftest.py#o...
tests/conftest.py:16: subprocess.check_call(['hg', 'init', site_dir])
On 2018/06/14 19:24:14, Jon Sonesen wrote:
> On 2018/06/13 13:35:23, Vasily Kuznetsov wrote:
> > Woohoo! All of this stuff GONE. Could even delete the copying if we could
> > arrange for the cache to be stored in /tmp or something like this (just an
> idea,
> > doesn't have to be in this review).
> 
> Yeah that's a good idea, I reckon we should do this in a different
commit/issue

I agree. Another commit would be better.

https://codereview.adblockplus.org/29805580/diff/29805586/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29805580/diff/29805586/tests/test_page_out...
tests/test_page_outputs.py:58: return
On 2018/06/14 19:24:15, Jon Sonesen wrote:
> On 2018/06/13 13:35:23, Vasily Kuznetsov wrote:
> > Maybe this could just be replaced with `else:`.
> 
> Heh, yeah for sure whoops

This one seems to still be there in PS3

https://codereview.adblockplus.org/29805580/diff/29808569/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29805580/diff/29808569/tests/test_page_out...
tests/test_page_outputs.py:70: source = FileSource(os.path.join('tests',
'test_site'))
Nit: It seems like having "tests/test_site" as base directory doesn't really add
value compared for example to "test_site" and the latter would be a bit simpler
and shorter.

https://codereview.adblockplus.org/29805580/diff/29808569/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29805580/diff/29808569/tox.ini#newcode38
tox.ini:38: py.test --cov-report term --cov-report html --cov=cms tests
--basetemp=tmp
The --basetemp=tmp creates ./tmp directory, why do we need this? Also, if we do
want leave this in, we should .hgignore and .gitignore it.

[jsonesen, 2018-07-05 18:58:32]

https://codereview.adblockplus.org/29805580/diff/29808569/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29805580/diff/29808569/tests/test_page_out...
tests/test_page_outputs.py:70: source = FileSource(os.path.join('tests',
'test_site'))
On 2018/07/03 08:50:24, Vasily Kuznetsov wrote:
> Nit: It seems like having "tests/test_site" as base directory doesn't really
add
> value compared for example to "test_site" and the latter would be a bit
simpler
> and shorter.

Acknowledged.

https://codereview.adblockplus.org/29805580/diff/29808569/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29805580/diff/29808569/tox.ini#newcode38
tox.ini:38: py.test --cov-report term --cov-report html --cov=cms tests
--basetemp=tmp
On 2018/07/03 08:50:24, Vasily Kuznetsov wrote:
> The --basetemp=tmp creates ./tmp directory, why do we need this? Also, if we
do
> want leave this in, we should .hgignore and .gitignore it.

This makes debugging the test suite easier by placing the temporary sites made
by it within the project directory rather than in the default tempfile location.
At least for me I have found it makes the paths which are printed more concise.
Will add to the ignore files too!

[Vasily Kuznetsov, 2018-07-05 21:48:20]

Hey Jon,

All good, but how about we remove the ./tmp temporary directory from this review
for now and think about the best way to make debugging the tests easier
separately?

Cheers,
Vasily

https://codereview.adblockplus.org/29805580/diff/29808569/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29805580/diff/29808569/tox.ini#newcode38
tox.ini:38: py.test --cov-report term --cov-report html --cov=cms tests
--basetemp=tmp
On 2018/07/05 18:58:31, jsonesen wrote:
> On 2018/07/03 08:50:24, Vasily Kuznetsov wrote:
> > The --basetemp=tmp creates ./tmp directory, why do we need this? Also, if we
> do
> > want leave this in, we should .hgignore and .gitignore it.
> 
> This makes debugging the test suite easier by placing the temporary sites made
> by it within the project directory rather than in the default tempfile
location.
> At least for me I have found it makes the paths which are printed more
concise.
> Will add to the ignore files too!

Hm, changing the temporary folder in tox.ini to make debugging easier is a bit
questionable. Tox is more of a CI tool whereas it would be better to just
manually invoke pytest for debugging (with the appropriate options). Also this
change is unrelated in this review. Maybe we remove it here for now and think
about it a bit?

[Vasily Kuznetsov, 2018-07-09 15:05:32]

Hi Jon,

Two more comments before this can be pushed:

- I found another place in the tests that needs to be changed for this patch:
test_additional_paths.py also calls hg to set up a test site and this is no
longer needed:
https://hg.adblockplus.org/cms/file/tip/tests/test_additional_paths.py#l65
- The commit message should read "Remove Mercurial dependency" instead of
"Removes". I saw that you used 3-rd person of verbs before instead of imperative
(and I missed that), but it would be better to be consistent and I think using
imperative is the best approach.

Cheers,
Vasily

[Jon Sonesen, 2018-07-09 18:06:35]

On 2018/07/09 15:05:32, Vasily Kuznetsov wrote:
> Hi Jon,
> 
> Two more comments before this can be pushed:
> 
> - I found another place in the tests that needs to be changed for this patch:
> test_additional_paths.py also calls hg to set up a test site and this is no
> longer needed:
> https://hg.adblockplus.org/cms/file/tip/tests/test_additional_paths.py#l65
Oh dang! my bad, good catch :)

> - The commit message should read "Remove Mercurial dependency" instead of
> "Removes". I saw that you used 3-rd person of verbs before instead of
imperative
> (and I missed that), but it would be better to be consistent and I think using
> imperative is the best approach.

Agreed, thanks!

[Vasily Kuznetsov, 2018-07-09 19:03:48]

LGTM. Although let's push it tomorrow carefully :D

[Jon Sonesen, 2018-07-09 19:24:37]

On 2018/07/09 19:03:48, Vasily Kuznetsov wrote:
> LGTM. Although let's push it tomorrow carefully :D

SGTM