webkitpy.layout_tets.ports.websocket_server will use port_obj.path_from_chromium_base(), but this method is available only for chromium port obj.
Created attachment 53456 [details] Patch
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.
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.
Created attachment 53773 [details] Patch
Comment on attachment 53773 [details] Patch This looks great! What is self._chromium used for? I don't see it anywhere...
(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.
Committed r58077: <http://trac.webkit.org/changeset/58077>
Clearly we need better unit testing since this took 3 clean-up patches. :)
Thank you for doing this fix. This will make running new-run-webkit-tests on Apple Windows much easier.
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.
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).
Okiedokes. All future changes much have unit tests. You heard it here first.
Created attachment 54103 [details] Patch
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).
Comment on attachment 54103 [details] Patch Sad, but ok.
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!!
(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?
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.
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
(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?
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.
reopening
Comment on attachment 54103 [details] Patch Clearing flags on attachment: 54103 Committed r58146: <http://trac.webkit.org/changeset/58146>
All reviewed patches have been landed. Closing bug.
revert was landed. So reopen this bug.
Created attachment 54130 [details] Patch
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.
(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.
Created attachment 54267 [details] Patch
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.
Created attachment 55213 [details] Patch
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.
Created attachment 55520 [details] Patch
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.
Created attachment 56210 [details] Patch
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.
Committed r59595: <http://trac.webkit.org/changeset/59595>
Looks like there are some WebSocket test failures on Windows: http://build.webkit.org/results/Windows%20Release%20(Tests)/r59626%20(12752)/results.html
It doesn't pass options to factory.get()), so it gets WinPort even if --chromium flag is used.
Created attachment 56569 [details] Patch
Committed r59823: <http://trac.webkit.org/changeset/59823>
http://trac.webkit.org/changeset/59823 might have broken SnowLeopard Intel Release (Tests)
The latest change broke websocket tests on all platforms. Please fix or rollout.
(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>