Noissue - Refactor translation tests to validate sync

tests/test_translations.py

Index: tests/test_translations.py
===================================================================
--- a/tests/test_translations.py
+++ b/tests/test_translations.py
@@ -1,52 +1,38 @@
 import os
+import sys
+import runpy
 import shutil
 import pytest
+import zipfile
 from wsgi_intercept import urllib3_intercept
 from wsgi_intercept import add_wsgi_intercept
 from wsgi_intercept import remove_wsgi_intercept
 
 from crowdin_mock_api import app
-from cms.bin import translate
 
 
 @pytest.fixture(scope='module')
-def expect_requests():
-    return [
-        ('info?key=test_key&json=1', ''),
-        ('supported-languages?key=test_key&json=1', ''),
-        ('add-file?key=test_key&json=1', 'translate.json'),
-        ('upload-translation?key=test_key&json=1', 'test_sample'),
-        ('delete-file?key=test_key&json=1', 'translate.json'),
-        ('delete-file?key=test_key&json=1', 'translate.json'),
-        ('delete-directory?key=test_key&json=1', ''),
-        ('delete-directory?key=test_key&json=1', ''),
-        ('export?key=test_key&json=1', ''),
-        ('download/all.zip?key=test_key', ''),
-    ]
-
-
-@pytest.fixture(scope='module')
-def make_crowdin_zip(temp_site):
+def api_zip(temp_site, request):

[Vasily Kuznetsov, 2018/01/02 15:10:04]

Am I right that the purpose of this fixture is to examine the zip-file that is
produced by running the translation script in line 36?

If this is indeed the case, I think the fixture is not the right approach.
Fixture is something we prepare before the test for the tested code (and
sometimes assertions) to use. However, here we want to examine the output of the
tested code (after it was run), so we should probably just put this code into
the test or make it a function.

[Jon Sonesen, 2018/01/05 10:27:07]

No, this generated the zips that the test validates

[Vasily Kuznetsov, 2018/01/05 14:30:16]

Ok, I see. But then I'm not sure what this test is testing. It seems like it
does the following:
1. Generates a zip file from the `locales` folder of the test website (that's
`api_zip` fixture).
2. Runs `cms.bin.translate` (that gets the generated `zip` file from the mock
API -- this hidden dependency is not great, but it existed before).
3. Checks that the `locales` folder of the test website has all the directories
from the zip file.

If step 2 doesn't do anything at all, the test would still pass, because the zip
file was generated from the folder with which we compare it. Seems like we
haven't really tested much.

So I think to make this meaningful, we need to introduce some changes to the zip
file and make the mock API return other changes that would have some effects
that we can then test.

Or we could just keep it simple and only check that nothing crashes during
Crowdin sync. But then the previous logging setup that was checking that the
correct HTTP requests were made was better than nothing, so we should probably
keep it.

     zip_name = os.path.join('tests', 'all')
     input_dir = os.path.join(temp_site, 'locales')
     shutil.make_archive(zip_name, 'zip', input_dir)
-    yield None
+    yield zipfile.ZipFile(zip_name + '.zip')
     os.remove(zip_name + '.zip')
+    return
 
 
 @pytest.fixture()
 def make_intercept(scope='module'):
     host = 'api.crowdin.com'
     port = 443
     urllib3_intercept.install()
     add_wsgi_intercept(host, port, lambda: app.wsgi_app)
     yield None
     remove_wsgi_intercept()
 
 
-def test_sync(temp_site, make_intercept, make_crowdin_zip, expect_requests):
-    translate.crowdin_sync(temp_site, 'test_key')
-    for (url, data), (expect_url, expect_data) in zip(app.request_log,
-                                                      expect_requests):
-        assert expect_url in url
-        assert expect_data in data
+def test_sync(temp_site, make_intercept, api_zip):
+    sys.argv = [None, temp_site, 'test_key']
+    runpy.run_module('cms.bin.translate', run_name='__main__')
+    for locale in [os.path.dirname(x) for x in api_zip.namelist()]:
+        assert locale in os.listdir(os.path.join(temp_site, 'locales'))

[Vasily Kuznetsov, 2018/01/02 15:10:04]

I get an error here:

    def test_sync(temp_site, make_intercept, api_zip):
        sys.argv = [None, temp_site, 'test_key']
        runpy.run_module('cms.bin.translate', run_name='__main__')
        for locale in [os.path.dirname(x) for x in api_zip.namelist()]:
->          assert locale in os.listdir(os.path.join(temp_site, 'locales'))
E           AssertionError: assert '.' in ['de', 'en']

[Jon Sonesen, 2018/01/05 10:27:07]

odd, i dont, but will look into it

[Vasily Kuznetsov, 2018/01/05 14:30:16]

I printed `api_zip.namelist()` in this test and it contains the current
directory for some reason:

['./', 'de/', 'en/', 'de/translate.json', 'en/global.json']

It's kind of dumb that `ZipFile` does this, but I guess we'd need to live with
it somehow to make the test work reliably.