Fixes 4464 - Restore get_min_sdk_version() function from sitescripts.extensions.android module that…

Fixes 4464 - Restore get_min_sdk_version() function from sitescripts.extensions.android module that was deleted by #3759

[Vasily Kuznetsov, 2016-09-22 16:06:10]

For now I haven't tested this for real, but the change is quite minimal so maybe
it's ok. Perhaps we should add an automatic test for this script. What would be
the minimal representative configuration for that?

[Sebastian Noack, 2016-09-22 16:14:52]

On 2016/09/22 16:06:10, Vasily Kuznetsov wrote:
> For now I haven't tested this for real, but the change is quite minimal so
maybe
> it's ok. Perhaps we should add an automatic test for this script. What would
be
> the minimal representative configuration for that?

Yes, if we could add tests for this script, while on it, that would be awesome.
For reference, here is my sitescripts.ini that I used for manual testing in the
past, it might be outdated/incomplete though:
https://pastebin.mozilla.org/8912209

https://codereview.adblockplus.org/29354748/diff/29354749/sitescripts/extensi...
File sitescripts/extensions/bin/updateUpdateManifests.py (right):

https://codereview.adblockplus.org/29354748/diff/29354749/sitescripts/extensi...
sitescripts/extensions/bin/updateUpdateManifests.py:40: ANDROID_VERSIONS =
['1.0', '1.1', '1.5', '1.6', '2.0', '2.0.1', '2.1',
Is this variable even used?

[Wladimir Palant, 2016-09-23 09:21:15]

On 2016/09/22 16:06:10, Vasily Kuznetsov wrote:
> What would be the minimal representative configuration for that?

Something like this should do:

[extensions]
adblockplusfirefox_repository=%(hgroot)s/adblockplus
adblockplusfirefox_type=gecko

adblockplusie_repository=%(hgroot)s/adblockplusie
adblockplusie_type=ie

adblockplussafari_repository=%(hgroot)s/adblockpluschrome/
adblockplussafari_type=safari
adblockplussafari_key=%(keysdir)s/adblockplussafari.pem

adblockplusandroid_repository=%(hgroot)s/adblockplusandroid
adblockplusandroid_type=android

downloadsRepo=%(hgroot)s/downloads
downloadsURL=https://downloads.adblockplus.org/

geckoUpdateManifestPath=%(wwwdir)s/update.rdf
androidUpdateManifestPath=%(wwwdir)s/androidupdates.xml
androidNewUpdateManifestPath=%(wwwdir)s/androidupdates.json
safariUpdateManifestPath=%(wwwdir)s/updates.plist
ieUpdateManifestPath=%(wwwdir)s/ieupdate.json

geckoUpdateManifest=extensions/template/update.rdf
chromeUpdateManifest=extensions/template/updates.xml
androidUpdateManifest=extensions/template/androidupdates.xml
safariUpdateManifest=extensions/template/updates.plist

This covers every type we support here. One complication is that hgroot has to
be a local path - you need local checkouts of all these repositories. Also,
adblockplussafari.pem needs to contain a private key and a certificate with
realistic info (self-signed will do, validity isn't verified). You need
something like "CN=Safari Developer: (GRYYZR985A) foo@adblockplus.org" in the
certificate for it to be accepted.

[Vasily Kuznetsov, 2016-09-23 09:39:26]

Thanks for the help, guys! Let me try to implement this now.

[Vasily Kuznetsov, 2016-09-23 16:33:35]

I got together some test even though it turned out a bit long. That would still
be ok but it clones repositories from hg.adblockplus.org, which also makes it
very slow. I replaced the downloads repository with a mock and added caching of
the plugin repositories to make the running time more acceptable.

Still, this cloning of repos from the web during a test is not great. Perhaps I
should make some simple mocks for the plugin repos as well. This would make the
test faster and more properly self-contained.

How does that sound?

https://codereview.adblockplus.org/29354748/diff/29354749/sitescripts/extensi...
File sitescripts/extensions/bin/updateUpdateManifests.py (right):

https://codereview.adblockplus.org/29354748/diff/29354749/sitescripts/extensi...
sitescripts/extensions/bin/updateUpdateManifests.py:40: ANDROID_VERSIONS =
['1.0', '1.1', '1.5', '1.6', '2.0', '2.0.1', '2.1',
On 2016/09/22 16:14:52, Sebastian Noack wrote:
> Is this variable even used?

Don't think so. It just seemed important so I copied it too :)

Done.

[Sebastian Noack, 2016-09-23 17:29:26]

On 2016/09/23 16:33:35, Vasily Kuznetsov wrote:
> I got together some test even though it turned out a bit long. That would
still
> be ok but it clones repositories from http://hg.adblockplus.org, which also
makes it
> very slow. I replaced the downloads repository with a mock and added caching
of
> the plugin repositories to make the running time more acceptable.
> 
> Still, this cloning of repos from the web during a test is not great. Perhaps
I
> should make some simple mocks for the plugin repos as well. This would make
the
> test faster and more properly self-contained.

Yeah, tests shouldn't download stuff from the web.

https://codereview.adblockplus.org/29354748/diff/29354749/sitescripts/extensi...
File sitescripts/extensions/bin/updateUpdateManifests.py (right):

https://codereview.adblockplus.org/29354748/diff/29354749/sitescripts/extensi...
sitescripts/extensions/bin/updateUpdateManifests.py:40: ANDROID_VERSIONS =
['1.0', '1.1', '1.5', '1.6', '2.0', '2.0.1', '2.1',
On 2016/09/23 16:33:35, Vasily Kuznetsov wrote:
> On 2016/09/22 16:14:52, Sebastian Noack wrote:
> > Is this variable even used?
> 
> Don't think so. It just seemed important so I copied it too :)
> 
> Done.

It was only used by get_min_android_version(), the function that is gone.

https://codereview.adblockplus.org/29354748/diff/29354834/sitescripts/extensi...
File sitescripts/extensions/test/test_updateManifests.py (right):

https://codereview.adblockplus.org/29354748/diff/29354834/sitescripts/extensi...
sitescripts/extensions/test/test_updateManifests.py:48: def root_dir(tmpdir):
Do we really need a fixture with that one-liner - while speaking of it, this can
be a one liner ;) - or couldn't the test simply use the tmpdir fixture directly?

https://codereview.adblockplus.org/29354748/diff/29354834/sitescripts/extensi...
sitescripts/extensions/test/test_updateManifests.py:70: call_hg(downloads_dir,
'init')
I wonder whether this is worth a helper function. Just convert the path object
to a string above, and inline subprocess.check_call(...) here and below?

[Vasily Kuznetsov, 2016-09-27 09:50:25]

Ok, I changed the approach to mocking all the repositories so the test runs
quicker and doesn't fetch stuff from the internet.

I also figured out why the test was failing under Tox before. It turns out that
the order of items in `update.rdf` is not stable. I wrote a more robust checker
to address this.

https://codereview.adblockplus.org/29354748/diff/29354834/sitescripts/extensi...
File sitescripts/extensions/test/test_updateManifests.py (right):

https://codereview.adblockplus.org/29354748/diff/29354834/sitescripts/extensi...
sitescripts/extensions/test/test_updateManifests.py:48: def root_dir(tmpdir):
On 2016/09/23 17:29:25, Sebastian Noack wrote:
> Do we really need a fixture with that one-liner - while speaking of it, this
can
> be a one liner ;) - or couldn't the test simply use the tmpdir fixture
directly?

Done.

https://codereview.adblockplus.org/29354748/diff/29354834/sitescripts/extensi...
sitescripts/extensions/test/test_updateManifests.py:70: call_hg(downloads_dir,
'init')
On 2016/09/23 17:29:25, Sebastian Noack wrote:
> I wonder whether this is worth a helper function. Just convert the path object
> to a string above, and inline subprocess.check_call(...) here and below?

So I ended up with quite a few calls to `call_hg` and having it saves a lot of
repetition so it seems justified.

[Sebastian Noack, 2016-09-27 14:49:11]

LGTM