Add python_utils to append to python path

Refactor test_server so some python related functions could be shared by other test servers. 
BUG=none
TEST=python_utils_unittest.cc

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=59671

[lzheng, 2010-09-14 06:38:38]

Please review. I can rebase http://codereview.chromium.org/3277008 with this CL
or we can get that CL in first and refactor the safe_browsing_browsertest.cc in
this cl.

[Paweł Hajdan Jr., 2010-09-14 17:17:07]

http://codereview.chromium.org/3366026/diff/43001/34003
File net/test/python_utils.cc (right):

http://codereview.chromium.org/3366026/diff/43001/34003#newcode38
net/test/python_utils.cc:38: bool GetPythonRunTime(FilePath* dir) {
It seems dangerous that we append to the directory. Shouldn't we rather
overwrite it?

http://codereview.chromium.org/3366026/diff/43001/34004
File net/test/python_utils.h (right):

http://codereview.chromium.org/3366026/diff/43001/34004#newcode11
net/test/python_utils.h:11: void AppendToPythonPath(const FilePath& dir);
Please comment both functions.

http://codereview.chromium.org/3366026/diff/43001/34004#newcode12
net/test/python_utils.h:12: bool GetPythonRunTime(FilePath* path);
nit: Add WARN_UNUSED_RESULT from base/compiler_specific.h.

http://codereview.chromium.org/3366026/diff/43001/34005
File net/test/python_utils_unittest.cc (right):

http://codereview.chromium.org/3366026/diff/43001/34005#newcode12
net/test/python_utils_unittest.cc:12: const char kPythonPath[] = "PYTHONPATH";
Should we declare this constant in python_util.h to avoid this duplication?

http://codereview.chromium.org/3366026/diff/43001/34005#newcode42
net/test/python_utils_unittest.cc:42: TEST(PythonUtils, PythonRunTime) {
I think this test is too specific. Well, we can try to launch Python with some
script from the command-line (like 'print "SUCCESS"').

[lzheng, 2010-09-14 17:52:55]

Pawel:

Thanks for the review. Please take another look.

http://codereview.chromium.org/3366026/diff/43001/34003
File net/test/python_utils.cc (right):

http://codereview.chromium.org/3366026/diff/43001/34003#newcode38
net/test/python_utils.cc:38: bool GetPythonRunTime(FilePath* dir) {
On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
> It seems dangerous that we append to the directory. Shouldn't we rather
> overwrite it?

Done.

http://codereview.chromium.org/3366026/diff/43001/34004
File net/test/python_utils.h (right):

http://codereview.chromium.org/3366026/diff/43001/34004#newcode11
net/test/python_utils.h:11: void AppendToPythonPath(const FilePath& dir);
On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
> Please comment both functions.

Done.

http://codereview.chromium.org/3366026/diff/43001/34004#newcode12
net/test/python_utils.h:12: bool GetPythonRunTime(FilePath* path);
On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
> nit: Add WARN_UNUSED_RESULT from base/compiler_specific.h.

Done.

http://codereview.chromium.org/3366026/diff/43001/34005
File net/test/python_utils_unittest.cc (right):

http://codereview.chromium.org/3366026/diff/43001/34005#newcode12
net/test/python_utils_unittest.cc:12: const char kPythonPath[] = "PYTHONPATH";
On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
> Should we declare this constant in python_util.h to avoid this duplication?

Done.

http://codereview.chromium.org/3366026/diff/43001/34005#newcode42
net/test/python_utils_unittest.cc:42: TEST(PythonUtils, PythonRunTime) {
On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
> I think this test is too specific. Well, we can try to launch Python with some
> script from the command-line (like 'print "SUCCESS"').
I think the goal of this unittest is to make sure the path are correctly set as
we want. I think we should leave the launch of python programs to system tests
(e.g.: browser tests) and leave unittest lite and fast.

[lzheng1, 2010-09-14 23:18:31]

FYI:

I updated the test_server to export its wait for server to start function.

On Tue, Sep 14, 2010 at 10:52 AM, <lzheng@chromium.org> wrote:

> Pawel:
>
> Thanks for the review. Please take another look.
>
>
>
> http://codereview.chromium.org/3366026/diff/43001/34003
> File net/test/python_utils.cc (right):
>
> http://codereview.chromium.org/3366026/diff/43001/34003#newcode38
> net/test/python_utils.cc:38: bool GetPythonRunTime(FilePath* dir) {
> On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
>
>> It seems dangerous that we append to the directory. Shouldn't we
>>
> rather
>
>> overwrite it?
>>
>
> Done.
>
>
> http://codereview.chromium.org/3366026/diff/43001/34004
> File net/test/python_utils.h (right):
>
> http://codereview.chromium.org/3366026/diff/43001/34004#newcode11
> net/test/python_utils.h:11: void AppendToPythonPath(const FilePath&
> dir);
> On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
>
>> Please comment both functions.
>>
>
> Done.
>
>
> http://codereview.chromium.org/3366026/diff/43001/34004#newcode12
> net/test/python_utils.h:12: bool GetPythonRunTime(FilePath* path);
> On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
>
>> nit: Add WARN_UNUSED_RESULT from base/compiler_specific.h.
>>
>
> Done.
>
>
> http://codereview.chromium.org/3366026/diff/43001/34005
> File net/test/python_utils_unittest.cc (right):
>
> http://codereview.chromium.org/3366026/diff/43001/34005#newcode12
> net/test/python_utils_unittest.cc:12: const char kPythonPath[] =
> "PYTHONPATH";
> On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
>
>> Should we declare this constant in python_util.h to avoid this
>>
> duplication?
>
> Done.
>
>
> http://codereview.chromium.org/3366026/diff/43001/34005#newcode42
> net/test/python_utils_unittest.cc:42: TEST(PythonUtils, PythonRunTime) {
> On 2010/09/14 17:17:07, Paweł Hajdan Jr. wrote:
>
>> I think this test is too specific. Well, we can try to launch Python
>>
> with some
>
>> script from the command-line (like 'print "SUCCESS"').
>>
> I think the goal of this unittest is to make sure the path are correctly
> set as we want. I think we should leave the launch of python programs to
> system tests (e.g.: browser tests) and leave unittest lite and fast.
>
>
> http://codereview.chromium.org/3366026/show
>

[Paweł Hajdan Jr., 2010-09-15 00:43:50]

http://codereview.chromium.org/3366026/diff/37004/59002
File net/test/python_utils.cc (right):

http://codereview.chromium.org/3366026/diff/37004/59002#newcode39
net/test/python_utils.cc:39: dir->clear();
nit: This is overcomplicated. Why not assign to dir instead of clearing and
appending?

http://codereview.chromium.org/3366026/diff/37004/59003
File net/test/python_utils.h (right):

http://codereview.chromium.org/3366026/diff/37004/59003#newcode9
net/test/python_utils.h:9: #include "base/compiler_specific.h"
nit: Add empty line below.

http://codereview.chromium.org/3366026/diff/37004/59003#newcode12
net/test/python_utils.h:12: extern const char kPythonPathEnv[];
nit: Add a comment and an empty line below.

http://codereview.chromium.org/3366026/diff/37004/59003#newcode16
net/test/python_utils.h:16: // Returns the path that could be used to launch
Python.
nit: could -> should, what do you think?

http://codereview.chromium.org/3366026/diff/37004/59006
File net/test/test_server.h (right):

http://codereview.chromium.org/3366026/diff/37004/59006#newcode75
net/test/test_server.h:75: static bool WaitServerStart(const std::string& host,
uint16 port,
I don't think it's the right way to share this code. Can we extract some
BaseTestServer or something like that? WaitForServerStart is an awkward name,
and should not be public. Also, its interface exposes implementation details,
and will not be sufficient to apply David Benjamin's patch to make the wait more
reliable.

[lzheng1, 2010-09-15 04:55:29]

On Tue, Sep 14, 2010 at 5:43 PM, <phajdan.jr@chromium.org> wrote:

>
> http://codereview.chromium.org/3366026/diff/37004/59002
>
> File net/test/python_utils.cc (right):
>
> http://codereview.chromium.org/3366026/diff/37004/59002#newcode39
> net/test/python_utils.cc:39: dir->clear();
> nit: This is overcomplicated. Why not assign to dir instead of clearing
> and appending?
>
> http://codereview.chromium.org/3366026/diff/37004/59003
>
> File net/test/python_utils.h (right):
>
> http://codereview.chromium.org/3366026/diff/37004/59003#newcode9
> net/test/python_utils.h:9: #include "base/compiler_specific.h"
> nit: Add empty line below.
>
> http://codereview.chromium.org/3366026/diff/37004/59003#newcode12
> net/test/python_utils.h:12: extern const char kPythonPathEnv[];
> nit: Add a comment and an empty line below.
>
> http://codereview.chromium.org/3366026/diff/37004/59003#newcode16
> net/test/python_utils.h:16: // Returns the path that could be used to
> launch Python.
> nit: could -> should, what do you think?
>
> http://codereview.chromium.org/3366026/diff/37004/59006
> File net/test/test_server.h (right):
>
> http://codereview.chromium.org/3366026/diff/37004/59006#newcode75
> net/test/test_server.h:75: static bool WaitServerStart(const
> std::string& host, uint16 port,
> I don't think it's the right way to share this code. Can we extract some
> BaseTestServer or something like that? WaitForServerStart is an awkward
> name, and should not be public. Also, its interface exposes
> implementation details, and will not be sufficient to apply David
> Benjamin's patch to make the wait more reliable.

As I mentioned in SafeBrowsing CL, I don't like this either. Where is Dave
B's patch? Can you forward me a link and let me take a look?

The base class approach is a bit overshoot since you could consider that the
"server" here is just a python application that we could launch use
LaunchApp. The only difference is that we need a Pinger to ping the port to
make sure the App is running. In that sense, the LaunchApp and Pinger are
what we want. The rest is how we set up the path to call the app and config
the cmdline (which python_utils.h is helping).

I was thinking to create another util file that calls the pinger to ping the
port, but I feel that is a bit overshoot too given the common code is just
to call the Pinger. For now, it might be better to keep it as is in my
Safebrowsing test and revisit after Dave's change if needed (Maybe I can
just call something from his cl).  I am open for better suggestions though.


>
> http://codereview.chromium.org/3366026/show
>

[Paweł Hajdan Jr., 2010-09-15 17:38:22]

David's CL is http://codereview.chromium.org/3368012/show, and TCPPinger is
going to be removed (and I want it to be removed because it induces flakiness).

[lzheng1, 2010-09-15 18:58:24]

First, I removed the WaitForServer start change. So, we only have the python
related change here now. Can you take another look? I think at least we
could get that part in and clean the SafeBrowsingTest CL.

Dave's code changes both the test server and test itself to make sure it
works. It will be hard for me to change the test suite server because it is
"third party" code and require more code and effort. What kind of flakiness
pinger introduces? Can we use it for other purpose? I think we need a way to
tell if a host/port pair is listening and ready to accept requests.


Lei
On Wed, Sep 15, 2010 at 10:38 AM, <phajdan.jr@chromium.org> wrote:

> David's CL is http://codereview.chromium.org/3368012/show, and TCPPinger
> is
> going to be removed (and I want it to be removed because it induces
> flakiness).
>
>
> http://codereview.chromium.org/3366026/show
>

[Paweł Hajdan Jr., 2010-09-15 20:15:53]

http://codereview.chromium.org/3366026/diff/18002/60003
File net/test/python_utils.cc (right):

http://codereview.chromium.org/3366026/diff/18002/60003#newcode39
net/test/python_utils.cc:39: dir->clear();
We should just assign below, please see my earlier comments.

http://codereview.chromium.org/3366026/diff/18002/60004
File net/test/python_utils.h (right):

http://codereview.chromium.org/3366026/diff/18002/60004#newcode10
net/test/python_utils.h:10: class FilePath;
nit: Add empty line above.

http://codereview.chromium.org/3366026/diff/18002/60004#newcode13
net/test/python_utils.h:13: // Appends the dir to python path environment
variable.
nit: Add empty line above.

http://codereview.chromium.org/3366026/diff/18002/60006
File net/test/test_server.cc (right):

http://codereview.chromium.org/3366026/diff/18002/60006#newcode23
net/test/test_server.cc:23: #include "net/test/python_utils.h"
nit: Preserve order of includes (alphabetical).

[lzheng, 2010-09-15 21:16:04]

Done. Another look?

Lei

http://codereview.chromium.org/3366026/diff/37004/59002
File net/test/python_utils.cc (right):

http://codereview.chromium.org/3366026/diff/37004/59002#newcode39
net/test/python_utils.cc:39: dir->clear();
On 2010/09/15 00:43:50, Paweł Hajdan Jr. wrote:
> nit: This is overcomplicated. Why not assign to dir instead of clearing and
> appending?

Done.

http://codereview.chromium.org/3366026/diff/37004/59003
File net/test/python_utils.h (right):

http://codereview.chromium.org/3366026/diff/37004/59003#newcode9
net/test/python_utils.h:9: #include "base/compiler_specific.h"
On 2010/09/15 00:43:50, Paweł Hajdan Jr. wrote:
> nit: Add empty line below.

Done.

http://codereview.chromium.org/3366026/diff/37004/59003#newcode12
net/test/python_utils.h:12: extern const char kPythonPathEnv[];
On 2010/09/15 00:43:50, Paweł Hajdan Jr. wrote:
> nit: Add a comment and an empty line below.

Done.

http://codereview.chromium.org/3366026/diff/37004/59003#newcode16
net/test/python_utils.h:16: // Returns the path that could be used to launch
Python.
On 2010/09/15 00:43:50, Paweł Hajdan Jr. wrote:
> nit: could -> should, what do you think?

Done.

http://codereview.chromium.org/3366026/diff/37004/59006
File net/test/test_server.h (right):

http://codereview.chromium.org/3366026/diff/37004/59006#newcode75
net/test/test_server.h:75: static bool WaitServerStart(const std::string& host,
uint16 port,
On 2010/09/15 00:43:50, Paweł Hajdan Jr. wrote:
> I don't think it's the right way to share this code. Can we extract some
> BaseTestServer or something like that? WaitForServerStart is an awkward name,
> and should not be public. Also, its interface exposes implementation details,
> and will not be sufficient to apply David Benjamin's patch to make the wait
more
> reliable.
Removed.

http://codereview.chromium.org/3366026/diff/18002/60003
File net/test/python_utils.cc (right):

http://codereview.chromium.org/3366026/diff/18002/60003#newcode39
net/test/python_utils.cc:39: dir->clear();
On 2010/09/15 20:15:53, Paweł Hajdan Jr. wrote:
> We should just assign below, please see my earlier comments.
Woops, sorry, I missed your early comments.
Done.

http://codereview.chromium.org/3366026/diff/18002/60004
File net/test/python_utils.h (right):

http://codereview.chromium.org/3366026/diff/18002/60004#newcode10
net/test/python_utils.h:10: class FilePath;
On 2010/09/15 20:15:53, Paweł Hajdan Jr. wrote:
> nit: Add empty line above.

Done.

http://codereview.chromium.org/3366026/diff/18002/60004#newcode13
net/test/python_utils.h:13: // Appends the dir to python path environment
variable.
On 2010/09/15 20:15:53, Paweł Hajdan Jr. wrote:
> nit: Add empty line above.

Done.

http://codereview.chromium.org/3366026/diff/18002/60006
File net/test/test_server.cc (right):

http://codereview.chromium.org/3366026/diff/18002/60006#newcode23
net/test/test_server.cc:23: #include "net/test/python_utils.h"
On 2010/09/15 20:15:53, Paweł Hajdan Jr. wrote:
> nit: Preserve order of includes (alphabetical).

Done.

[Paweł Hajdan Jr., 2010-09-15 22:02:58]

lg with a nit

http://codereview.chromium.org/3366026/diff/60010/66004
File net/test/python_utils.h (right):

http://codereview.chromium.org/3366026/diff/60010/66004#newcode13
net/test/python_utils.h:13: // This is the python path avariable name.
nit: typo (should be variable)

[lzheng, 2010-09-15 22:36:50]

Done.
Submitting ...

On 2010/09/15 22:02:58, Paweł Hajdan Jr. wrote:
> lg with a nit
> 
> http://codereview.chromium.org/3366026/diff/60010/66004
> File net/test/python_utils.h (right):
> 
> http://codereview.chromium.org/3366026/diff/60010/66004#newcode13
> net/test/python_utils.h:13: // This is the python path avariable name.
> nit: typo (should be variable)