Issue 29993614: Issue 2267 - Unify form handling by reusing form_handler() (Closed)
|
Created:
Jan. 30, 2019, 11:43 p.m. by rhowell Modified:
Feb. 8, 2019, 9 p.m. Reviewers:
Vasily Kuznetsov Base URL:
https://hg.adblockplus.org/sitescripts/ Visibility:
Public. |
DescriptionIssue 2267 - Unify form handling by reusing form_handler()
Repository: https://hg.adblockplus.org/sitescripts
Base revision of PS1: 5cccbbe6bc8d
Base revision of PS2+: 9070626bba97
Patch Set 1 #
Total comments: 3
Patch Set 2 : Get the tests (clumsily) working #
Total comments: 4
Patch Set 3 : Add mocker patches #
Total comments: 11
Patch Set 4 : Add form_handler() and encode_email_address() #
Total comments: 11
Patch Set 5 : Remove unnecessary checks #
MessagesTotal messages: 12
Hi Vasily! I've been learning a lot about the WSGI interceptor, and making a little progress on this ticket. The main spot I'm stuck at now is in updateReport.py(49)`reportData = getReport(guid)` I get an error: WSGIAppError: OperationalError(2003, "Can't connect to MySQL server on 'localhost' ([Errno 111] Connection refused)") at /home/emma/Dropbox/eyeo/hg/sitescripts/.tox/py27/local/lib/python2.7/site-packages/pymysql/connections.py:630 It seems like there might be a way for the WSGI interceptor to catch and handle this, but I haven't made much progress yet figuring out how. Unless you know of a better way? https://codereview.adblockplus.org/29993614/diff/29993615/sitescripts/__init_... File sitescripts/__init__.py (right): https://codereview.adblockplus.org/29993614/diff/29993615/sitescripts/__init_... sitescripts/__init__.py:25: pymysql.install_as_MySQLdb() updateReport.py uses some modules from sitescripts/reports/utils.py, and that file needs MySQLdb, but I kept getting import errors. I searched around online and tried a lot of different workarounds, but this was the only one that seemed to work. (https://stackoverflow.com/a/42285214)
Hi Rosie,
It seems like testing this handler properly is quite a pain -- sorry about this.
I think there are two approaches that we could take: (1) simulate a MySQL
database with the right tables and data or (2) mock the functions that talk to
the database. Whether we choose 1 or 2, we would need to make sure that
`sitescripts.reports.utils` doesn't crash when it tries to import `MySQLdb`.
I did a bit of experimentation and I found that if we put something into
`sys.modules['MySQLdb']`, the import just returns this object and doesn't
attempt to find the module on the disk:
$ python
Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec 5 2015, 12:54:16)
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.modules['MySQLdb'] = sys # Can be any module...
>>> import MySQLdb
>>> MySQLdb
<module 'sys' (built-in)>
We can use this to either replace MySQLdb with a mock for approach 1 or to
replace it with anything for approach 2.
I think approach 2 is preferable: we can just mock `getReport` and `saveReport`
in `sitescripts.reports.web.updateReport` (the first one should check the
provided id and return something that looks like a report and the second one
should check that the update happened) and after this calling `handleRequest`
should not trigger any errors and will allow us to test all of its code. We
won't test interaction with a real MySQL database this way, so it's a bit of a
shallow test, but I don't think figuring out how to fake enough of the MySQL
database to make all those functions in `sitescripts.reports.utils` work is
worthwhile in this case.
Let me know if this makes sense and if you agree. I'm also happy to answer any
further questions in IRC.
Cheers,
Vasily
https://codereview.adblockplus.org/29993614/diff/29993615/sitescripts/__init_...
File sitescripts/__init__.py (right):
https://codereview.adblockplus.org/29993614/diff/29993615/sitescripts/__init_...
sitescripts/__init__.py:21: import pymysql
I don't think that it's a good idea to make any use of sitescripts require
PyMySQL. This might break servers that don't have it.
Hey Vasily! A couple of questions/thoughts: 1. Since I've pulled and rebased, should I update the base revision in the Description? 2. I tried using `sys.modules['MySQLdb']` but wasn't sure exactly where it should go (in which file?). 3. Mostly, I wanted to check with you about the approach of using _getReport and _sendReport in updateReport.py to bypass those functions. And a couple more comments below. Thanks for the feedback! https://codereview.adblockplus.org/29993614/diff/29993615/sitescripts/__init_... File sitescripts/__init__.py (right): https://codereview.adblockplus.org/29993614/diff/29993615/sitescripts/__init_... sitescripts/__init__.py:21: import pymysql On 2019/01/31 17:34:29, Vasily Kuznetsov wrote: > I don't think that it's a good idea to make any use of sitescripts require > PyMySQL. This might break servers that don't have it. Good point. I am able to move this to sitescripts/reports/__init__.py. https://codereview.adblockplus.org/29993614/diff/29996559/sitescripts/reports... File sitescripts/reports/web/updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/29996559/sitescripts/reports... sitescripts/reports/web/updateReport.py:30: return {'usefulness': 1} If I also return an email address here, we can test more of the code in handleRequest(), but then we'd also have to intercept updateUserUsefulness() and sendUpdateNotification(). Wanted to check with you about how it looks so far before potentially going too far in the wrong direction.
Hi Rosie! The test itself looks pretty good but I think there are a few things that can be made more efficiently. > 1. Since I've pulled and rebased, should I update the base revision in the > Description? Yeah, I think it would make sense. You can keep the original revision there too, just change it to something like "Base revision of patch 1: ..." perhaps. > 2. I tried using `sys.modules['MySQLdb']` but wasn't sure exactly where it > should go (in which file?). This should be in the test, before we import the modules that use MySQLdb. If you go for this approach you won't need to install and import pymysql (I think it's better, but if you choose to not do it or if it doesn't work, see inline comment below about pymysql). > 3. Mostly, I wanted to check with you about the approach of using _getReport and > _sendReport in updateReport.py to bypass those functions. I think it's better to use mocking for that (see below). Cheers, Vasily https://codereview.adblockplus.org/29993614/diff/29996559/sitescripts/reports... File sitescripts/reports/__init__.py (right): https://codereview.adblockplus.org/29993614/diff/29996559/sitescripts/reports... sitescripts/reports/__init__.py:1: import pymysql If we only use pymysql instead of MySQLdb for testing, this import should be done in the tests. P.S. We might want to switch to pymysql in general (given that MySQLdb seems to be dead and doesn't support Python 3, this might be a good idea...) but this would need a separate ticket and review and then it would make more sense to put this import into reports/utils.py instead of here. https://codereview.adblockplus.org/29993614/diff/29996559/sitescripts/reports... File sitescripts/reports/web/updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/29996559/sitescripts/reports... sitescripts/reports/web/updateReport.py:30: return {'usefulness': 1} On 2019/02/02 05:45:22, rhowell wrote: > If I also return an email address here, we can test more of the code in > handleRequest(), but then we'd also have to intercept updateUserUsefulness() and > sendUpdateNotification(). Wanted to check with you about how it looks so far > before potentially going too far in the wrong direction. I think it would make more sense to mock getReport and saveReport (in the testing code) instead of introducing all this testing-related logic here in production code. Something like this: def test_xxx(mocker): mocker.patch('sitescripts.reports.web.updateReport.getReport', new=lambda *args: {'usefulness': 1}) mocker.patch('sitescripts.reports.web.updateReport.saveReport', new=test_saveReport) # do our thing and the patching will be undone at the end of the test. See the documentation of pytest-mock (https://pypi.org/project/pytest-mock/) for more info on how it works.
Hey Vasily, Hopefully this is looking closer to what you had in mind! Thanks for the helpful suggestions. I was hoping to get the test coverage to 100%, but there's a couple lines in updateReport.py that don't get hit (30, 34-35)(https://hg.adblockplus.org/sitescripts/file/tip/sitescripts/reports/web/updateReport.py#l30). I'm not sure how to mess with those `environ` variables. I think they're generated somewhere along the call chain, but I'm not sure where. Should I spend much more time trying to figure it out? Or do you think 94% test coverage is ok? And let me know if you see anything else! :)
Hi Rosie! The testing approach looks great, I only have some minor comments. You are still planning to add form_handler() decorator to the handleRequest() and use the email handling thingie, right? Cheers, Vasily https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... File sitescripts/reports/tests/test_updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:24: sys.modules['MySQLdb'] = sys We should probably add a comment here explaining why we need this line. What do you think? https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:73: mocker.patch('sitescripts.reports.web.updateReport.getReport', This patch could be made into a fixture, since we reuse it in three tests. Would be less code repetition this way. https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:87: mocker.patch('sitescripts.reports.web.updateReport.saveReport', If you want a noop function, you actually don't even need to provide the `new` argument. mocker.patch() would create a MagicMock object by default, that tries to do something reasonable in all possible scenarios. The MagicMock will also be returned from this call so you can inspect it at the end of the test to check that the mocked function has been called and check the invocation arguments. This way we get a more thorough test.
Hey Vasily! Jeez I thought I had already added those! Well, I did at some point, but must've lost that changeset. Thanks for catching that! Got a couple other comments/questions below. Thanks! :) Rosie https://codereview.adblockplus.org/29993614/diff/29996559/sitescripts/reports... File sitescripts/reports/web/updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/29996559/sitescripts/reports... sitescripts/reports/web/updateReport.py:30: return {'usefulness': 1} On 2019/02/04 17:48:50, Vasily Kuznetsov wrote: > On 2019/02/02 05:45:22, rhowell wrote: > > If I also return an email address here, we can test more of the code in > > handleRequest(), but then we'd also have to intercept updateUserUsefulness() > and > > sendUpdateNotification(). Wanted to check with you about how it looks so far > > before potentially going too far in the wrong direction. > > I think it would make more sense to mock getReport and saveReport (in the > testing code) instead of introducing all this testing-related logic here in > production code. Something like this: > > def test_xxx(mocker): > mocker.patch('sitescripts.reports.web.updateReport.getReport', > new=lambda *args: {'usefulness': 1}) > mocker.patch('sitescripts.reports.web.updateReport.saveReport', > new=test_saveReport) > # do our thing and the patching will be undone at the end of the test. > > See the documentation of pytest-mock (https://pypi.org/project/pytest-mock/) for > more info on how it works. Done. https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... File sitescripts/reports/tests/test_updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:24: sys.modules['MySQLdb'] = sys On 2019/02/05 17:12:08, Vasily Kuznetsov wrote: > We should probably add a comment here explaining why we need this line. What do > you think? Done. https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:73: mocker.patch('sitescripts.reports.web.updateReport.getReport', On 2019/02/05 17:12:08, Vasily Kuznetsov wrote: > This patch could be made into a fixture, since we reuse it in three tests. > Would be less code repetition this way. Each of the three calls to getReport return something different (usefulness, usefulness+email, and None). I guess I could make it a pytest fixture that takes an argument and returns a different report based on the argument, but that doesn't seem like it would save too much code duplication. Unless you think it would be better? https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:87: mocker.patch('sitescripts.reports.web.updateReport.saveReport', On 2019/02/05 17:12:08, Vasily Kuznetsov wrote: > If you want a noop function, you actually don't even need to provide the `new` > argument. mocker.patch() would create a MagicMock object by default, that tries > to do something reasonable in all possible scenarios. The MagicMock will also be > returned from this call so you can inspect it at the end of the test to check > that the mocked function has been called and check the invocation arguments. > This way we get a more thorough test. Ah, nice. Good idea. https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:98: new=lambda *args: None) For some reason, I get an error if I don't provide the `new=lambda *args: None` here. WSGIAppError: AttributeError("'module' object has no attribute 'connect'",) at ... /sitescripts/reports/utils.py:275
Hi Rosie! Looks good. I have basically only cosmetic comments. However, I'm wondering if it would make more sense to separate the news checks that you added into a separate patch. This way we'd have simpler history: "unify ...", "add checks ..." instead of everything being combined. Another option would be to keep the new checks in this patch and update the commit message. I'll leave it up to you which way to go. Cheers, Vasily https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... File sitescripts/reports/tests/test_updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:73: mocker.patch('sitescripts.reports.web.updateReport.getReport', On 2019/02/07 03:54:32, rhowell wrote: > On 2019/02/05 17:12:08, Vasily Kuznetsov wrote: > > This patch could be made into a fixture, since we reuse it in three tests. > > Would be less code repetition this way. > > Each of the three calls to getReport return something different (usefulness, > usefulness+email, and None). I guess I could make it a pytest fixture that takes > an argument and returns a different report based on the argument, but that > doesn't seem like it would save too much code duplication. Unless you think it > would be better? Yeah, you're right. Not worth it. https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:98: new=lambda *args: None) On 2019/02/07 03:54:33, rhowell wrote: > For some reason, I get an error if I don't provide the `new=lambda *args: None` > here. > > WSGIAppError: AttributeError("'module' object has no attribute 'connect'",) at > ... /sitescripts/reports/utils.py:275 I tested it a bit and it seems that when we provide no `new` value, pytest-mock inserts a MagicMock there and the call to it doesn't return None (it returns another MagicMock). Then our check for None doesn't work, execution continues and fails at some db-related call that we haven't mocked. I think the simplest way around this is to keep your original code as it is in this version. https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... File sitescripts/reports/tests/test_updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:33: UR_PATH = 'sitescripts.reports.web.updateReport.' Nice idea to make this a constant to avoid those super-long strings. https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:94: uu_mock = mocker.patch(UR_PATH + 'updateUserUsefulness') Maybe this should be uuu_mock to follow the same convention as others ;) https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... File sitescripts/reports/web/updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/web/updateReport.py:29: params = {name: data.get(name, '').strip() for name in ['name', 'email', These checks were not there before, right? If this is so, this would be new and not completely related functionality. Maybe it's better to put these changes into a separate change just to make things more clear. What do you think? https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/web/updateReport.py:30: 'subject', 'message', 'guid', 'secret', 'status', 'usefulness', What do you think about putting this list of fields into a constant at the start of the module? Would be a bit more organized and this line wouldn't have to be broken so much. https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/web/updateReport.py:44: guid_regex = r'^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$' This could also be a constant at the top of the module.
Hey Vasily, Sorry for the confusion with those checks! That was my mistake, see below. Let me know if you see anything else? Cheers, Rosie https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... File sitescripts/reports/tests/test_updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:73: mocker.patch('sitescripts.reports.web.updateReport.getReport', On 2019/02/07 20:19:15, Vasily Kuznetsov wrote: > On 2019/02/07 03:54:32, rhowell wrote: > > On 2019/02/05 17:12:08, Vasily Kuznetsov wrote: > > > This patch could be made into a fixture, since we reuse it in three tests. > > > Would be less code repetition this way. > > > > Each of the three calls to getReport return something different (usefulness, > > usefulness+email, and None). I guess I could make it a pytest fixture that > takes > > an argument and returns a different report based on the argument, but that > > doesn't seem like it would save too much code duplication. Unless you think it > > would be better? > > Yeah, you're right. Not worth it. Acknowledged. https://codereview.adblockplus.org/29993614/diff/29998590/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:98: new=lambda *args: None) On 2019/02/07 20:19:15, Vasily Kuznetsov wrote: > On 2019/02/07 03:54:33, rhowell wrote: > > For some reason, I get an error if I don't provide the `new=lambda *args: > None` > > here. > > > > WSGIAppError: AttributeError("'module' object has no attribute 'connect'",) at > > ... /sitescripts/reports/utils.py:275 > > I tested it a bit and it seems that when we provide no `new` value, pytest-mock > inserts a MagicMock there and the call to it doesn't return None (it returns > another MagicMock). Then our check for None doesn't work, execution continues > and fails at some db-related call that we haven't mocked. I think the simplest > way around this is to keep your original code as it is in this version. Done. https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... File sitescripts/reports/tests/test_updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:33: UR_PATH = 'sitescripts.reports.web.updateReport.' On 2019/02/07 20:19:16, Vasily Kuznetsov wrote: > Nice idea to make this a constant to avoid those super-long strings. Thanks. :) https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/tests/test_updateReport.py:94: uu_mock = mocker.patch(UR_PATH + 'updateUserUsefulness') On 2019/02/07 20:19:15, Vasily Kuznetsov wrote: > Maybe this should be uuu_mock to follow the same convention as others ;) Right! I had it like that at first, but this line was too long by one character, so I shortened it. But, after making the UR_PATH variable, guess I forgot to put it back to uuu_mock. Done. https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... File sitescripts/reports/web/updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/web/updateReport.py:29: params = {name: data.get(name, '').strip() for name in ['name', 'email', On 2019/02/07 20:19:16, Vasily Kuznetsov wrote: > These checks were not there before, right? If this is so, this would be new and > not completely related functionality. Maybe it's better to put these changes > into a separate change just to make things more clear. What do you think? Actually, it seems that most of these params are not mandatory anyway. (I think only `guid` and `secret` are mandatory.) I was looking at formmail.py and thinking it was updateReport.py. Oops, that's where my confusion came from. Sorry! https://codereview.adblockplus.org/5717434384252928/diff/5747976207073280/sit... https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/web/updateReport.py:30: 'subject', 'message', 'guid', 'secret', 'status', 'usefulness', On 2019/02/07 20:19:16, Vasily Kuznetsov wrote: > What do you think about putting this list of fields into a constant at the start > of the module? Would be a bit more organized and this line wouldn't have to be > broken so much. Turns out, we don't need this list for anything! https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/web/updateReport.py:44: guid_regex = r'^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$' On 2019/02/07 20:19:16, Vasily Kuznetsov wrote: > This could also be a constant at the top of the module. Done.
Hi Rosie! LGTM! So in the end the additional checks will not be in a separate review because they are not needed, right? Cheers, Vasily https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... File sitescripts/reports/web/updateReport.py (right): https://codereview.adblockplus.org/29993614/diff/30000601/sitescripts/reports... sitescripts/reports/web/updateReport.py:30: 'subject', 'message', 'guid', 'secret', 'status', 'usefulness', On 2019/02/08 01:34:52, rhowell wrote: > On 2019/02/07 20:19:16, Vasily Kuznetsov wrote: > > What do you think about putting this list of fields into a constant at the > start > > of the module? Would be a bit more organized and this line wouldn't have to be > > broken so much. > > Turns out, we don't need this list for anything! Acknowledged.
On 2019/02/08 19:06:07, Vasily Kuznetsov wrote: > Hi Rosie! > > LGTM! > > So in the end the additional checks will not be in a separate review because > they are not needed, right? Right. Looks like the only mandatory fields for `handleRequest()` are `guid` and `secret`, and there are already checks in place for those. I'll go ahead and push and close this ticket then. :) 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
