Issue 29355396: Issue 4380 - Add Mock Crowdin API and Sync Test (Closed)
|
Created:
Sept. 30, 2016, 7:20 a.m. by Jon Sonesen Modified:
Oct. 25, 2016, 6:04 p.m. Visibility:
Public. |
DescriptionIssue 4380 - Add Mock Crowdin API and Sync Test
Repository: hg.adblockplus.org/cms
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add basic assertion for sync test #
Total comments: 22
Patch Set 3 : #
Total comments: 2
Patch Set 4 : better assertions and expected request data added #
Total comments: 2
Patch Set 5 : formatting fix #
MessagesTotal messages: 21
https://codereview.adblockplus.org/29355396/diff/29355397/tests/test_page_out... File tests/test_page_outputs.py (left): https://codereview.adblockplus.org/29355396/diff/29355397/tests/test_page_out... tests/test_page_outputs.py:50: p = subprocess.Popen(['python', 'runserver.py', temp_site]) There is a problem with Werkzeug (a dependency that is supported by the test server as well) and the way it handles SIGTERM from the subprocess library in python. This causes the server to become a zombie process. When the tests move forward it removes the directory that the server is looking to retrieve content from causing 500's my more robust handling of the SIGTERM seems to remedy this but it probably is not the best solution. Additionally, I chose to utilize flask here because it made sense to me for there to be the simplest api possible that does not incur maintenance efforts on our part. I figure letting someone else handle the maintenance of that code was smart. But I am open to changing that as well.
here are some comments https://codereview.adblockplus.org/29355396/diff/29355397/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355397/tests/test_translat... tests/test_translations.py:16: shutil.make_archive(zip_name, 'zip', input_dir) the archive is not made in the temp site because the api has know way of knowing that path, also not sure about assigning variables here for those paths
https://codereview.adblockplus.org/29355396/diff/29355397/tests/mock_api.py File tests/mock_api.py (right): https://codereview.adblockplus.org/29355396/diff/29355397/tests/mock_api.py#n... tests/mock_api.py:1: from flask import Flask, send_from_directory, jsonify I used this in order to keep the api code as light and simple as possible so that we are not spending as much time in the future maintaining or porting large amounts of test code to python 3
I just summarised our talk about asserting the CMS behaviour below. I think we don't need to do anything to Flask console logging because it gets eaten by Pytest capturing. In case of error we'll see it but that's actually useful. https://codereview.adblockplus.org/29355396/diff/29355397/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355397/tests/test_translat... tests/test_translations.py:32: translate.crowdin_sync(temp_site, 'test_key') As we discussed, we should add assertions to this call. Basically apart from making sure it doesn't crash, we also need to make sure it did what we expected it to do. Some ideas about what and how we could check (basically notes from our quick brainstorm session): - That the CMS got the translations from our API mock and put them where they should be. Just look at the filesystem and check that the right files are there. - That the CMS called the API mock as we expect with expected parameters. We could add an attribute to `mock_api.app` that is a list and make sure our handlers add the relevant logging data to this list. Then we can return this list from the fixture as its value so inside the test we can look at what's there after the call and make sure it's right.
https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py File tests/mock_api.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py#n... tests/mock_api.py:4: app.request_data = {} This is my preliminary solution to the logging problem, this way it is a simple dict which can be accessed by the test suite. The, it can be iterated over to make assertions https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... tests/test_translations.py:34: assert data_set is not None 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
https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py File tests/mock_api.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py#n... tests/mock_api.py:1: from flask import Flask, send_from_directory, jsonify, request I wonder whether using Flask here would be overkill. I know, the additional dependency would only be for the tests. But IMO it wouldn't be much (if any) more complicated to use Werkzeug (http://werkzeug.pocoo.org/docs/tutorial) directly. It is the low-level web framework which Flask is implemented on top. Also note that the CMS is already using Werkzeug (optionally) for its own development server. What do you think? https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py#n... tests/mock_api.py:1: from flask import Flask, send_from_directory, jsonify, request The name of this module is a little to generic, given that this is going to land in the CMS, where CrowdIn integration isn't the core feature. How about calling it "crowdin_mock_api" (or some other name that contains "crowdin")? https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_site/loc... File tests/test_site/locales/en/translate.json (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_site/loc... tests/test_site/locales/en/translate.json:1: { Note that the source translation (i.e. English) is usually extracted out of the page, rather than provided by a JSON file. So having the source string both, in a JSON file, and in-inline in pages/translate.md seems redundant. https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_site/set... File tests/test_site/settings.ini (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_site/set... tests/test_site/settings.ini:6: [locale_overrides] This has obviously been copied from web.adblockplus.org. I wonder whether this section is relevant for your tests? And if so, I'd prefer to not duplicate the full list but just a few simple examples (note that those doesn't necessarily need to be browser names). https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... tests/test_translations.py:25: urllib3_intercept.install() 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. https://codereview.adblockplus.org/29355396/diff/29355828/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29355396/diff/29355828/tox.ini#newcode26 tox.ini:26: pytest-mock Mind keeping the test dependencies above the flake8 stuff?
https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py File tests/mock_api.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py#n... tests/mock_api.py:1: from flask import Flask, send_from_directory, jsonify, request On 2016/10/05 12:06:16, Sebastian Noack wrote: > I wonder whether using Flask here would be overkill. I know, the additional > dependency would only be for the tests. But IMO it wouldn't be much (if any) > more complicated to use Werkzeug (http://werkzeug.pocoo.org/docs/tutorial) > directly. It is the low-level web framework which Flask is implemented on top. > Also note that the CMS is already using Werkzeug (optionally) for its own > development server. What do you think? I am aware that Flask is implemented on top of Werkzeug, my argument here is that the code I write in making my own wsgi app with Werkzeug is already implemented by Flask, additionally the Flask team will be maintaining that code. The alternative is that I reinvent the wheel that Flask has already invented. Currently this app is super small, and easy to read and maintain. However if you feel strongly about this I can just implement it on top of werkzeug. https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py#n... tests/mock_api.py:1: from flask import Flask, send_from_directory, jsonify, request On 2016/10/05 12:06:16, Sebastian Noack wrote: > The name of this module is a little to generic, given that this is going to land > in the CMS, where CrowdIn integration isn't the core feature. How about calling > it "crowdin_mock_api" (or some other name that contains "crowdin")? yes that is a good point and I will do so. https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_site/set... File tests/test_site/settings.ini (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_site/set... tests/test_site/settings.ini:6: [locale_overrides] On 2016/10/05 12:06:16, Sebastian Noack wrote: > This has obviously been copied from http://web.adblockplus.org. I wonder whether this > section is relevant for your tests? And if so, I'd prefer to not duplicate the > full list but just a few simple examples (note that those doesn't necessarily > need to be browser names). yeah these are not needed https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... tests/test_translations.py:25: urllib3_intercept.install() On 2016/10/05 12:06:16, Sebastian Noack wrote: > 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. 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. https://codereview.adblockplus.org/29355396/diff/29355828/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29355396/diff/29355828/tox.ini#newcode26 tox.ini:26: pytest-mock On 2016/10/05 12:06:16, Sebastian Noack wrote: > Mind keeping the test dependencies above the flake8 stuff? Acknowledged.
https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py File tests/mock_api.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py#n... tests/mock_api.py:4: app.request_data = {} On 2016/10/05 03:54:49, Jon Sonesen wrote: > This is my preliminary solution to the logging problem, this way it is a simple > dict which can be accessed by the test suite. The, it can be iterated over to > make assertions Perhaps it would make more sense to make this a list of tuples `(url, data)` instead to be able to see in which order the requests were made and take care of the possibility that we'll have several requests to the same url. https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... tests/test_translations.py:25: urllib3_intercept.install() On 2016/10/05 12:06:16, Sebastian Noack wrote: > 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. 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.
https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py File tests/mock_api.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py#n... tests/mock_api.py:1: from flask import Flask, send_from_directory, jsonify, request On 2016/10/05 14:38:13, Jon Sonesen wrote: > On 2016/10/05 12:06:16, Sebastian Noack wrote: > > I wonder whether using Flask here would be overkill. I know, the additional > > dependency would only be for the tests. But IMO it wouldn't be much (if any) > > more complicated to use Werkzeug (http://werkzeug.pocoo.org/docs/tutorial) > > directly. It is the low-level web framework which Flask is implemented on top. > > Also note that the CMS is already using Werkzeug (optionally) for its own > > development server. What do you think? > > I am aware that Flask is implemented on top of Werkzeug, my argument here is > that the code > I write in making my own wsgi app with Werkzeug is already implemented by Flask, > additionally the Flask team will be maintaining that code. The alternative is > that I reinvent the > wheel that Flask has already invented. Currently this app is super small, and > easy to read and maintain. > However if you feel strongly about this I can just implement it on top of > werkzeug. Werkzeug might not be as low-level as you think. It just has a somewhat different flavor, e.g instead of using the route() decorator, you use Map and Rule objects: http://werkzeug.pocoo.org/docs/routing/ Flask does more magic and has more shortcuts, keeping the code a little more concise, but not necessarily easier to understand. And you are definitely not "reinventing the wheel" if you use Werkzeug directly here. In particular if you have to figure out what is happening under the hood, either to debug an issue or because a given API doesn't seem powerful enough, each layer you have to go through makes it more complicated. Note that for those reasons, we avoided Flask so far. However, it doesn't matter as much here, since it is just test code. https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... tests/test_translations.py:25: urllib3_intercept.install() On 2016/10/05 14:44:42, Vasily Kuznetsov wrote: > On 2016/10/05 12:06:16, Sebastian Noack wrote: > > 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. > > 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. 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. > The only part that is not covered by it is the urllib3 internals and we don't want to test them anyway. 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. > 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. 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.
https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... tests/test_translations.py:25: urllib3_intercept.install() On 2016/10/05 16:36:44, Sebastian Noack wrote: > On 2016/10/05 14:44:42, Vasily Kuznetsov wrote: > > On 2016/10/05 12:06:16, Sebastian Noack wrote: > > > 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. > > > > 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. > > 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. > > > The only part that is not covered by it is the urllib3 internals and we don't > want to test them anyway. > > 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. > > > 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. > > 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. 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?
https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... tests/test_translations.py:25: urllib3_intercept.install() On 2016/10/06 09:38:59, Vasily Kuznetsov wrote: > On 2016/10/05 16:36:44, Sebastian Noack wrote: > > On 2016/10/05 14:44:42, Vasily Kuznetsov wrote: > > > On 2016/10/05 12:06:16, Sebastian Noack wrote: > > > > 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. > > > > > > 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. > > > > 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. > > > > > The only part that is not covered by it is the urllib3 internals and we > don't > > want to test them anyway. > > > > 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. > > > > > 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. > > > > 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. > > 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? 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.
https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py File tests/mock_api.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py#n... tests/mock_api.py:1: from flask import Flask, send_from_directory, jsonify, request On 2016/10/05 16:36:44, Sebastian Noack wrote: > On 2016/10/05 14:38:13, Jon Sonesen wrote: > > On 2016/10/05 12:06:16, Sebastian Noack wrote: > > > I wonder whether using Flask here would be overkill. I know, the additional > > > dependency would only be for the tests. But IMO it wouldn't be much (if any) > > > more complicated to use Werkzeug (http://werkzeug.pocoo.org/docs/tutorial) > > > directly. It is the low-level web framework which Flask is implemented on > top. > > > Also note that the CMS is already using Werkzeug (optionally) for its own > > > development server. What do you think? > > > > I am aware that Flask is implemented on top of Werkzeug, my argument here is > > that the code > > I write in making my own wsgi app with Werkzeug is already implemented by > Flask, > > additionally the Flask team will be maintaining that code. The alternative is > > that I reinvent the > > wheel that Flask has already invented. Currently this app is super small, and > > easy to read and maintain. > > However if you feel strongly about this I can just implement it on top of > > werkzeug. > > Werkzeug might not be as low-level as you think. It just has a somewhat > different flavor, e.g instead of using the route() decorator, you use Map and > Rule objects: http://werkzeug.pocoo.org/docs/routing/ > > Flask does more magic and has more shortcuts, keeping the code a little more > concise, but not necessarily easier to understand. And you are definitely not > "reinventing the wheel" if you use Werkzeug directly here. In particular if you > have to figure out what is happening under the hood, either to debug an issue or > because a given API doesn't seem powerful enough, each layer you have to go > through makes it more complicated. Note that for those reasons, we avoided Flask > so far. However, it doesn't matter as much here, since it is just test code. I see, I think that if you are okay with it that sticking with what we have would be my preferred option. Also, how would going the werkzeug route affect the list of tuples solution for logging the requests made by the translate module? https://codereview.adblockplus.org/29355396/diff/29355828/tests/mock_api.py#n... tests/mock_api.py:4: app.request_data = {} On 2016/10/05 14:44:42, Vasily Kuznetsov wrote: > On 2016/10/05 03:54:49, Jon Sonesen wrote: > > This is my preliminary solution to the logging problem, this way it is a > simple > > dict which can be accessed by the test suite. The, it can be iterated over to > > make assertions > > Perhaps it would make more sense to make this a list of tuples `(url, data)` > instead to be able to see in which order the requests were made and take care of > the possibility that we'll have several requests to the same url. I agree here. Will do! https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29355828/tests/test_translat... tests/test_translations.py:25: urllib3_intercept.install() On 2016/10/06 10:25:26, Sebastian Noack wrote: > On 2016/10/06 09:38:59, Vasily Kuznetsov wrote: > > On 2016/10/05 16:36:44, Sebastian Noack wrote: > > > On 2016/10/05 14:44:42, Vasily Kuznetsov wrote: > > > > On 2016/10/05 12:06:16, Sebastian Noack wrote: > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > > > > The only part that is not covered by it is the urllib3 internals and we > > don't > > > want to test them anyway. > > > > > > 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. > > > > > > > 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. > > > > > > 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. > > > > 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? > > 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. 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.
Looks good. Now it would be good to add some more meaningful assertions to that test (see below). https://codereview.adblockplus.org/29355396/diff/29356354/tests/crowdin_mock_... File tests/crowdin_mock_api.py (right): https://codereview.adblockplus.org/29355396/diff/29356354/tests/crowdin_mock_... tests/crowdin_mock_api.py:4: app.request_data = [] I think this name doesn't really tell us that it's basically a log of the requests that the mock received. Maybe better rename it to `request_log` or something like that? https://codereview.adblockplus.org/29355396/diff/29356354/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29356354/tests/test_translat... tests/test_translations.py:34: assert data_set is not None It would be better to not just check that there were _some_ request and that they are not None (it seems that there can't be any None's there anyway unless the testing code is broken) but to make sure that the code under test sent the requests that we expect. At this point we basically want to capture the existing behavior so what I would do is dump `crowdin_mock_api.app.request_data` to a file, then look at it and and for each request take a characteristic part of the URL and of the data. Then make a test that checks that what we're getting is right by doing something like: for (url, data), (expect_url, expect_data) in zip(request_log, expect_list): assert expect_url in url assert expect_data in data
On 2016/10/11 17:33:25, Vasily Kuznetsov wrote: > Looks good. Now it would be good to add some more meaningful assertions to that > test (see below). > > https://codereview.adblockplus.org/29355396/diff/29356354/tests/crowdin_mock_... > File tests/crowdin_mock_api.py (right): > > https://codereview.adblockplus.org/29355396/diff/29356354/tests/crowdin_mock_... > tests/crowdin_mock_api.py:4: app.request_data = [] > I think this name doesn't really tell us that it's basically a log of the > requests that the mock received. Maybe better rename it to `request_log` or > something like that? > > https://codereview.adblockplus.org/29355396/diff/29356354/tests/test_translat... > File tests/test_translations.py (right): > > https://codereview.adblockplus.org/29355396/diff/29356354/tests/test_translat... > tests/test_translations.py:34: assert data_set is not None > It would be better to not just check that there were _some_ request and that > they are not None (it seems that there can't be any None's there anyway unless > the testing code is broken) but to make sure that the code under test sent the > requests that we expect. > > At this point we basically want to capture the existing behavior so what I would > do is dump `crowdin_mock_api.app.request_data` to a file, then look at it and > and for each request take a characteristic part of the URL and of the data. Then > make a test that checks that what we're getting is right by doing something > like: > > for (url, data), (expect_url, expect_data) in zip(request_log, expect_list): > assert expect_url in url > assert expect_data in data I implemented a fixture containing the expected requests data and urls and also the above assertions. also, the reason the list of requests is so long is because I have removed the 'en/locales' dir in the test_site however i did not remove it from the mock api responses so that the sync script will actually sync something because should this be documented?
A couple of minor formatting suggestions (and I'm not super strong on those ones) but otherwise LGTM. https://codereview.adblockplus.org/29355396/diff/29357691/tests/crowdin_mock_... File tests/crowdin_mock_api.py (right): https://codereview.adblockplus.org/29355396/diff/29357691/tests/crowdin_mock_... tests/crowdin_mock_api.py:115: return jsonify( This could probably be one line. https://codereview.adblockplus.org/29355396/diff/29357691/tests/test_translat... File tests/test_translations.py (right): https://codereview.adblockplus.org/29355396/diff/29357691/tests/test_translat... tests/test_translations.py:15: ('info?key=test_key&json=1', ''), It seems that 4 spaces indent would be enough here, no need for 8.
Here are the formatting fixes
Here are the formatting fixes
On 2016/10/25 08:16:08, Jon Sonesen wrote: > Here are the formatting fixes LGTM
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
