Merge lp:~vila/bzr/1451448-skip-racy-tests into lp:bzr

Proposed by Vincent Ladeuil on 2015-09-06
Status: Merged
Approved by: Richard Wilbur on 2015-09-07
Approved revision: 6606
Merged at revision: 6604
Proposed branch: lp:~vila/bzr/1451448-skip-racy-tests
Merge into: lp:bzr
Diff against target: 137 lines (+35/-7)
5 files modified
bzrlib/tests/test_http.py (+18/-3)
bzrlib/tests/test_osutils.py (+3/-1)
bzrlib/tests/test_transport.py (+2/-1)
bzrlib/tests/test_urlutils.py (+3/-2)
doc/en/release-notes/bzr-2.7.txt (+9/-0)
To merge this branch: bzr merge lp:~vila/bzr/1451448-skip-racy-tests
Reviewer Review Type Date Requested Status
Richard Wilbur 2015-09-06 Approve on 2015-09-07
Review via email: mp+270266@code.launchpad.net

Commit message

Fix test failures blocking package builds.

Description of the change

This MP fixes the failures encountered since python 2.7.9 (see bug).

The http tests are the most trivial http tests so I'm confident the code is ok and that a *new* race is exposed. Moreover it's exposed only when the http client and server are running in the same thread.

I also skip the windows tests are this needs to be fixed and tested on windows (and I don't have access to a windows host) and that shouldn't block other platforms.

To post a comment you must log in.
lp:~vila/bzr/1451448-skip-racy-tests updated on 2015-09-06
6606. By Vincent Ladeuil on 2015-09-06

Fix news entries order.

Richard Wilbur (richard-wilbur) wrote :

Thanks for the fixes, Vincent. Both situations seem suspect: running the http client and server in the same thread seems prone to races, running windows filesystem-specific tests on a non-windows filesystem sounds like it would be likely to fail.
+1

review: Approve
Vincent Ladeuil (vila) wrote :

Thanks for the review.

> Thanks for the fixes, Vincent. Both situations seem suspect:

> running the http client and server in the same thread seems prone to races,

Historically, all client/server tests has been written this way since day one. It took years to run into the races and realize the root cause was that the python library was not intended to support both the client and the server in this context.

So, after the fact, it seems obvious that this was a bad design decision but the thing is: when everything works and once in a blue moon a test fails, nobody suspects that design... ;)

> running
> windows filesystem-specific tests on a non-windows filesystem sounds like it
> would be likely to fail.

Well, history again, it was easier at one point to be able to run those tests on ubuntu to make sure the windows support wasn't regressing until tests can be run there. Nowadays... it's more complicated.

Hope this shed some light on the past ;)

Richard Wilbur (richard-wilbur) wrote :

On Mon, Sep 7, 2015 at 2:32 AM, Vincent Ladeuil <email address hidden> wrote:
> Thanks for the review.

I'm down to one machine that boots off a CD (won't boot from HD any
more :(, so I get on to check my E-mail but haven't bothered trying to
set up hydrazine, etc. Thus I can review and Approve, but not merge
till I finish fixing at least one of these machines.

[...]
> Hope this shed some light on the past ;)

I quite appreciate the history lesson. Thank you!

Can you send this to PQM? If not, let me know and it will be a good
excuse to concentrate on fixing at least one of my computers.

lp:~vila/bzr/1451448-skip-racy-tests updated on 2015-09-08
6607. By Vincent Ladeuil on 2015-09-08

Skip the missed test in test_transport.

Vincent Ladeuil (vila) wrote :

> On Mon, Sep 7, 2015 at 2:32 AM, Vincent Ladeuil <email address hidden> wrote:
> > Thanks for the review.
>
> I'm down to one machine that boots off a CD (won't boot from HD any
> more :(,

:-/

> so I get on to check my E-mail but haven't bothered trying to
> set up hydrazine, etc. Thus I can review and Approve, but not merge
> till I finish fixing at least one of these machines.

ack.

>
> [...]
> > Hope this shed some light on the past ;)
>
> I quite appreciate the history lesson. Thank you!
>
> Can you send this to PQM? If not, let me know and it will be a good
> excuse to concentrate on fixing at least one of my computers.

Still waiting for PQM to resurrect (the related ticket priority has been bumped so crossed-fingers-hoping the fix will happen rsn).

Vincent Ladeuil (vila) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/test_http.py'
2--- bzrlib/tests/test_http.py 2012-09-17 06:58:15 +0000
3+++ bzrlib/tests/test_http.py 2015-09-08 08:00:02 +0000
4@@ -1,4 +1,4 @@
5-# Copyright (C) 2005-2011 Canonical Ltd
6+# Copyright (C) 2005-2012, 2015 Canonical Ltd
7 #
8 # This program is free software; you can redistribute it and/or modify
9 # it under the terms of the GNU General Public License as published by
10@@ -656,13 +656,28 @@
11
12 _req_handler_class = BadStatusRequestHandler
13
14+ def setUp(self):
15+ super(TestBadStatusServer, self).setUp()
16+ # See https://bugs.launchpad.net/bzr/+bug/1451448 for details.
17+ # TD;LR: Running both a TCP client and server in the same process and
18+ # thread uncovers a race in python. The fix is to run the server in a
19+ # different process. Trying to fix yet another race here is not worth
20+ # the effort. -- vila 2015-09-06
21+ if 'HTTP/1.0' in self.id():
22+ raise tests.TestSkipped(
23+ 'Client/Server in the same process and thread can hang')
24+
25 def test_http_has(self):
26 t = self.get_readonly_transport()
27- self.assertRaises(errors.InvalidHttpResponse, t.has, 'foo/bar')
28+ self.assertRaises((errors.ConnectionError, errors.ConnectionReset,
29+ errors.InvalidHttpResponse),
30+ t.has, 'foo/bar')
31
32 def test_http_get(self):
33 t = self.get_readonly_transport()
34- self.assertRaises(errors.InvalidHttpResponse, t.get, 'foo/bar')
35+ self.assertRaises((errors.ConnectionError, errors.ConnectionReset,
36+ errors.InvalidHttpResponse),
37+ t.get, 'foo/bar')
38
39
40 class InvalidStatusRequestHandler(http_server.TestingHTTPRequestHandler):
41
42=== modified file 'bzrlib/tests/test_osutils.py'
43--- bzrlib/tests/test_osutils.py 2014-04-09 08:05:23 +0000
44+++ bzrlib/tests/test_osutils.py 2015-09-08 08:00:02 +0000
45@@ -1,4 +1,4 @@
46-# Copyright (C) 2005-2011 Canonical Ltd
47+# Copyright (C) 2005-2015 Canonical Ltd
48 #
49 # This program is free software; you can redistribute it and/or modify
50 # it under the terms of the GNU General Public License as published by
51@@ -925,6 +925,7 @@
52 """Test that _win32 versions of os utilities return appropriate paths."""
53
54 def test_abspath(self):
55+ self.requireFeature(features.win32_feature)
56 self.assertEqual('C:/foo', osutils._win32_abspath('C:\\foo'))
57 self.assertEqual('C:/foo', osutils._win32_abspath('C:/foo'))
58 self.assertEqual('//HOST/path', osutils._win32_abspath(r'\\HOST\path'))
59@@ -977,6 +978,7 @@
60 self.assertEqual('C:\\foo', osutils._win32_fixdrive('c:\\foo'))
61
62 def test_win98_abspath(self):
63+ self.requireFeature(features.win32_feature)
64 # absolute path
65 self.assertEqual('C:/foo', osutils._win98_abspath('C:\\foo'))
66 self.assertEqual('C:/foo', osutils._win98_abspath('C:/foo'))
67
68=== modified file 'bzrlib/tests/test_transport.py'
69--- bzrlib/tests/test_transport.py 2011-12-14 19:44:40 +0000
70+++ bzrlib/tests/test_transport.py 2015-09-08 08:00:02 +0000
71@@ -1,4 +1,4 @@
72-# Copyright (C) 2005-2011 Canonical Ltd
73+# Copyright (C) 2005-2011, 2015 Canonical Ltd
74 #
75 # This program is free software; you can redistribute it and/or modify
76 # it under the terms of the GNU General Public License as published by
77@@ -815,6 +815,7 @@
78 class TestWin32LocalTransport(tests.TestCase):
79
80 def test_unc_clone_to_root(self):
81+ self.requireFeature(features.win32_feature)
82 # Win32 UNC path like \\HOST\path
83 # clone to root should stop at least at \\HOST part
84 # not on \\
85
86=== modified file 'bzrlib/tests/test_urlutils.py'
87--- bzrlib/tests/test_urlutils.py 2012-01-05 13:02:31 +0000
88+++ bzrlib/tests/test_urlutils.py 2015-09-08 08:00:02 +0000
89@@ -1,4 +1,4 @@
90-# Copyright (C) 2006-2012 Canonical Ltd
91+# Copyright (C) 2006-2012, 2015 Canonical Ltd
92 #
93 # This program is free software; you can redistribute it and/or modify
94 # it under the terms of the GNU General Public License as published by
95@@ -26,7 +26,7 @@
96 InvalidRebaseURLs,
97 PathNotChild,
98 )
99-from bzrlib.tests import TestCaseInTempDir, TestCase, TestSkipped
100+from bzrlib.tests import features, TestCaseInTempDir, TestCase, TestSkipped
101
102
103 class TestUrlToPath(TestCase):
104@@ -411,6 +411,7 @@
105 self.assertFalse(isinstance(result, unicode))
106
107 def test_win32_unc_path_to_url(self):
108+ self.requireFeature(features.win32_feature)
109 to_url = urlutils._win32_local_path_to_url
110 self.assertEqual('file://HOST/path',
111 to_url(r'\\HOST\path'))
112
113=== modified file 'doc/en/release-notes/bzr-2.7.txt'
114--- doc/en/release-notes/bzr-2.7.txt 2014-12-15 20:24:42 +0000
115+++ doc/en/release-notes/bzr-2.7.txt 2015-09-08 08:00:02 +0000
116@@ -76,6 +76,11 @@
117 suite. This can include new facilities for writing tests, fixes to
118 spurious test failures and changes to the way things should be tested.
119
120+* Fix racy http tests (TestBadStatusServer is so simple, it exposes a race
121+ in python 2.7.9. This happens only when both the http server and client
122+ are run in the same process.). Only tests are affected.
123+ (Vincent Ladeuil, #1451448)
124+
125 * Fix warnings on stderr caused by the atexit handler triggering for the
126 wrong reason: the 'config' command should explicitly save the changes when
127 modifying or removing an option and not rely on the atexit
128@@ -93,5 +98,9 @@
129 * Restrict access to '.netrc' in tests or recent python (2.7.5-8) will
130 complain. (Vincent Ladeuil, #1233413)
131
132+* Skip windows-only tests that start failing with python 2.7.9, there is no
133+ way to fix them without testing on windows itself.
134+ (Vincent Ladeuil, #1451448)
135+
136 ..
137 vim: tw=74 ft=rst ff=unix