Bug 37664 – Chromium: Add --chromium option to new-run-webkit-websocketserver

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

webkitpy.layout_tets.ports.websocket_server will use port_obj.path_from_chromium_base(), but this method is available only for chromium port obj.

[Fumitoshi Ukai]

Created attachment 53456 [details]
Patch

[Adam Barth]

Comment on attachment 53456 [details]
Patch

Ok...

+ if self._chromium and sys.platform in ('cygwin', 'win32'):

All of these lines are wrong.  Knowledge of chromium should be confined to the chromium port classes.

[Adam Barth]

Comment on attachment 53456 [details]
Patch

Oops.  Misclick.  This code is improperly factored. websocket_server.py should have no knowledge of chromium or path_from_chromium_base.  All that knowledge needs to be moved into files with the word "chromium" in their names.

[Fumitoshi Ukai]

Created attachment 53773 [details]
Patch

[Adam Barth]

Comment on attachment 53773 [details]
Patch

This looks great!  What is self._chromium used for?  I don't see it anywhere...

[Fumitoshi Ukai]

(In reply to comment #5)
> (From update of attachment 53773 [details])
> This looks great!  What is self._chromium used for?  I don't see it anywhere...

Oh, unnecessary. I'll remove self._chromium from websocket_server.PyWebSocket.

[Fumitoshi Ukai]

Committed r58077: <http://trac.webkit.org/changeset/58077>

[Eric Seidel (no email)]

Clearly we need better unit testing since this took 3 clean-up patches. :)

[Eric Seidel (no email)]

Thank you for doing this fix.  This will make running new-run-webkit-tests on Apple Windows much easier.

[Eric Seidel (no email)]

Actually it took 5 cleanup patches, and it's still wrong.  This broke rebaseline-chromium-webkit-tests:
http://paste.lisp.org/display/98166

We shouldnt' be doing this CYGWIN stuff in __init__ but rather in some other explicit method call.

[Eric Seidel (no email)]

We need to get better about requiring unit testing for every python change.  We were good about it at the beginning of webkitpy, but have slacked with the addition of all this untested chromium py code. :(

I've been bad about it too.  See my 12 commits to try and get the unicode() stuff working yesterday (which is still broken).

[Adam Barth]

Okiedokes.  All future changes much have unit tests.  You heard it here first.

[James Robinson]

Created attachment 54103 [details]
Patch

[James Robinson]

Patch up to revert this and all the follow-ups.  rebaseline-chromium-webkit-test is still broken.  Please re-land after this code has some testing (at least manual tests before landing).

[Adam Barth]

Comment on attachment 54103 [details]
Patch

Sad, but ok.

[Fumitoshi Ukai]

Comment on attachment 54103 [details]
Patch

> @@ -230,7 +246,7 @@ class PyWebSocket(http_server.Lighttpd):
>             pid = self._process.pid
>         elif self._pidfile:
>              with codecs.open(self._pidfile, "r", "ascii") as file:
> -                pid = int(file.read().strip())
> +                pid = int(f.read().strip())
> 
>         if not pid:
>             raise PyWebSocketNotFound(

Don't revert here!!

[Fumitoshi Ukai]

(In reply to comment #10)
> Actually it took 5 cleanup patches, and it's still wrong.  This broke
> rebaseline-chromium-webkit-tests:
> http://paste.lisp.org/display/98166
> 
> We shouldnt' be doing this CYGWIN stuff in __init__ but rather in some other
> explicit method call.

It looks rebaseline_chromium_webkit_tests.py assumes chromium port, but the log looks didn't run in chromium tree.  So, usage error?

[James Robinson]

Before this set of patches, I could run rebaseline-chromium-webkit-tests just fine from a pure WebKit checkout.  Breaking that behavior would be a regression.  Regardless, it's pretty obvious from the stack that the script is attempting to run windows-specific logic on a mac, which won't work from a WebKit or a chromium checkout.

[James Robinson]

Log:


100422 17:47:47 rebaseline_chromium_webkit_tests.py:151 INFO ----------------------------- Rebaseline done: mac -----------------------------
Traceback (most recent call last):
  File "./WebKitTools/Scripts/rebaseline-chromium-webkit-tests", line 44, in <module>
    rebaseline_chromium_webkit_tests.main()
  File "/usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py", line 1069, in main
    rebaseliner = Rebaseliner(port_obj, target_port_obj, platform, options)
  File "/usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py", line 224, in __init__
    self._target_port.test_platform_name_to_name(platform), options)
  File "/usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/webkitpy/layout_tests/port/factory.py", line 85, in get
    return chromium_win.ChromiumWinPort(port_name, options)
  File "/usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py", line 56, in __init__
    self.path_from_chromium_base('third_party', 'cygwin', 'bin'),
  File "/usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py", line 148, in path_from_chromium_base
    abspath)
AssertionError: could not find Chromium base dir from /usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.pyc

[Fumitoshi Ukai]

(In reply to comment #18)
> Before this set of patches, I could run rebaseline-chromium-webkit-tests just
> fine from a pure WebKit checkout.  Breaking that behavior would be a
> regression.  Regardless, it's pretty obvious from the stack that the script is
> attempting to run windows-specific logic on a mac, which won't work from a
> WebKit or a chromium checkout.

Then, it should be a bug of webkitpy.layout_tests.port.factory.  It got wrong port object?

[James Robinson]

I don't know what the exact problem is.  I know that it used to work, then a bunch of patches landed, and now it doesn't work.  I would much rather get back to the state where it worked and then have folks figure out what the bug is without checking it back in.

[James Robinson]

reopening

[WebKit Commit Bot]

Comment on attachment 54103 [details]
Patch

Clearing flags on attachment: 54103

Committed r58146: <http://trac.webkit.org/changeset/58146>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fumitoshi Ukai]

revert was landed. So reopen this bug.

[Fumitoshi Ukai]

Created attachment 54130 [details]
Patch

[Adam Barth]

Comment on attachment 54130 [details]
Patch

What's different this time?  There's also the question of when we're going to start requiring tests for this stuff.

[Fumitoshi Ukai]

(In reply to comment #27)
> (From update of attachment 54130 [details])
> What's different this time?  There's also the question of when we're going to
> start requiring tests for this stuff.

Extracted code using path_from_chromium_base() into setup_environ_for_server() from constructor.
Since chromium port object might be constructed in non-chromium tree (e.g rebaseline tool), we couldn't use path_from_chromium_base() in constructor even if it is in chromium port object.
So, move such code to setup_environ_for_server() and this method is called at original location.

I'm not sure what kind of tests would be ok.. because it depends on mostly platform, so simple test would run on some platform, but may not on other.

[Fumitoshi Ukai]

Created attachment 54267 [details]
Patch

[Chris Jerdonek]

Comment on attachment 54267 [details]
Patch

(In reply to comment #29)
> Created an attachment (id=54267) [details]

I notice several style improvements that can be made to this patch.  Some
of these improvements may be judgment calls while others are more clearly
spelled out by PEP8.  I'm including all of them for future guidance and consideration, etc.

I will let another reviewer review the substantive elements of this patch, as I haven't studied this particular area of code much.

> +++ b/WebKitTools/ChangeLog
> +        env setup and setup_mount for cygwin is moved in ChromiumWinPort.setup_environ_for_server.
> +
> +        * Scripts/webkitpy/layout_tests/port/http_server.py:
> +          remove register_cygwin parameter
> +          call setup_environ_for_server()
> +        * Scripts/webkitpy/layout_tests/port/websocket_server.py:
> +          remove register_cygwin parameter
> +          call setup_environ_for_server()

The sentences in the ChangeLog comments would be more readable if they
began with a capital letter and ended in a period as proper sentences do.

> +++ b/WebKitTools/Scripts/new-run-webkit-websocketserver
> @@ -55,6 +55,10 @@ def main():
>                               default='', help='TLS private key file.')
>      option_parser.add_option('-c', '--certificate', dest='certificate',
>                               default='', help='TLS certificate file.')
> +    option_parser.add_option('--chromium', action='store_true',
> +                             dest='chromium',
> +                             default=False,
> +                             help='use the Chromium port')

The formatting of the help text should be consistent with the formatting
of the texts of the other help options for this command.  Does the prevailing
formatting for these options begin with a capital letter?  Does the
prevailing format give the texts an ending period or not?

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> @@ -419,7 +419,12 @@ class Port(object):
>  
>      def setup_test_run(self):
>          """This routine can be overridden to perform any port-specific
> -        work that shouuld be done at the beginning of a test run."""
> +        work that should be done at the beginning of a test run."""
> +        pass
> +
> +    def setup_environ_for_server(self):
> +        """This routine can be overridden to perform any port-specific
> +        work that should be done at the beginning of a server launch."""

PEP8 describes specific style rules that doc strings should follow.
In particular--

(1) The first sentence should fit on one line (under 80 characters).
(2) The first sentence should read as a command (e.g. "Set up a test run.").
(3) The closing triple quotes should be on a line by themselves and be
    preceded by a blank line.
    
For example--

>      def setup_test_run(self):
>          """Perform port-specific work at the beginning of a test run.
>
>          You can override this routine if necessary.
>
>          """

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py
> +        # Put the cygwin directory first in the path to find cygwin1.dll

PEP8 says that, generally speaking, Python comments should be written
as a complete sentence and end in a period.

> +        # Put the cygwin directory so that pywebsocket finds proper python
> +        # executable to run cgi program.

Do you mean to say, "Configure the cygwin directory so that..."?

> +        if sys.platform == 'win32' and self._options and \
> +                hasattr(self._options, 'register_cygwin') and \
> +                self._options.register_cygwin:

PEP8 says that the preferred way to wrap long lines is to use implied
line continuation rather than a trailing backslash.  This will also
make it clearer where to indent on the second and subsequent lines, e.g.

> +        if (sys.platform == 'win32' and self._options and
> +            hasattr(self._options, 'register_cygwin') and
> +            self._options.register_cygwin):


> +            setup_mount = self.path_from_chromium_base(
> +                'third_party', 'cygwin', 'setup_mount.bat')

This may read better as--

> +            setup_mount = self.path_from_chromium_base('third_party',
> +                                                       'cygwin',
> +                                                       'setup_mount.bat')

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/factory_unittest.py
> +import unittest
> +import chromium_linux
> +import chromium_mac
> +import chromium_win
> +import dryrun
> +import factory
> +import gtk
> +import mac
> +import qt
> +import sys
> +import test
> +import win

PEP8 says to group import statements in the following order and separate
the groups by a blank line: (1) standard library, (2) third party, and 
(3) local library.  For example--

import sys
import unittest

import chromium_...
...

> +class FactoryTest(unittest.TestCase):
> +    """Tests factory can create proper port object from port_name,
> +    sys.platform and options.
> +    """

Docstring styling as above.

> +    class WebKitOptions(object):
> +        """Mimimum options for WebKit port."""

Minimum.  Also, you may want to say, "Represents the minimum options...
."

> +    class ChromiumOptions(WebKitOptions):
> +        """Mimimum options for Chromium port."""

Minimum.

[Fumitoshi Ukai]

Created attachment 55213 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55213 [details]
Patch

+        You can override this routine if necessary.

doesn't seem to add any clarity and should likely be removed.

Seems:
 201         self._port_obj.setup_environ_for_server()

Should return an env.  Then you don't ever need to modify os.environ.

this should be some sort of helper assert:
124         sys.platform = 'cygwin'
 125         self.assertTrue(isinstance(factory.get(options=self.chromium_options),
 126                                    chromium_win.ChromiumWinPort))
self.assert_port("cygwin", chromium.ChromiumWinPort, self.chromium_options)
or similar.

That would get rid of a bunch of copy/paste code in your test file.

r- for the environ modification and the copy/paste code.

[Fumitoshi Ukai]

Created attachment 55520 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 55520 [details]
Patch

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:52
 +          chromium.ChromiumPort.setup_environ_for_server(self)
This is wrong.  I know it doesn't do anyting yet, but I Think you meant env = chromium.ChromiumPort.setup_environ_for_server, no?

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:54
 +          env = os.environ
This line wouldn't be needed if the above was fixed.

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:55
 +          env['PATH'] = '%s;%s' % (
Not a big deal at all, but I think the conciseness was to standardize on " over '.   I don't really care which we use so long as we try to pick one. :)

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:62
 +          if (sys.platform == 'win32' and self._options and
We need to move away from this hasattr pattern and towards one where the options are closer to the code at hand.  A genric "options" element is a bad design.  Code which uses certain optiosn should always require those to be passed.

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:68
 +              subprocess.Popen(setup_mount).wait()
Why not Executive.run_command()?

WebKitTools/Scripts/webkitpy/layout_tests/port/factory_unittest.py:51
 +      class WebKitOptions(object):
The ports themselves should expose what options they require.  But we'll fix that globally at some later date.

WebKitTools/Scripts/webkitpy/layout_tests/port/factory_unittest.py:139
 +  if __name__ == '__main__':
I don't think these are generally used in our unittest setup.

Certainly looks better than the code that was there before.  r- for the env = chromium.ChromiumPort.setup_environ_for_server() issue, all the rest are nits.

[Fumitoshi Ukai]

Created attachment 56210 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 56210 [details]
Patch

I'm uncertain if we should be returning a copy of os.environ or not.
 426         return os.environ

Your change does not change behavior though, so it's fine.

[Fumitoshi Ukai]

Committed r59595: <http://trac.webkit.org/changeset/59595>

[Eric Seidel (no email)]

Looks like there are some WebSocket test failures on Windows:
http://build.webkit.org/results/Windows%20Release%20(Tests)/r59626%20(12752)/results.html

[Fumitoshi Ukai]

It doesn't pass options to factory.get()), so it gets WinPort even if --chromium flag is used.

[Fumitoshi Ukai]

Created attachment 56569 [details]
Patch

[Fumitoshi Ukai]

Committed r59823: <http://trac.webkit.org/changeset/59823>

[WebKit Review Bot]

http://trac.webkit.org/changeset/59823 might have broken SnowLeopard Intel Release (Tests)

[Eric Seidel (no email)]

The latest change broke websocket tests on all platforms.  Please fix or rollout.

[Fumitoshi Ukai]

(In reply to comment #43)
> The latest change broke websocket tests on all platforms.  Please fix or rollout.

Sorry.
Fixed by r59824: <http://trac.webkit.org/changeset/59824>