Bug 64137

Summary: test-webkitpy fails on chromium win
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
clarify comments per eseidel's review
none
make it clearer that there are integration tests going on none

Description Dirk Pranke 2011-07-07 17:27:11 PDT
test-webkitpy fails on chromium win
Comment 1 Dirk Pranke 2011-07-07 17:31:24 PDT
Created attachment 100053 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-07-07 17:35:10 PDT
Comment on attachment 100053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100053&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:37
> +    # FIXME: We don't test some of the combinations on win32 because paths are returned using the Win32 "\\" conventions
> +    # and the tests will fail. It's unclear if there's a good solution to this.

I'm confused by this statement.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:40
> +        if sys.platform != 'win32':

A constant on the class or in the file would read better than checking in each place.
Comment 3 Dirk Pranke 2011-07-07 17:41:33 PDT
Created attachment 100054 [details]
clarify comments per eseidel's review
Comment 4 Eric Seidel (no email) 2011-07-07 17:42:56 PDT
Comment on attachment 100054 [details]
clarify comments per eseidel's review

View in context: https://bugs.webkit.org/attachment.cgi?id=100054&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:40
> +    # FIXME: We can't test some of the mac and linux code when running on
> +    # Windows because the filename paths returned from test_files.find()
> +    # have the Win32 "\\" filenames and # the code in the mac and linux ports'
> +    # implementation of 'relative_test_filename()' expect them to have forward slashes.
> +    FILES_HAVE_FORWARD_SLASHES = sys.platform != 'win32'

This is more clear, but I'm still confused why os.path.sep isn't our friend here?
Comment 5 Eric Seidel (no email) 2011-07-07 17:44:21 PDT
Comment on attachment 100054 [details]
clarify comments per eseidel's review

View in context: https://bugs.webkit.org/attachment.cgi?id=100054&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:40
>> +    FILES_HAVE_FORWARD_SLASHES = sys.platform != 'win32'
> 
> This is more clear, but I'm still confused why os.path.sep isn't our friend here?

Maybe it isn't worth the effort to make this platform-agnostic.  I don't know.  That's your judgment call to make.  But it's not immediately obvious to me why it's not platform-agnostic automatically if we're using os.path.sep where we're supposed to.  I don't see any '/' in these tests themselves.  Should we be making this check inside assertOverridesWorked instead of in each test?
Comment 6 Eric Seidel (no email) 2011-07-07 17:57:41 PDT
What does the failure look like?
Comment 7 Eric Seidel (no email) 2011-07-07 18:11:20 PDT
Comment on attachment 100054 [details]
clarify comments per eseidel's review

I think the real bug here is that these tests should talk to a MockFileSystem.  Since they aren't, they arent' unit tests and the file should be renamed _integrationtest.py appropriately.
Comment 8 Dirk Pranke 2011-07-07 18:30:36 PDT
Created attachment 100060 [details]
make it clearer that there are integration tests going on
Comment 9 Eric Seidel (no email) 2011-07-07 18:32:05 PDT
Comment on attachment 100060 [details]
make it clearer that there are integration tests going on

View in context: https://bugs.webkit.org/attachment.cgi?id=100060&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:50
> +    def integration_test_chromium_gpu_win(self):

Do integration_test methods get run?
Comment 10 Dirk Pranke 2011-07-07 18:35:45 PDT
The GPU failures (prior to the patch, obviously) looked like:

======================================================================
ERROR: test_get_chromium_gpu__on_linux (webkitpy.layout_tests.port.chromium_gpu_unittest.ChromiumGp
Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "d:\src\dev\src\third_party\WebKit\Tools\Scripts\webkitpy\layout_tests\port\chromium_gpu_uni
test.py", line 45, in test_get_chromium_gpu__on_linux
    self.assertOverridesWorked('chromium-gpu-linux', 'chromium-gpu', 'linux2')
  File "d:\src\dev\src\third_party\WebKit\Tools\Scripts\webkitpy\layout_tests\port\chromium_gpu_uni
test.py", line 91, in assertOverridesWorked
    self.assertTrue(path in files)
  File "d:\src\dev\src\third_party\WebKit\Tools\Scripts\webkitpy\layout_tests\port\chromium_gpu_uni
test.py", line 91, in assertOverridesWorked
    self.assertTrue(path in files)
  File "d:\src\depot_tools\python_bin\lib\bdb.py", line 46, in trace_dispatch
    return self.dispatch_line(frame)
  File "d:\src\depot_tools\python_bin\lib\bdb.py", line 65, in dispatch_line
    if self.quitting: raise BdbQuit
BdbQuit
Comment 11 Dirk Pranke 2011-07-07 18:39:23 PDT
(In reply to comment #9)
> (From update of attachment 100060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100060&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:50
> > +    def integration_test_chromium_gpu_win(self):
> 
> Do integration_test methods get run?

Not by default. When you run the test files directly you can run them by specifying the -i flag, e.g.: pythom chromium_gpu_unittest.py -i

I would have put a lot of money on me either having added the -i flag to test-webkitpy, or at least filed a bug and posted a patch for it, but I'm not finding it now, so I'll file another bug for it.
Comment 12 Dirk Pranke 2011-07-07 18:54:54 PDT
Filed bug 64137 for adding integration test support to test-wekbitpy.

Note that I did not rename the file in this patch, just so you can actually see the diff.
Comment 13 Eric Seidel (no email) 2011-07-07 19:13:34 PDT
Comment on attachment 100060 [details]
make it clearer that there are integration tests going on

OK.
Comment 14 WebKit Review Bot 2011-07-07 19:22:53 PDT
Comment on attachment 100060 [details]
make it clearer that there are integration tests going on

Clearing flags on attachment: 100060

Committed r90607: <http://trac.webkit.org/changeset/90607>
Comment 15 WebKit Review Bot 2011-07-07 19:22:58 PDT
All reviewed patches have been landed.  Closing bug.