Issue 4380 - Add Mock Crowdin API and Sync Test

tests/test_translations.py

 import os
 import shutil
 import pytest
-import mock_api
 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', 'de'),
+        ('delete-directory?key=test_key&json=1', 'en'),
+        ('export?key=test_key&json=1', ''),
+        ('download/all.zip?key=test_key', ''),
+    ]
 
 
 @pytest.fixture(scope='module')
 def make_crowdin_zip(temp_site):
     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
     os.remove(zip_name + '.zip')
 
 
 @pytest.fixture()
 def make_intercept(scope='module'):
     host = 'api.crowdin.com'
     port = 443
     urllib3_intercept.install()

[Sebastian Noack, 2016/10/05 12:06:16]

Wasn't the idea, to not mock the HTTP client library, but to tweak the URL for
the CrowdIn server using a global variable? As discussed back then in the Python
developer meeting, this would be preferable, because that way we test the
whole/original code path. Also that way the tests can be agnostic of the HTTP
client-library used.

[Jon Sonesen, 2016/10/05 14:38:13]

I believe that was the original plan but when implementing it seems simpler to
intercept the request and then have the mock_api handle it accordingly.

[Vasily Kuznetsov, 2016/10/05 14:44:42]

We've decided to switch the approach to wsgi_intercept because it's as good as
the HTTP server approach while being somewhat simpler and less
environment-dependent. The only part that is not covered by it is the urllib3
internals and we don't want to test them anyway. As for being tied to the HTTP
client library, that's true, but if we decide to switch to another one,
modifying the tests will be a matter of changing this one fixture (probably just
one line of it) so it's a negligible amount of effort compared to actually
changing the HTTP library.

[Sebastian Noack, 2016/10/05 16:36:44]

How is this any simpler or even less environment dependent? As per the original
approach, we agreed on, all we would have to do is changing the code in
cms/bin/tranlate.py like that:

  CROWDIN_API_ENDPOINT = 'https://api.crowdin.com/'

  class CrowdinAPI:
      def __init__(self, api_key, project_name):
          self.connection = urllib3.connection_from_url(CROWDIN_API_ENDPOINT)

And then, just set CROWDIN_API_ENDPOINT to 'http://localhost:8000', and run the
server on that port, in the tests. Since you seem to implement the fake CrowdIn
API as WSGI app anyway, turning it into a server is trivial. Well, you probably
have to do so in a thread and then take care to shut it down afterwards. But we
have to do so anyway, when adding tests for the cms development server, later.

However, as discussed before, that way we test the whole code path, and not only
until the point where wsgi_intercept hook into. And that way it's actually
environment independent, as it doesn't matter which HTTP client library is used.
It is not about testing urllib3 itself, but about testing that the way we use
whatever client library works through the entire stack. For example what happens
if the sent data doesn't fit in the underlying buffer, what if unicode instead
str is passed (or vice-versa), what if the headers are malformed, etc. I'm sure
there are some corner cases where wsgi_intercept behaves different from the
client library it hooks into. But to be honest, it is probably not too important
here, as there ultimately are some differences on the server side, which we
completely replace, anyway. Just saying that it is still preferable to have a
test environment that is as close as possible to the production environment.
Even if it just one line, changing the tests, while changing the code that it
tests, is a quite common cause for regressions, in particular if you don't
actually want to change the behavior but replace some underlying modules.

[Vasily Kuznetsov, 2016/10/06 09:38:59]

You make some good points. Indeed it's pretty easy to convert current code to
the HTTP-based approach. Also, while testing integration with urllib3 is not the
point of this test suite, if we get it for free, well, even better. One thing
that I dislike in the HTTP approach is that if port 8000 turns out to be taken
for some reason, our test will fail (that's what I meant by being environment
dependent). We could probably deal with this by picking a port at random and
incrementing it until it works. Then we also need to run additional thread(s)
for HTTP server and kill them when the fixture is no longer needed. So we end up
with more testing code in exchange for testing integration with urllib3 and
easier replacement of HTTP library (I agree that not touching the tests in this
case is better but I'd also point out that we're unlikely to do it). If you feel
that this is worth it, I don't mind.

Jon, do you have any thoughts on this?

[Sebastian Noack, 2016/10/06 10:25:26]

I agree that it's not that great to require a particular TCP port to be free,
and that fallback logic would add otherwise unnecessary complexity. And if we
wouldn't have to deal with that situation anyway when testing the cms
development server, I might agree that using wsgi_intercept is a good
compromise. Anyway, let's leave it up to Jon.

[Jon Sonesen, 2016/10/08 02:21:40]

Let's leave it with wsgi_intercept and if we find more compelling reasons to
justify 
the switch to a global variable and the http solution we can. Although I think
the added port logic as 
well as the additional code for handling the threads is better to avoid right
now.

-    add_wsgi_intercept(host, port, lambda: mock_api.app.wsgi_app)
+    add_wsgi_intercept(host, port, lambda: app.wsgi_app)
     yield None
     remove_wsgi_intercept()
 
 
-def test_sync(temp_site, make_intercept, make_crowdin_zip):
+def test_sync(temp_site, make_intercept, make_crowdin_zip, expect_requests):
     translate.crowdin_sync(temp_site, 'test_key')
-    for data_set in mock_api.app.request_data.items():
-        assert data_set is not None

[Jon Sonesen, 2016/10/05 03:54:49]

this is just a preliminary assertion. I am open to suggestions on how to improve
this. Currently it is just a string with the post data, but it seems that as
long as there actually is data there it can be safe to assume it is correct.
Also, the api only adds stuff here if there is data, but it may make more sense
to always add members to the dict, and set the key to the method type, then
assert that POST keys are not missing data

+    for (url, data), (expect_url, expect_data) in zip(
+                                                      app.request_log,
+                                                      expect_requests):
+        assert expect_url in url
+        assert expect_data in data