Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+412 lines, -130 lines) Patch
M .gitignore View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M .hgignore View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M cms/bin/test_server.py View 1 2 3 5 2 chunks +262 lines, -106 lines 0 comments Download
M tests/conftest.py View 1 2 3 1 chunk +13 lines, -0 lines 0 comments Download
A tests/test_dynamic_server.py View 1 2 3 1 chunk +63 lines, -0 lines 0 comments Download
M tests/test_page_outputs.py View 1 2 3 3 chunks +25 lines, -14 lines 0 comments Download
M tests/utils.py View 1 2 3 4 5 6 7 2 chunks +48 lines, -5 lines 0 comments Download
M tox.ini View 1 2 3 4 2 chunks +1 line, -2 lines 0 comments Download

Messages

Total messages: 23
Tudor Avram
Hi Vasily, After what seemed to be ages, I finally managed to (almost-fully) test the ...
Oct. 16, 2018, 11:47 a.m. (2018-10-16 11:47:54 UTC) #1
Vasily Kuznetsov
Hi Tudor, Some comments from the quick look through. Looking forward to objectificatio... err, I ...
Oct. 16, 2018, 1:18 p.m. (2018-10-16 13:18:24 UTC) #2
Tudor Avram
Hi Vasily, I addressed your comments and objectified the server handler. On top of that, ...
Oct. 18, 2018, 1:44 p.m. (2018-10-18 13:44:06 UTC) #3
Tudor Avram
Hi Vasily, I updated the exception testing (as mentioned in IRC). I also removed the ...
Oct. 18, 2018, 4:10 p.m. (2018-10-18 16:10:35 UTC) #4
Vasily Kuznetsov
Hi Tudor, I still have lots of comments but they are mostly about documentation and ...
Oct. 22, 2018, 2:36 p.m. (2018-10-22 14:36:48 UTC) #5
Tudor Avram
Hi Vasily, Thanks for your comments! I replied to a couple of them, let me ...
Oct. 22, 2018, 5:29 p.m. (2018-10-22 17:29:38 UTC) #6
Vasily Kuznetsov
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.py#newcode20 cms/bin/test_server.py:20: from ...
Oct. 23, 2018, 10:16 a.m. (2018-10-23 10:16:14 UTC) #7
Tudor Avram
Hi Vasily, I addressed your comments from the last patch. Let me know what you ...
Oct. 23, 2018, 4:44 p.m. (2018-10-23 16:44:36 UTC) #8
Tudor Avram
Hi Vasily, I addressed your comments from the last patch. Let me know what you ...
Oct. 23, 2018, 4:44 p.m. (2018-10-23 16:44:37 UTC) #9
Vasily Kuznetsov
LGTM One note: currently the tests for dynamic servers heisen-break on my computer. If I ...
Oct. 24, 2018, 1:31 p.m. (2018-10-24 13:31:53 UTC) #10
Tudor Avram
Hey, Thanks for this! I can try to add default waiting for a TCP port ...
Oct. 24, 2018, 2:30 p.m. (2018-10-24 14:30:08 UTC) #11
Tudor Avram
Hi Vasily, It's all done :D. Let me know if you have any further comments. ...
Oct. 24, 2018, 3:59 p.m. (2018-10-24 15:59:37 UTC) #12
Vasily Kuznetsov
On 2018/10/24 15:59:37, Tudor Avram wrote: > Hi Vasily, > > It's all done :D. ...
Oct. 25, 2018, 9:29 a.m. (2018-10-25 09:29:04 UTC) #13
Tudor Avram
Hi Vasily, Changed it now :D Sorry about the confusion. I set a hard limit ...
Oct. 25, 2018, 12:23 p.m. (2018-10-25 12:23:57 UTC) #14
Jon Sonesen
Hey guys, For some reason, this review literally just showed up for me on my ...
Oct. 25, 2018, 3:39 p.m. (2018-10-25 15:39:11 UTC) #15
Jon Sonesen
Hey guys, For some reason, this review literally just showed up for me on my ...
Oct. 25, 2018, 3:39 p.m. (2018-10-25 15:39:13 UTC) #16
Jon Sonesen
Hey guys, For some reason, this review literally just showed up for me on my ...
Oct. 25, 2018, 3:39 p.m. (2018-10-25 15:39:17 UTC) #17
Jon Sonesen
Overall though this looks great :) thanks tudor
Oct. 25, 2018, 3:42 p.m. (2018-10-25 15:42:15 UTC) #18
Vasily Kuznetsov
On 2018/10/25 15:39:17, Jon Sonesen wrote: > Hey guys, > > For some reason, this ...
Oct. 25, 2018, 3:50 p.m. (2018-10-25 15:50:13 UTC) #19
Vasily Kuznetsov
Hi Tudor, This looks good, but we can simplify the checking a bit (see below). ...
Oct. 25, 2018, 3:51 p.m. (2018-10-25 15:51:44 UTC) #20
Jon Sonesen
On 2018/10/25 15:50:13, Vasily Kuznetsov wrote: > On 2018/10/25 15:39:17, Jon Sonesen wrote: > > ...
Oct. 25, 2018, 4:01 p.m. (2018-10-25 16:01:37 UTC) #21
Tudor Avram
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 ...
Oct. 29, 2018, 10:42 a.m. (2018-10-29 10:42:52 UTC) #22
Vasily Kuznetsov
Oct. 29, 2018, 11:09 a.m. (2018-10-29 11:09:18 UTC) #23
LGTM

It seems that .gitignore and .hgignore changes are not necessary (and fail to
apply) but I'll correct this locally.

Powered by Google App Engine
This is Rietveld