[Webkit-unassigned] [Bug 48614] webkitpy.layout_tests.run_webkit_tests_unittest.MainTest gets wedged on Windows XP/Cygwin 1.5/Python 2.5.2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48614





--- Comment #18 from Adam Roben (aroben) <aroben at apple.com>  2010-11-02 12:40:18 PST ---
(From update of attachment 72671)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.