Issue 29912588: Issue 7019 - [CMS] Refactor `test_server.py` (Closed)
|
Created:
Oct. 16, 2018, 11:42 a.m. by Tudor Avram Modified:
Oct. 30, 2018, 5:04 p.m. Visibility:
Public. |
DescriptionIssue 7019 - Refactor `test_server.py`
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed commments from Patch Set #1. Objectified server handler. Added test cases. #Patch Set 3 : Updated exception tests. Removed duplicates from ignores #
Total comments: 34
Patch Set 4 : Addressed comments from Patch Set #3 #
Total comments: 2
Patch Set 5 : Added timeout option when setting up server #Patch Set 6 : Added test_and_wait to test server fixture #
Total comments: 2
Patch Set 7 : Simplified server waiting logic #Patch Set 8 : Addressed docstring nit #
MessagesTotal messages: 23
Hi Vasily, After what seemed to be ages, I finally managed to (almost-fully) test the refactored test server. As a bonus, I removed the `--disable-warnings` flag when running the tests. We don't really need it anymore and it was causing stdout capturing to fail when running the server in a separate thread. Let me know what you think! Thanks, Tudor.
Hi Tudor, Some comments from the quick look through. Looking forward to objectificatio... err, I mean classificati... well, you know what I mean... of test_server.py Cheers, Vasily https://codereview.adblockplus.org/29912588/diff/29912589/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29912588/diff/29912589/cms/bin/test_server... cms/bin/test_server.py:29: class Parameters: As we discussed via IRC, let's make this whole thing a class! Then we won't need these global variables. https://codereview.adblockplus.org/29912588/diff/29912589/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29912588/diff/29912589/tests/test_page_out... tests/test_page_outputs.py:61: def dynamic_server_failing_import(temp_site, monkeypatch): This fixture should probably just be called `dynamic_builtins_server` or something like that since we're not doing the failing import thingie anymore. https://codereview.adblockplus.org/29912588/diff/29912589/tests/test_page_out... tests/test_page_outputs.py:80: urllib2.urlopen(mock_params.base_url + 'some_page') I think it would be better to join the thread here, just to make sure it ends as we expect. https://codereview.adblockplus.org/29912588/diff/29912589/tests/utils.py File tests/utils.py (right): https://codereview.adblockplus.org/29912588/diff/29912589/tests/utils.py#newc... tests/utils.py:50: This looks kind of unrelated. Probably better remove it to make the review smaller :D https://codereview.adblockplus.org/29912588/diff/29912589/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29912588/diff/29912589/tox.ini#newcode9 tox.ini:9: #/cms/bin/test_server.py : A101,A107,A302,E501,F401 Can we just delete this now? https://codereview.adblockplus.org/29912588/diff/29912589/tox.ini#newcode40 tox.ini:40: py.test --cov-report term --cov-report html --cov=cms tests woop woop!
Hi Vasily, I addressed your comments and objectified the server handler. On top of that, I added a couple of test cases (i.e. for `404 Not Found` and for page conflicts), as discussed in IRC. Thanks for your help with the coverage thingy, by the way! Looking forward for your feedback! Thanks, Tudor. https://codereview.adblockplus.org/29912588/diff/29912589/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29912588/diff/29912589/cms/bin/test_server... cms/bin/test_server.py:29: class Parameters: On 2018/10/16 13:18:23, Vasily Kuznetsov wrote: > As we discussed via IRC, let's make this whole thing a class! Then we won't need > these global variables. Done. https://codereview.adblockplus.org/29912588/diff/29912589/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29912588/diff/29912589/tests/test_page_out... tests/test_page_outputs.py:61: def dynamic_server_failing_import(temp_site, monkeypatch): On 2018/10/16 13:18:23, Vasily Kuznetsov wrote: > This fixture should probably just be called `dynamic_builtins_server` or > something like that since we're not doing the failing import thingie anymore. Done. https://codereview.adblockplus.org/29912588/diff/29912589/tests/test_page_out... tests/test_page_outputs.py:80: urllib2.urlopen(mock_params.base_url + 'some_page') On 2018/10/16 13:18:23, Vasily Kuznetsov wrote: > I think it would be better to join the thread here, just to make sure it ends as > we expect. No longer relevant https://codereview.adblockplus.org/29912588/diff/29912589/tests/utils.py File tests/utils.py (right): https://codereview.adblockplus.org/29912588/diff/29912589/tests/utils.py#newc... tests/utils.py:50: On 2018/10/16 13:18:23, Vasily Kuznetsov wrote: > This looks kind of unrelated. Probably better remove it to make the review > smaller :D Done. https://codereview.adblockplus.org/29912588/diff/29912589/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29912588/diff/29912589/tox.ini#newcode9 tox.ini:9: #/cms/bin/test_server.py : A101,A107,A302,E501,F401 On 2018/10/16 13:18:23, Vasily Kuznetsov wrote: > Can we just delete this now? Done. https://codereview.adblockplus.org/29912588/diff/29912589/tox.ini#newcode40 tox.ini:40: py.test --cov-report term --cov-report html --cov=cms tests On 2018/10/16 13:18:24, Vasily Kuznetsov wrote: > woop woop! Done.
Hi Vasily, I updated the exception testing (as mentioned in IRC). I also removed the duplicated `.idea/` from `.gitignore` and `.hgignore`. Thanks, Tudor.
Hi Tudor, I still have lots of comments but they are mostly about documentation and organizing things more neatly. Should be pretty easy to address them. Overall this definitely exceeds my expectations for what would come out of this ticket! Cheers, Vasily https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:20: from argparse import ArgumentParser In some cases we get easier to follow code by importing modules and then using the dotted notation (e.g. import os --> foo = os.path.join(bar, baz)) as opposed to importing the functions and classes (e.g. from os.path import join --> foo = join(bar, baz)). Google style guide [1] specifically recommends the former approach and I'm starting to think that they are right. We don't have anything about this in our style guide though. I see you replaced the module import with a class import here. My first impulse is to say that this is in contradiction to the thinking above and is not desirable. However, I can also see that it's pretty clear where ArgumentParser is coming from, so any confusion is unlikely. Still, there's an argument for a consistent style. So at the end of the day... it seems complicated :/. What was your thinking when you changed this? [1]: https://github.com/google/styleguide/blob/gh-pages/pyguide.md#22-imports https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:82: str/ bytes Numpy docstring standard [1], that we use here seems to suggest using literal "or" in such cases, so it would be "str or bytes". [1]: https://numpydoc.readthedocs.io/en/latest/format.html#sections https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:106: (str, str) Numpy docstring standard (link in the previous comment) has two possible formats for the return. I think here we could use the second one nicely: Returns -------- (page_name, page_contents) : (str, str) The name of the page and its rendered contents. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:138: Parameters Nit: there should be an empty line before the header https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:170: Needs to have the following format: "<status_code> It would be better to not break the line the middle of this quoted string, otherwise it's not very clear what exactly the format is. Maybe we just move the whole thing to the next line? https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:175: of a `jinja2 Template`. Nit: redundant line break above? https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:180: of utf8 strings - fragments of the corresponding error HTML I think "of utf8 strings" should be part of the "type" here. I've seen such "human-readable" types used a lot in NumPy style docstrings and I think they work pretty well. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:194: """Execute the handler, according with the WSGI standards. Nit: according to the internet the normal form is "according to" or "in accordance with" but not "according with". https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:209: list It's "list of str", right? Perhaps we can add that to the type. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:231: def get_handler(): I was imagining a structure where we parse the arguments in this function and return `args` that are be used in `main()` to instantiate the server. Bundling the two together feels a bit strange. What do you think about this approach? https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:278: import logging Is it necessary to import logging here and not at the top? Seems a bit weird, doesn't it? https://codereview.adblockplus.org/29912588/diff/29915561/tests/conftest.py File tests/conftest.py (right): https://codereview.adblockplus.org/29912588/diff/29915561/tests/conftest.py#n... tests/conftest.py:32: with open(os.path.join(site_dir, 'locales', 'en', 'translate'), 'w') \ Nit: maybe construct the path in a separate line to avoid the ugly line break? https://codereview.adblockplus.org/29912588/diff/29915561/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29912588/diff/29915561/tests/test_page_out... tests/test_page_outputs.py:52: werkzeug_dir = tmpdir.mkdir('werkzeug') Perhaps we can add a comment here explaining what the hell's going on in this fixture. Right now we both understand it well, but it's far from obvious ;) https://codereview.adblockplus.org/29912588/diff/29915561/tests/test_page_out... tests/test_page_outputs.py:54: werkzeug_dir.join('serving.py').write('raise ImportError') It seems that this line is actually not necessary. Just a broken __init__.py is enough for werkzeug to not import. https://codereview.adblockplus.org/29912588/diff/29915561/tests/test_page_out... tests/test_page_outputs.py:82: @pytest.mark.slowtest It seems that now these four slowtests with related fixtures can be moved into a separate file. They are not really about testing page outputs but are about testing the test_server. What do you think?
Hi Vasily, Thanks for your comments! I replied to a couple of them, let me know if you have any further thoughts. Thanks, Tudor. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:20: from argparse import ArgumentParser On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > In some cases we get easier to follow code by importing modules and then using > the dotted notation (e.g. import os --> foo = os.path.join(bar, baz)) as opposed > to importing the functions and classes (e.g. from os.path import join --> foo = > join(bar, baz)). Google style guide [1] specifically recommends the former > approach and I'm starting to think that they are right. We don't have anything > about this in our style guide though. > > I see you replaced the module import with a class import here. My first impulse > is to say that this is in contradiction to the thinking above and is not > desirable. However, I can also see that it's pretty clear where ArgumentParser > is coming from, so any confusion is unlikely. Still, there's an argument for a > consistent style. So at the end of the day... it seems complicated :/. > > What was your thinking when you changed this? > > [1]: https://github.com/google/styleguide/blob/gh-pages/pyguide.md#22-imports Well, basically the idea was to make the entire thing less verbose. As you said, it seemed to me that `ArgumentParser` was obvious enough and there was need to write `argparse.ArgumentParser`. But yeah, I guess I can just change that to comply with the standards. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:278: import logging On 2018/10/22 14:36:46, Vasily Kuznetsov wrote: > Is it necessary to import logging here and not at the top? Seems a bit weird, > doesn't it? It's not used anywhere else in the script, that's why I put it here (it would only be imported if `werkzeug` is installed when running the script. Thus, it wouldn't be unnecessarily imported.
Hey Tudor! See my replies below. Cheers, Vasily https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:20: from argparse import ArgumentParser On 2018/10/22 17:29:38, Tudor Avram wrote: > On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > > In some cases we get easier to follow code by importing modules and then using > > the dotted notation (e.g. import os --> foo = os.path.join(bar, baz)) as > opposed > > to importing the functions and classes (e.g. from os.path import join --> foo > = > > join(bar, baz)). Google style guide [1] specifically recommends the former > > approach and I'm starting to think that they are right. We don't have anything > > about this in our style guide though. > > > > I see you replaced the module import with a class import here. My first > impulse > > is to say that this is in contradiction to the thinking above and is not > > desirable. However, I can also see that it's pretty clear where ArgumentParser > > is coming from, so any confusion is unlikely. Still, there's an argument for a > > consistent style. So at the end of the day... it seems complicated :/. > > > > What was your thinking when you changed this? > > > > [1]: https://github.com/google/styleguide/blob/gh-pages/pyguide.md#22-imports > > Well, basically the idea was to make the entire thing less verbose. As you said, > it seemed to me that `ArgumentParser` was obvious enough and there was need to > write `argparse.ArgumentParser`. > > But yeah, I guess I can just change that to comply with the standards. I see. Well, at the moment there's no standard about this and perhaps we should think if such standard would make sense (let me know what you think) and, if it does, introduce it. As for now both styles are ok, so you can leave it as is. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:278: import logging On 2018/10/22 17:29:38, Tudor Avram wrote: > On 2018/10/22 14:36:46, Vasily Kuznetsov wrote: > > Is it necessary to import logging here and not at the top? Seems a bit weird, > > doesn't it? > > It's not used anywhere else in the script, that's why I put it here (it would > only be imported if `werkzeug` is installed when running the script. Thus, it > wouldn't be unnecessarily imported. I see. Importing `logging` is probably not a big deal performance-wise, but yeah, it kind of makes sense. What if we move the import to line 273 instead, just after the werkzeug import?
Hi Vasily, I addressed your comments from the last patch. Let me know what you think. Thanks, Tudor. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:20: from argparse import ArgumentParser On 2018/10/23 10:16:14, Vasily Kuznetsov wrote: > On 2018/10/22 17:29:38, Tudor Avram wrote: > > On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > > > In some cases we get easier to follow code by importing modules and then > using > > > the dotted notation (e.g. import os --> foo = os.path.join(bar, baz)) as > > opposed > > > to importing the functions and classes (e.g. from os.path import join --> > foo > > = > > > join(bar, baz)). Google style guide [1] specifically recommends the former > > > approach and I'm starting to think that they are right. We don't have > anything > > > about this in our style guide though. > > > > > > I see you replaced the module import with a class import here. My first > > impulse > > > is to say that this is in contradiction to the thinking above and is not > > > desirable. However, I can also see that it's pretty clear where > ArgumentParser > > > is coming from, so any confusion is unlikely. Still, there's an argument for > a > > > consistent style. So at the end of the day... it seems complicated :/. > > > > > > What was your thinking when you changed this? > > > > > > [1]: > https://github.com/google/styleguide/blob/gh-pages/pyguide.md#22-imports > > > > Well, basically the idea was to make the entire thing less verbose. As you > said, > > it seemed to me that `ArgumentParser` was obvious enough and there was need to > > write `argparse.ArgumentParser`. > > > > But yeah, I guess I can just change that to comply with the standards. > > I see. Well, at the moment there's no standard about this and perhaps we should > think if such standard would make sense (let me know what you think) and, if it > does, introduce it. As for now both styles are ok, so you can leave it as is. I guess it's more important to make it obvious where classes/ methods comes from. In consequence, I changed this to `import argparse` and then using `argparse.ArgumentParser(...)` https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:82: str/ bytes On 2018/10/22 14:36:46, Vasily Kuznetsov wrote: > Numpy docstring standard [1], that we use here seems to suggest using literal > "or" in such cases, so it would be "str or bytes". > > [1]: https://numpydoc.readthedocs.io/en/latest/format.html#sections Done. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:106: (str, str) On 2018/10/22 14:36:46, Vasily Kuznetsov wrote: > Numpy docstring standard (link in the previous comment) has two possible formats > for the return. I think here we could use the second one nicely: > > Returns > -------- > (page_name, page_contents) : (str, str) > The name of the page and its rendered contents. Done. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:138: Parameters On 2018/10/22 14:36:46, Vasily Kuznetsov wrote: > Nit: there should be an empty line before the header Done. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:170: Needs to have the following format: "<status_code> On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > It would be better to not break the line the middle of this quoted string, > otherwise it's not very clear what exactly the format is. Maybe we just move the > whole thing to the next line? Done. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:175: of a `jinja2 Template`. On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > Nit: redundant line break above? Done. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:180: of utf8 strings - fragments of the corresponding error HTML On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > I think "of utf8 strings" should be part of the "type" here. I've seen such > "human-readable" types used a lot in NumPy style docstrings and I think they > work pretty well. Done. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:194: """Execute the handler, according with the WSGI standards. On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > Nit: according to the internet the normal form is "according to" or "in > accordance with" but not "according with". Done. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:209: list On 2018/10/22 14:36:46, Vasily Kuznetsov wrote: > It's "list of str", right? Perhaps we can add that to the type. Done. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:231: def get_handler(): On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > I was imagining a structure where we parse the arguments in this function and > return `args` that are be used in `main()` to instantiate the server. Bundling > the two together feels a bit strange. What do you think about this approach? Done. https://codereview.adblockplus.org/29912588/diff/29915561/cms/bin/test_server... cms/bin/test_server.py:278: import logging On 2018/10/23 10:16:14, Vasily Kuznetsov wrote: > On 2018/10/22 17:29:38, Tudor Avram wrote: > > On 2018/10/22 14:36:46, Vasily Kuznetsov wrote: > > > Is it necessary to import logging here and not at the top? Seems a bit > weird, > > > doesn't it? > > > > It's not used anywhere else in the script, that's why I put it here (it would > > only be imported if `werkzeug` is installed when running the script. Thus, it > > wouldn't be unnecessarily imported. > > I see. Importing `logging` is probably not a big deal performance-wise, but > yeah, it kind of makes sense. What if we move the import to line 273 instead, > just after the werkzeug import? Done. https://codereview.adblockplus.org/29912588/diff/29915561/tests/conftest.py File tests/conftest.py (right): https://codereview.adblockplus.org/29912588/diff/29915561/tests/conftest.py#n... tests/conftest.py:32: with open(os.path.join(site_dir, 'locales', 'en', 'translate'), 'w') \ On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > Nit: maybe construct the path in a separate line to avoid the ugly line break? Done. https://codereview.adblockplus.org/29912588/diff/29915561/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29912588/diff/29915561/tests/test_page_out... tests/test_page_outputs.py:52: werkzeug_dir = tmpdir.mkdir('werkzeug') On 2018/10/22 14:36:48, Vasily Kuznetsov wrote: > Perhaps we can add a comment here explaining what the hell's going on in this > fixture. Right now we both understand it well, but it's far from obvious ;) Done. https://codereview.adblockplus.org/29912588/diff/29915561/tests/test_page_out... tests/test_page_outputs.py:54: werkzeug_dir.join('serving.py').write('raise ImportError') On 2018/10/22 14:36:47, Vasily Kuznetsov wrote: > It seems that this line is actually not necessary. Just a broken __init__.py is > enough for werkzeug to not import. Done. https://codereview.adblockplus.org/29912588/diff/29915561/tests/test_page_out... tests/test_page_outputs.py:82: @pytest.mark.slowtest On 2018/10/22 14:36:48, Vasily Kuznetsov wrote: > It seems that now these four slowtests with related fixtures can be moved into a > separate file. They are not really about testing page outputs but are about > testing the test_server. What do you think? Done.
Hi Vasily, I addressed your comments from the last patch. Let me know what you think. Thanks, Tudor.
LGTM One note: currently the tests for dynamic servers heisen-break on my computer. If I bump the delay after launching the server to 1 second, they start to pass, but that also seems like a half-assed solution. Perhaps we should implement a server launch delay, something like this: https://gist.github.com/butla/2d9a4c0f35ea47b7452156c96a4e7b12 -- it would probably be a separate issue (or noissue) though. https://codereview.adblockplus.org/29912588/diff/29921555/cms/bin/test_server.py File cms/bin/test_server.py (right): https://codereview.adblockplus.org/29912588/diff/29921555/cms/bin/test_server... cms/bin/test_server.py:233: argparse.Namespace Cool! That's quite some detail, many people don't even know this class exists :D https://codereview.adblockplus.org/29912588/diff/29921555/tests/test_dynamic_... File tests/test_dynamic_server.py (right): https://codereview.adblockplus.org/29912588/diff/29921555/tests/test_dynamic_... tests/test_dynamic_server.py:19: # Creating an invalid `werkzeug` module and add it to the head of the 👍
Hey, Thanks for this! I can try to add default waiting for a TCP port to the test server and add it as an extra patch, so we can merge it all at once. What do you think? Tudor.
Hi Vasily, It's all done :D. Let me know if you have any further comments. Thanks, Tudor.
On 2018/10/24 15:59:37, Tudor Avram wrote: > Hi Vasily, > > It's all done :D. Let me know if you have any further comments. > > Thanks, > Tudor. Ah, wait, that's not what I meant 😆 What you implemented is that the server waits a bit if the port is not available instead of immediately failing. This could be useful sometimes, but it seems that in sane cases we don't need this. What I was proposing to the test failures that I have observed was to make the fixture that launches web servers and then waits until something is listening on port 5000 (in a way similar to this gist: https://gist.github.com/butla/2d9a4c0f35ea47b7452156c96a4e7b12). We would replace our current 0.5 second delay, that sometimes turns out to be insufficient, with a flexible delay that checks and waits until the server is up and listening. This way the tests would not fail because they try to get pages from our server fixture before it started listening on port 5000. Sorry for not being more clear yesterday 😖 Does this make sense or am I misunderstanding something? Cheers, Vasily
Hi Vasily, Changed it now :D Sorry about the confusion. I set a hard limit of 2 seconds after which the server setup would fail. Let me know what you think. Tudor.
Hey guys, For some reason, this review literally just showed up for me on my issues page. This actually sort of leads me to (probably way too late) suggest that we just use flask here, since it's already a cms dependency and the code would shrink considerably and we dont have to rewrite a wsgi
Hey guys, For some reason, this review literally just showed up for me on my issues page. This actually sort of leads me to (probably way too late) suggest that we just use flask here, since it's already a cms dependency and the code would shrink considerably and we dont have to rewrite a wsgi
Hey guys, For some reason, this review literally just showed up for me on my issues page. This actually sort of leads me to (probably way too late) suggest that we just use flask here, since it's already a cms dependency and the code would shrink considerably and we dont have to rewrite a wsgi
Overall though this looks great :) thanks tudor
On 2018/10/25 15:39:17, Jon Sonesen wrote: > Hey guys, > > For some reason, this review literally just showed up for me on my issues page. > > This actually sort of leads me to (probably way too late) suggest that we just > use flask here, since it's already a cms dependency and the code would shrink > considerably and we dont have to rewrite a wsgi Hi Jon! I'm not sure if Flask would remove that much code, actually. It's good at fancy routing (which we specifically don't need, because we are doing our own routing and we want it this way) and things like request parsing (but we're just impersonating a dumb static server) and Jinja integration (but we do our own Jinja hookup for better of worse). So seems like we don't really benefit from Flask much as opposed to Werkzeug. Maybe I'm missing something though, we can try to use Flask instead of Werkzeug and see what happens (although I would prefer to land this review and then look into that). Cheers, Vasily
Hi Tudor, This looks good, but we can simplify the checking a bit (see below). Cheers, Vasily https://codereview.adblockplus.org/29912588/diff/29924555/tests/utils.py File tests/utils.py (right): https://codereview.adblockplus.org/29912588/diff/29924555/tests/utils.py#newc... tests/utils.py:70: os.killpg(os.getpgid(p.pid), signal.SIGINT) We can get rid of the replication of this line using a try/finally block. Like this: try: _wait_for_server() yield 'http://.....' finally: os.killpg(...) and then _wait_for_server() would just reraise the exception once the timeout is expired: I don't think the fancy handling adds that much value here.
On 2018/10/25 15:50:13, Vasily Kuznetsov wrote: > On 2018/10/25 15:39:17, Jon Sonesen wrote: > > Hey guys, > > > > For some reason, this review literally just showed up for me on my issues > page. > > > > This actually sort of leads me to (probably way too late) suggest that we just > > use flask here, since it's already a cms dependency and the code would shrink > > considerably and we dont have to rewrite a wsgi > > Hi Jon! > > I'm not sure if Flask would remove that much code, actually. It's good at fancy > routing (which we specifically don't need, because we are doing our own routing > and we want it this way) and things like request parsing (but we're just > impersonating a dumb static server) and Jinja integration (but we do our own > Jinja hookup for better of worse). So seems like we don't really benefit from > Flask much as opposed to Werkzeug. > > Maybe I'm missing something though, we can try to use Flask instead of Werkzeug > and see what happens (although I would prefer to land this review and then look > into that). > > Cheers, > Vasily Yeah, we should land this first. As I said it looks great overall, I will spend some time on it today. I am happy to see the refactor :)
Hi Vasily, I simplified the logic in `_wait_for_server`, as you suggested. Tudor. https://codereview.adblockplus.org/29912588/diff/29924555/tests/utils.py File tests/utils.py (right): https://codereview.adblockplus.org/29912588/diff/29924555/tests/utils.py#newc... tests/utils.py:70: os.killpg(os.getpgid(p.pid), signal.SIGINT) On 2018/10/25 15:51:43, Vasily Kuznetsov wrote: > We can get rid of the replication of this line using a try/finally block. Like > this: > > try: > _wait_for_server() > yield 'http://.....' > finally: > os.killpg(...) > > and then _wait_for_server() would just reraise the exception once the timeout is > expired: I don't think the fancy handling adds that much value here. Done.
LGTM It seems that .gitignore and .hgignore changes are not necessary (and fail to apply) but I'll correct this locally. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
