To reproduce: 1. test-webkitpy webkitpy.layout_tests.run_webkit_tests_unittest.MainTest.test_basic On Windows XP with Cygwin 1.5 and Python 2.5.2, this wedges Python. It may also happen in other configurations. It is known not to happen with Windows 7/Cygwin 1.7/Python 2.6.5.
In the meantime we should skip this test when this configuration is detected.
That's not a single test, it's a whole class of tests. AFAIK, there isn't a way to disable a whole class is there? Note that I do agree with you though; we should be skipping these tests if they're hanging.
(In reply to comment #2) > That's not a single test, it's a whole class of tests. AFAIK, there isn't a way to disable a whole class is there? There is in Python 3.1+ via a decorator. We can emulate it in older versions.
Created attachment 72555 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x
Comment on attachment 72555 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x View in context: https://bugs.webkit.org/attachment.cgi?id=72555&action=review > WebKitTools/Scripts/webkitpy/test/skip.py:39 > +def skip_if(klass, condition, message=None): > + if not condition: > + return klass > + for name in dir(klass): > + attr = getattr(klass, name) > + if not callable(attr): > + continue > + if not name.startswith('test_'): > + continue > + setattr(klass, name, _skipped_method(attr)) > + klass._printed_skipped_message = False > + return klass I'm not sure of the best way to test skip_if. Any suggestions?
Comment on attachment 72555 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x View in context: https://bugs.webkit.org/attachment.cgi?id=72555&action=review change looks reasonable to me. I assume this is backported from code in 3.2 or other parts of unittest? Sadly, I'm not a reviewer so you'll still need someone else to r+ this. >> WebKitTools/Scripts/webkitpy/test/skip.py:39 >> + return klass > > I'm not sure of the best way to test skip_if. Any suggestions? I would create a dummy class, e.g.: TestSkipFixture(object): def test_foo(self): pass and then call skip_if(TestSkipFixture, True), then call test_foo() and use outputcapture to ensure the messages get logged. And vice-versa for skip_if(TestSkipFixture, False).
(In reply to comment #6) > (From update of attachment 72555 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72555&action=review > > change looks reasonable to me. I assume this is backported from code in 3.2 or other parts of unittest? Yes, backported from 3.1+. > >> WebKitTools/Scripts/webkitpy/test/skip.py:39 > >> + return klass > > > > I'm not sure of the best way to test skip_if. Any suggestions? > > I would create a dummy class, e.g.: > > TestSkipFixture(object): > def test_foo(self): > pass > > and then call skip_if(TestSkipFixture, True), then call test_foo() and use outputcapture to ensure the messages get logged. And vice-versa for skip_if(TestSkipFixture, False). Thanks for the suggestion. Unfortunately outputcapture doesn't seem to be capturing the output from skip_if's logger. Any ideas why that might be happening? Maybe the logger buffers its input?
(In reply to comment #7) > > Thanks for the suggestion. Unfortunately outputcapture doesn't seem to be capturing the output from skip_if's logger. Any ideas why that might be happening? Maybe the logger buffers its input? The loggers don't buffer any input, so that's not going to be it. It could be a couple of things. You didn't say in what environment it wasn't working, or how you know that outputcapture isn't capturing it. So, I'll guess that the regular stderr logger isn't configured to print messages at log.info. You might need to do something like: import logging class Test(unittest.TestCase) def setUp(): self.logger = logging.getLogger() self.old_level = self.logger.level self.logger.setLevel(logging.INFO) def tearDown(): self.logger.setLevel(self._old_level) def test_skip_if(self): ... If that doesn't work, lmk or ping me on IRC and upload whatever patch you have so I can take a look and see if I can reproduce it.
Created attachment 72665 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x
<rdar://problem/8620361>
Created attachment 72671 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x
Comment on attachment 72671 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x Change basically looks good to me. Can you add a comment to skip_if noting that this class is backported from 3.1+ ? Also, I've discussed this w/ eseidel, but we should add the scaffolding to outputcapture to do install and remove logging handlers so that you don't have to do the stuff you did in setUp() and tearDown().
Comment on attachment 72671 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x Cool.
(In reply to comment #12) > (From update of attachment 72671 [details]) > Change basically looks good to me. Can you add a comment to skip_if noting that this class is backported from 3.1+ ? Done. > Also, I've discussed this w/ eseidel, but we should add the scaffolding to outputcapture to do install and remove logging handlers so that you don't have to do the stuff you did in setUp() and tearDown(). That would be great!
Committed r71146: <http://trac.webkit.org/changeset/71146>
Leaving this bug open to cover unwedging the tests on this platform.
Comment on attachment 72671 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x View in context: https://bugs.webkit.org/attachment.cgi?id=72671&action=review abarth let you off easy. :) > WebKitTools/ChangeLog:18 > + unittest.skip_if decorator added in Python 3.1. false. it's skipIf and added in 2.7: http://docs.python.org/library/unittest.html#unittest.skipIf > WebKitTools/Scripts/webkitpy/test/skip.py:28 > +def skip_if(klass, condition, message=None, logger=None): We should note that this exists in 2.7: http://docs.python.org/library/unittest.html#unittest.skipIf Why does your decorator look so different from the one I wrote for memoized? Is this a "new" style? I guess this is a class decorator not a function decorator? > WebKitTools/Scripts/webkitpy/test/skip_unittest.py:41 > + > + self.old_level = self.logger.level > + self.logger.setLevel(logging.INFO) > + > + self.old_propagate = self.logger.propagate > + self.logger.propagate = False > + > + self.log_stream = StringIO.StringIO() > + self.handler = logging.StreamHandler(self.log_stream) So this is using "INFO" because currently INFO isnt' logged? Maybe this integrates with dpranke's attempt to make OutputCapture understand python logging.py? OutputCapture is our current way of grabbing output during a unit test and should be taught how to grab logging.py output too, by teaching it about the wekbit-patch logging handler (currently > INFO) and how to disable/remove it while an OutputCapture is in place. > WebKitTools/Scripts/webkitpy/test/skip_unittest.py:59 > + def create_fixture_class(self): > + class TestSkipFixture(object): > + def __init__(self, callback): > + self.callback = callback > + > + def test_foo(self): > + self.callback() > + > + return TestSkipFixture I'm confused why not just declare this at the top of the file? Creating it dynamically in this method doesn't make much sense to me? > WebKitTools/Scripts/webkitpy/test/skip_unittest.py:65 > + klass = skip_if(self.create_fixture_class(), False, 'Should not see this message.', logger=self.logger) Would have been nice to see an example of it using the decorator syntax. > WebKitTools/Scripts/webkitpy/test/skip_unittest.py:75 > + two spaces.
Comment on attachment 72671 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x View in context: https://bugs.webkit.org/attachment.cgi?id=72671&action=review Thanks for taking a look! >> WebKitTools/ChangeLog:18 >> + unittest.skip_if decorator added in Python 3.1. > > false. it's skipIf and added in 2.7: http://docs.python.org/library/unittest.html#unittest.skipIf OK, I got confused by the "New in version 3.1" from <http://docs.python.org/dev/library/unittest.html#skipping-tests-and-expected-failures>. That doesn't explain the typo, though. >> WebKitTools/Scripts/webkitpy/test/skip.py:28 >> +def skip_if(klass, condition, message=None, logger=None): > > We should note that this exists in 2.7: http://docs.python.org/library/unittest.html#unittest.skipIf > > Why does your decorator look so different from the one I wrote for memoized? Is this a "new" style? > > I guess this is a class decorator not a function decorator? I added a docstring in the patch I checked in based on Dirk's suggestion. I assume the decorator looks different because it is a class decorator. >> WebKitTools/Scripts/webkitpy/test/skip_unittest.py:41 >> + self.handler = logging.StreamHandler(self.log_stream) > > So this is using "INFO" because currently INFO isnt' logged? Maybe this integrates with dpranke's attempt to make OutputCapture understand python logging.py? OutputCapture is our current way of grabbing output during a unit test and should be taught how to grab logging.py output too, by teaching it about the wekbit-patch logging handler (currently > INFO) and how to disable/remove it while an OutputCapture is in place. Yes, Dirk said basically the same thing in comment 12. >> WebKitTools/Scripts/webkitpy/test/skip_unittest.py:59 >> + return TestSkipFixture > > I'm confused why not just declare this at the top of the file? Creating it dynamically in this method doesn't make much sense to me? The decorator modifies the class itself. Since we want to test skip_if twice (once with True and once with False) we need to have two different classes so that the tests don't interfere with each other. Maybe I should have added a comment. >> WebKitTools/Scripts/webkitpy/test/skip_unittest.py:65 >> + klass = skip_if(self.create_fixture_class(), False, 'Should not see this message.', logger=self.logger) > > Would have been nice to see an example of it using the decorator syntax. My understanding is that class decorator syntax wasn't added until Python 2.6. >> WebKitTools/Scripts/webkitpy/test/skip_unittest.py:75 >> + > > two spaces. Hm, check-webkit-style didn't complain here.
(In reply to comment #18) > (From update of attachment 72671 [details]) > I assume the decorator looks different because it is a class decorator. What does the class decorator syntax look like? I wonder if we want a class decorator or just function decorators like unittest seems to have? > >> WebKitTools/Scripts/webkitpy/test/skip_unittest.py:59 > >> + return TestSkipFixture > > > > I'm confused why not just declare this at the top of the file? Creating it dynamically in this method doesn't make much sense to me? > > The decorator modifies the class itself. Since we want to test skip_if twice (once with True and once with False) we need to have two different classes so that the tests don't interfere with each other. Maybe I should have added a comment. Yeah, I'm just not sure what a "class decorator" looks like, and I'm not sure we want one over method decorators. We could basically write our own wkunittest.py module, and then use things like this via: @wkunittest.skipIf(condition) I think it would be *super awesome* to be able to use more of the new skip* stuff from unittest, so I'm glad you've started us down that path. I just think we should try to be as much like the stuff coming in python 2.7 instead of inventing our own syntax. > >> WebKitTools/Scripts/webkitpy/test/skip_unittest.py:65 > >> + klass = skip_if(self.create_fixture_class(), False, 'Should not see this message.', logger=self.logger) > > > > Would have been nice to see an example of it using the decorator syntax. > > My understanding is that class decorator syntax wasn't added until Python 2.6. http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/memoized.py seems to work with 2.5. :)
Obviously if when we skip this test we should add a FIXME, since we still dont' seem to understand why it wedges.
(In reply to comment #19) > > Yeah, I'm just not sure what a "class decorator" looks like, and I'm not sure we want one over method decorators. > I think the class decorator mechanism is useful to avoid a lot of cutting and pasting, since we know none of the tests in that class are going to work. It seems like skipping modules and classes would be generally useful in our testing infrastructure. I've actually been debating looking at switching to unittest2, since it has all the 2.7+ goodness but has been backported to work with the earlier versions. -- Dirk
(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 72671 [details] [details]) > > I assume the decorator looks different because it is a class decorator. > > What does the class decorator syntax look like? I wonder if we want a class decorator or just function decorators like unittest seems to have? Class decorator syntax looks just like method decorator syntax, except you put it above a class declaration instead of a method declaration: @mydecorator class MyClass: You can see an example in <http://docs.python.org/library/unittest.html#skipping-tests-and-expected-failures> where it says "Classes can be skipped just like methods:" > > >> WebKitTools/Scripts/webkitpy/test/skip_unittest.py:59 > > >> + return TestSkipFixture > > > > > > I'm confused why not just declare this at the top of the file? Creating it dynamically in this method doesn't make much sense to me? > > > > The decorator modifies the class itself. Since we want to test skip_if twice (once with True and once with False) we need to have two different classes so that the tests don't interfere with each other. Maybe I should have added a comment. > > Yeah, I'm just not sure what a "class decorator" looks like, and I'm not sure we want one over method decorators. Hopefully the above information will help you. > We could basically write our own wkunittest.py module, and then use things like this via: > @wkunittest.skipIf(condition) Sure, that sounds fine. > I think it would be *super awesome* to be able to use more of the new skip* stuff from unittest, so I'm glad you've started us down that path. I just think we should try to be as much like the stuff coming in python 2.7 instead of inventing our own syntax. OK. > > >> WebKitTools/Scripts/webkitpy/test/skip_unittest.py:65 > > >> + klass = skip_if(self.create_fixture_class(), False, 'Should not see this message.', logger=self.logger) > > > > > > Would have been nice to see an example of it using the decorator syntax. > > > > My understanding is that class decorator syntax wasn't added until Python 2.6. > > http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/memoized.py > seems to work with 2.5. :) memoized is a method decorator, not a class decorator.
Comment on attachment 72671 [details] Skip webkitpy.layout_tests.run_webkit_tests_unittest.MainTest on Cygwin Python 2.5.x View in context: https://bugs.webkit.org/attachment.cgi?id=72671&action=review > WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:292 > +MainTest = skip_if(MainTest, sys.platform == 'cygwin' and compare_version(sys, '2.6')[0] < 0, 'new-run-webkit-tests tests hang on Cygwin Python 2.5.2') OK. Since python 2.5 doesn't have class decorator syntax (pardon my confusion) this seems fine.
disclaiming ownership on this one; I'm probably not going to look at it any time soon. I'm not sure if it's even relevant any more. Adam, can we assume that everything is on Cygwin 7 (or can be made to be) instead?
We no longer support Python 2.5 in WebKit, so this can be closed, right? *** This bug has been marked as a duplicate of bug 71593 ***