Issue 4574 - Adds Tests to createNightlies platform specific revisions

sitescripts/extensions/test/test_createNightlies.py

Index: sitescripts/extensions/test/test_createNightlies.py
===================================================================
--- a/sitescripts/extensions/test/test_createNightlies.py
+++ b/sitescripts/extensions/test/test_createNightlies.py
@@ -9,30 +9,56 @@
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 """Tests for create nightlies script."""
+import subprocess
 import pytest
+import os

[Sebastian Noack, 2016/11/07 16:24:41]

As per PEP-8, corelib imports are supposed to go above third-party modules (i.e.
"pytest" here) and should be separated by a blank line:

  import subprocess
  import os

  import pytest

  from sitescripts.extensions.bin import createNightlies
  from sitescripts.utils import get_config

[Jon Sonesen, 2016/11/08 17:29:45]

Acknowledged.

 
 from sitescripts.extensions.bin import createNightlies
 from sitescripts.utils import get_config
 
 
-@pytest.fixture()
+@pytest.fixture(scope='session')
 def config(hg_dir):
+    """Set and return config obj for NightlyBuild"""
     config = get_config()
     config.type = 'safari'
-    config.repository = str(hg_dir.join('adblockplusnightly'))
     config.revision = 'safari'
+    config.repositoryName = 'adblockplusnightly'
+    config.repository = hg_dir.join('adblockplusnightly').strpath

[Jon Sonesen, 2016/11/04 15:11:55]

I prefer to use strpath rather than the explicit cast. I have been trying to
research to see if there is any benefit to this (initially I was thinking the
strpath is already in memory with the object thus saving resources on the
casting of the object but alas, not able to confirm) But it is easier to type
and also seems more readable to me. If it's better to cast I will happily change
this.

[Vasily Kuznetsov, 2016/11/07 14:53:21]

I'm not sure which way is preferable to be honest. `strpath` is only mentioned
in the section about svn path in the docs
(https://py.readthedocs.io/en/latest/path.html) even though it's supported on
local paths too so I wasn't quite sure how stable this API is. But it's probably
as stable as `py.path` itself :)

[Jon Sonesen, 2016/11/08 17:29:45]

Yeah I spent some time trying to research it but did not find much. Perhaps we
just use the str() cast since that seems to be the standard in the existing code
base and I couldn't find any reason why it is more preferable.

     return config
 
 
-@pytest.fixture()
+@pytest.fixture(scope='session')
 def nightlybuild(config):
     return createNightlies.NightlyBuild(config)
 
 
+@pytest.fixture(scope='session')
+def nightlydir(hg_dir):
+    return hg_dir.join('adblockplusnightly')
+
+
+@pytest.fixture(scope='session')
+def current_revision(nightlydir):
+    command = [
+        'hg', 'id', '-R', nightlydir.strpath, '-i', '-r', 'ancestors(safari)']

[Sebastian Noack, 2016/11/07 16:24:41]

The brackets look somewhat misplaced. I'm frankly surprised that flake8 doesn't
complain. Personally, I'd rather go for hanging indention:

  command = ['hg', 'id', '-R', nightlydir.strpath,
             '-i', '-r', 'ancestors(safari)']

Or if your prefer, that would be OK too:

  command = [
      'hg', 'id', '-R', nightlydir.strpath, '-i', '-r', 'ancestors(safari)'
  ]

[Vasily Kuznetsov, 2016/11/07 17:11:00]

PEP-8 actually allows this style (that's why flake8 doesn't complain I suppose)
but we've agreed that we'll use one of the two options in Sebastian's example.
We haven't written this into the style guide -- perhaps we should (we can follow
up in #python on IRC).

[Jon Sonesen, 2016/11/08 17:29:45]

Acknowledged.

Also I can open a ticket to add this bit to the style guide....we could also
probably do an update to the flake-8 extension.

+
+    return subprocess.check_output(command).strip()

[Jon Sonesen, 2016/11/04 15:11:55]

Personally, I don't like this approach. It seems to me that a better way to
implement this would be to add an exported diff to the repo, and have the temp
repo import that. This way it would be statically coded and I'm not duplicating
the code that I am meant to be testing. Thoughts?

[Vasily Kuznetsov, 2016/11/07 14:53:21]

Sounds good, that would be an improvement.

[Jon Sonesen, 2016/11/08 17:29:45]

Acknowledged.

+
+
 def test_nightly_object_bookmark(nightlybuild):
     assert nightlybuild.config.revision == 'safari'
+
+
+def test_current_revision(current_revision, nightlybuild):
+    assert nightlybuild.revision == current_revision
+
+
+def test_copy_repository(nightlybuild, nightlydir):
+    nightlybuild.copyRepository()
+    assert os.listdir(nightlybuild.tempdir) == os.listdir(nightlydir.strpath)

[Jon Sonesen, 2016/11/04 15:11:55]

an alternative approach...or maybe even just another assertion that could be
made would be to clone the 'master' bookmark into another tempdir and then make
sure the the expected differences are there (namely the addition of the foo
file). This would basically look like 

assert os.listdir(nightlybuild.tempdir) != os.listdir(masterdir.strpath)

[Vasily Kuznetsov, 2016/11/07 14:53:21]

So which behavior would we be testing with this?

[Jon Sonesen, 2016/11/08 17:29:45]

copyRepository copies the specific bookmark to build the nightly from so this
was ensure that proper set of code get's copied.