Bug 64137 – test-webkitpy fails on chromium win

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

test-webkitpy fails on chromium win

[Dirk Pranke]

Created attachment 100053 [details]
Patch

[Eric Seidel (no email)]

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.

[Dirk Pranke]

Created attachment 100054 [details]
clarify comments per eseidel's review

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

What does the failure look like?

[Eric Seidel (no email)]

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.

[Dirk Pranke]

Created attachment 100060 [details]
make it clearer that there are integration tests going on

[Eric Seidel (no email)]

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?

[Dirk Pranke]

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

[Dirk Pranke]

(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.

[Dirk Pranke]

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.

[Eric Seidel (no email)]

Comment on attachment 100060 [details]
make it clearer that there are integration tests going on

OK.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.