Correctly kill 'git daemon' child process, fixing a lot of testing issues.

Correctly kill 'git daemon' child process, fixing a lot of testing issues.

Add code to wait for the bound port to open and close correctly, removing race
conditions.

BUG=test reliability
TEST=better

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

[M-A Ruel, 2011-03-04 18:08:16]

I have more changes following to improve reliability

[Evan Martin, 2011-03-04 18:13:13]

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py
File tests/fake_repos.py (right):

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode38
tests/fake_repos.py:38: def addKill():
The capitalization style of this doesn't match the function above, nor do either
match the one below.  Please pick one style for the file.  :)

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode53
tests/fake_repos.py:53: pass
Maybe split this out into a function like kill_pid?

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode267
tests/fake_repos.py:267: self.gitpidfile = None
git_pid_file

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode315
tests/fake_repos.py:315: def tearDownSVN(self):
Also weird capitalization here.

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode330
tests/fake_repos.py:330: def tearDownGIT(self):
"Git" or "git", but never "GIT"

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode447
tests/fake_repos.py:447: logging.fatal('%d shouldn\'t be open' % port)
"open" is a confusing word here -- it means not-free, yeah?

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode466
tests/fake_repos.py:466: sock.close()
Should this have a timeout int it?

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode473
tests/fake_repos.py:473: logging.debug('%d was open, waiting to close' % port)
Is the wait done by the sock.close()?

[M-A Ruel, 2011-03-04 18:53:30]

Updated.

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py
File tests/fake_repos.py (right):

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode38
tests/fake_repos.py:38: def addKill():
On 2011/03/04 18:13:13, Evan Martin wrote:
> The capitalization style of this doesn't match the function above, nor do
either
> match the one below.  Please pick one style for the file.  :)

I want to convert to PEP8 but I do that incrementally to keep the diff readable.
The reason tearDownXX() style is used is because of unittest member functions.

I've changed many more function names.

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode53
tests/fake_repos.py:53: pass
On 2011/03/04 18:13:13, Evan Martin wrote:
> Maybe split this out into a function like kill_pid?

done

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode447
tests/fake_repos.py:447: logging.fatal('%d shouldn\'t be open' % port)
On 2011/03/04 18:13:13, Evan Martin wrote:
> "open" is a confusing word here -- it means not-free, yeah?

Renamed.

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode466
tests/fake_repos.py:466: sock.close()
On 2011/03/04 18:13:13, Evan Martin wrote:
> Should this have a timeout int it?

Added a clamp at 30 seconds.

http://codereview.chromium.org/6627013/diff/1/tests/fake_repos.py#newcode473
tests/fake_repos.py:473: logging.debug('%d was open, waiting to close' % port)
On 2011/03/04 18:13:13, Evan Martin wrote:
> Is the wait done by the sock.close()?

There's no wait done, it loops as fast as it can, which is not that fast. Even
for svnserve, which is rather slow to start, it does 20 loops max.

[Evan Martin, 2011-03-04 18:57:08]

LGTM (not sure if you wanted dpranke to look, sorry for hijacking the review)

[commit-bot: I haz the power, 2011-03-04 18:58:43]

Can't apply patch for file tests/gclient_smoketest.py.
patching file tests/gclient_smoketest.py
Hunk #1 FAILED at 257.
Hunk #2 FAILED at 690.
Hunk #3 FAILED at 907.
Hunk #4 FAILED at 1070.
4 out of 4 hunks FAILED -- saving rejects to file tests/gclient_smoketest.py.rej

[M-A Ruel, 2011-03-04 19:00:14]

On 2011/03/04 18:58:43, commit-bot wrote:
> Can't apply patch for file tests/gclient_smoketest.py.
> patching file tests/gclient_smoketest.py
> Hunk #1 FAILED at 257.
> Hunk #2 FAILED at 690.
> Hunk #3 FAILED at 907.
> Hunk #4 FAILED at 1070.
> 4 out of 4 hunks FAILED -- saving rejects to file
tests/gclient_smoketest.py.rej

Ah! The patch is based against http://codereview.chromium.org/6621013 and the
commit-queue won't let me go away with it. It wasn't a problem initially since I
wasn't touching gclient_smoketest.py.

[Dirk Pranke, 2011-03-04 21:15:16]

LGTM, with the minor nits noted.

http://codereview.chromium.org/6627013/diff/4002/tests/fake_repos.py
File tests/fake_repos.py (right):

http://codereview.chromium.org/6627013/diff/4002/tests/fake_repos.py#newcode66
tests/fake_repos.py:66: return kill_pid(process.pid)
I'd likely just change kill_pid to take process as an argument for consistency
w/ kill_win()

http://codereview.chromium.org/6627013/diff/4002/tests/fake_repos.py#newcode466
tests/fake_repos.py:466: start = datetime.datetime.utcnow()
Curious why you're using datetime instead of just time?

[commit-bot: I haz the power, 2011-03-04 21:16:13]

Change committed as 76966

[M-A Ruel, 2011-03-04 21:18:21]

http://codereview.chromium.org/6627013/diff/4002/tests/fake_repos.py
File tests/fake_repos.py (right):

http://codereview.chromium.org/6627013/diff/4002/tests/fake_repos.py#newcode66
tests/fake_repos.py:66: return kill_pid(process.pid)
On 2011/03/04 21:15:17, dpranke wrote:
> I'd likely just change kill_pid to take process as an argument for consistency
> w/ kill_win()

I can't because I need to kill 'git-daemon' child process by pid.

http://codereview.chromium.org/6627013/diff/4002/tests/fake_repos.py#newcode466
tests/fake_repos.py:466: start = datetime.datetime.utcnow()
On 2011/03/04 21:15:17, dpranke wrote:
> Curious why you're using datetime instead of just time?

Habit, no real reason.

[Dirk Pranke, 2011-03-04 21:27:04]

On Fri, Mar 4, 2011 at 1:18 PM,  <maruel@chromium.org> wrote:
>
> http://codereview.chromium.org/6627013/diff/4002/tests/fake_repos.py
> File tests/fake_repos.py (right):
>
> http://codereview.chromium.org/6627013/diff/4002/tests/fake_repos.py#newcode66
> tests/fake_repos.py:66: return kill_pid(process.pid)
> On 2011/03/04 21:15:17, dpranke wrote:
>>
>> I'd likely just change kill_pid to take process as an argument for
>
> consistency
>>
>> w/ kill_win()
>
> I can't because I need to kill 'git-daemon' child process by pid.
>

Ah, right, sorry.

-- Dirk