Bug 64468

It appears that Chromium's DumpRenderTree implementation expects file:/// uris: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py#L439 but all the WebKit ones seem to expect absolute file paths? http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py#L465 is that correct? It should be easy to convert the paths, but we don't have very standard cygwin support in the codebase yet.

(In reply to comment #1) > It appears that Chromium's DumpRenderTree implementation expects file:/// uris: > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py#L439 > > but all the WebKit ones seem to expect absolute file paths? > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py#L465 > > is that correct? > As far as I know, yes. > It should be easy to convert the paths, but we don't have very standard cygwin support in the codebase yet. Do you need more than what we have in common.system.path ? There is also the port.uri_to_test_name() and port.abspath_for_test() routines that should allow you to get from any variant of URL to path to test.

It looks like abspath_for_test should already do the right thing on windows: def abspath_for_test(self, test_name): """Returns the full path to the file for a given test name. This is the inverse of relative_test_filename().""" return self._filesystem.normpath(self._filesystem.join(self.layout_tests_dir(), test_name)) I think the bug is that we're calling test_to_uri in WebKItDriver when we really want to just call abspath_for_test. I'll upload a patch which might fix this, but again, someone with Windows will have to test.

Hmm... I'll have to check to see what ORWT does for http tests on windows.

Note that abspath_for_test isn't sufficient on Windows because it will produce a Cygwin-style path. The bare-minimum required change is here: http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py?rev=90826#L465 Changing this: command = uri[7:] if uri.startswith("file:///") else uri to this: command = cygpath(urllib.unquote(uri[7:])) if uri.startswith("file:///") else uri This is a bit icky, of course, since we're converting to a URL then back to a path (but the code already does this). I think the urllib.unquote call is actually the right thing to do on all platforms if we keep this test name -> URL -> file path approach. Only the cygpath() call is Windows-specific.

And of course my above change completely ignores using abstraction. :-) A new method like Port.prepare_path_for_driver could do the cygpath conversion.

(In reply to comment #5) > Note that abspath_for_test isn't sufficient on Windows because it will produce a Cygwin-style path. > > The bare-minimum required change is here: > > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py?rev=90826#L465 > > Changing this: > > command = uri[7:] if uri.startswith("file:///") else uri > > to this: > > command = cygpath(urllib.unquote(uri[7:])) if uri.startswith("file:///") else uri > > This is a bit icky, of course, since we're converting to a URL then back to a path (but the code already does this). I think the urllib.unquote call is actually the right thing to do on all platforms if we keep this test name -> URL -> file path approach. Only the cygpath() call is Windows-specific. There's no reason to go through the URL if you don't need to. You could just as easily change command_From_driver_input() to something like: if driver_input.test_name.startswith('http/'): command = self.test_to_uri(driver_input.test_name) else: command = cygpath(self.abspath_for_test(driver_input.test_name)

Agreed.

(In reply to comment #7) > There's no reason to go through the URL if you don't need to. You could just as easily change command_From_driver_input() to something like: > > if driver_input.test_name.startswith('http/'): > command = self.test_to_uri(driver_input.test_name) > else: > command = cygpath(self.abspath_for_test(driver_input.test_name) Unfortunately, test_to_uri contains some tricky logic about whether to use an http:/https: URL or a file: URL. I don't think we want to duplicate that logic in command_from_driver_input. Maybe we can share the logic, or maybe we can make the roundtrip through test_to_uri work.

(In reply to comment #5) > Note that abspath_for_test isn't sufficient on Windows because it will produce a Cygwin-style path. > > The bare-minimum required change is here: > > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py?rev=90826#L465 > > Changing this: > > command = uri[7:] if uri.startswith("file:///") else uri > > to this: > > command = cygpath(urllib.unquote(uri[7:])) if uri.startswith("file:///") else uri > > This is a bit icky, of course, since we're converting to a URL then back to a path (but the code already does this). I think the urllib.unquote call is actually the right thing to do on all platforms if we keep this test name -> URL -> file path approach. Only the cygpath() call is Windows-specific. It seems that uri now contains a Windows-style file: URL, not a Cygwin-style file: URL. Not sure when that changed. Anyway, now the minimum change is: command = urllib.unquote(uri[8:]).replace('/', '\\') if uri.startswith("file:///") else uri Gotta come up with some better solution that doesn't break Mac etc. of course :-)

Test names as passed to run_test() are expected to be relative-path filenames. We could centralize and standardize testing whether the test is an http test in a separate method on Port() if we wanted, but the change I suggested in comment #7 is correct for the cygwin platform. It would probably be good to change cygpath to be a no-op on non-cygpath platform just to be portable, or you could change the implementation to get an abspath and then only call cygpath if needed. I would not go through the uri transformation; it's unnecessary and confusing.

that is, assuming webkit-based DRTs expects filenames and not URIs :).

(In reply to comment #11) > Test names as passed to run_test() are expected to be relative-path filenames. We could centralize and standardize testing whether the test is an http test in a separate method on Port() if we wanted, but the change I suggested in comment #7 is correct for the cygwin platform. It would probably be good to change cygpath to be a no-op on non-cygpath platform just to be portable, or you could change the implementation to get an abspath and then only call cygpath if needed. > > I would not go through the uri transformation; it's unnecessary and confusing. Your proposed change in comment 7 isn't quite right: tests in http/tests/local are supposed to be loaded as local files, not as http: URLs. At least according to Port.test_to_uri. It seems best to me to keep cygpath as-is. Having it be a no-op for most developers will most likely mean that it won't be used correctly. I'm trying to come up with a good name for the concept of "path to test that the driver will understand" as opposed to "path to test that Python will understand". If we had such a name, we could have a Port.test_to_<name> function that would generate the right type of path.

(In reply to comment #13) > Your proposed change in comment 7 isn't quite right: tests in http/tests/local are supposed to be loaded as local files, not as http: URLs. At least according to Port.test_to_uri. > Oh. Hm. You're right. That's bad, as it means the logic in controllers/manager.py is broken, and we should probably create an is_http_test() to actually codify/fix this. > It seems best to me to keep cygpath as-is. Having it be a no-op for most developers will most likely mean that it won't be used correctly. > Sounds good. > I'm trying to come up with a good name for the concept of "path to test that the driver will understand" as opposed to "path to test that Python will understand". If we had such a name, we could have a Port.test_to_<name> function that would generate the right type of path. "path to test that the driver will understand" is probably only actually used under Driver.run_test(), so I'm not sure if there's value in creating a separate routine for it (at least, not on the Port object, we could put on on the driver object if necessary). By that same logic, test_to_uri() is only used by the chromium Driver (and various pieces of test code), and should probably be moved out of the Port object and down into the chromium Driver.

As an aside, it seems goofy that we have tests that aren't http tests under the http directory, but that long predates me so I have no idea why ...

(In reply to comment #15) > As an aside, it seems goofy that we have tests that aren't http tests under the http directory, but that long predates me so I have no idea why ... http://trac.webkit.org/changeset/19553

(In reply to comment #16) > (In reply to comment #15) > > As an aside, it seems goofy that we have tests that aren't http tests under the http directory, but that long predates me so I have no idea why ... > > http://trac.webkit.org/changeset/19553 Ah. What an annoying special case to have to cover :).

(In reply to comment #14) > (In reply to comment #13) > > Your proposed change in comment 7 isn't quite right: tests in http/tests/local are supposed to be loaded as local files, not as http: URLs. At least according to Port.test_to_uri. > > > > Oh. Hm. You're right. That's bad, as it means the logic in controllers/manager.py is broken, and we should probably create an is_http_test() to actually codify/fix this. Sounds good to me! I think http/tests/websocket/tests/local is supposed to be run locally too. > > I'm trying to come up with a good name for the concept of "path to test that the driver will understand" as opposed to "path to test that Python will understand". If we had such a name, we could have a Port.test_to_<name> function that would generate the right type of path. > > "path to test that the driver will understand" is probably only actually used under Driver.run_test(), so I'm not sure if there's value in creating a separate routine for it (at least, not on the Port object, we could put on on the driver object if necessary). Does Driver know whether it's running under Cygwin or not? I didn't think it did. > By that same logic, test_to_uri() is only used by the chromium Driver (and various pieces of test code), and should probably be moved out of the Port object and down into the chromium Driver. It's also used by WebKitPort._command_from_driver_input, of course!

(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > As an aside, it seems goofy that we have tests that aren't http tests under the http directory, but that long predates me so I have no idea why ... > > > > http://trac.webkit.org/changeset/19553 > > Ah. What an annoying special case to have to cover :). We could probably change things such that Apache is always running whenever any test is run. Then these tests could be moved out of the http/ directory entirely.

(In reply to comment #19) > (In reply to comment #17) > > (In reply to comment #16) > > > (In reply to comment #15) > > > > As an aside, it seems goofy that we have tests that aren't http tests under the http directory, but that long predates me so I have no idea why ... > > > > > > http://trac.webkit.org/changeset/19553 > > > > Ah. What an annoying special case to have to cover :). > > We could probably change things such that Apache is always running whenever any test is run. Then these tests could be moved out of the http/ directory entirely. The Qt folks run more than one instance of RWT/NRWT per server. So they are excited about having httpd run for as short a period of time as possible. I'm not sure we really want to support that usecase (at least not in that way). A better alternative would be to fix all DRT instances to remap port 8080 requests to whatever port the DRT was told to remap to on startup, but that's more involved.

(In reply to comment #18) > (In reply to comment #14) > > (In reply to comment #13) > > > Your proposed change in comment 7 isn't quite right: tests in http/tests/local are supposed to be loaded as local files, not as http: URLs. At least according to Port.test_to_uri. > > > > > > > Oh. Hm. You're right. That's bad, as it means the logic in controllers/manager.py is broken, and we should probably create an is_http_test() to actually codify/fix this. > > Sounds good to me! I think http/tests/websocket/tests/local is supposed to be run locally too. > Actually, now that I think about it further, the manager logic is correct; it needs to know whether a server is needed, not whether the test itself is local or http. > Does Driver know whether it's running under Cygwin or not? I didn't think it did. > It certainly could know if it needed it ... > > By that same logic, test_to_uri() is only used by the chromium Driver (and various pieces of test code), and should probably be moved out of the Port object and down into the chromium Driver. > > It's also used by WebKitPort._command_from_driver_input, of course! right :) Even so, this should probably be on the Driver class and not the port class if nothing outside of Drivers need it. (In reply to comment #19) > We could probably change things such that Apache is always running whenever any test is run. Then these tests could be moved out of the http/ directory entirely. In addition to Eric's comment, it also makes NRWT's startup significantly slower in the common case of just running a subset of non-http tests.

(In reply to comment #21) > (In reply to comment #18) > > (In reply to comment #14) > > Does Driver know whether it's running under Cygwin or not? I didn't think it did. > > > > It certainly could know if it needed it ... > > > > By that same logic, test_to_uri() is only used by the chromium Driver (and various pieces of test code), and should probably be moved out of the Port object and down into the chromium Driver. > > > > It's also used by WebKitPort._command_from_driver_input, of course! > > right :) Even so, this should probably be on the Driver class and not the port class if nothing outside of Drivers need it. I don't think I understand your proposal well enough to implement it myself. Any chance you'd be willing to take a stab at it?

(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #18) > > > (In reply to comment #14) > > > Does Driver know whether it's running under Cygwin or not? I didn't think it did. > > > > > > > It certainly could know if it needed it ... > > > > > > By that same logic, test_to_uri() is only used by the chromium Driver (and various pieces of test code), and should probably be moved out of the Port object and down into the chromium Driver. > > > > > > It's also used by WebKitPort._command_from_driver_input, of course! > > > > right :) Even so, this should probably be on the Driver class and not the port class if nothing outside of Drivers need it. > > I don't think I understand your proposal well enough to implement it myself. Any chance you'd be willing to take a stab at it? Sure, I can probably post something in a bit.

check out https://bugs.webkit.org/show_bug.cgi?id=75648 .

(In reply to comment #24) > check out https://bugs.webkit.org/show_bug.cgi?id=75648 . Thanks! (In reply to comment #21) > (In reply to comment #18) > > Does Driver know whether it's running under Cygwin or not? I didn't think it did. > > > > It certainly could know if it needed it ... Any preference for how this should be done?

(In reply to comment #25) > (In reply to comment #24) > > check out https://bugs.webkit.org/show_bug.cgi?id=75648 . > > Thanks! > > (In reply to comment #21) > > (In reply to comment #18) > > > Does Driver know whether it's running under Cygwin or not? I didn't think it did. > > > > > > > It certainly could know if it needed it ... > > Any preference for how this should be done? I typically just check for sys.platform == 'cygwin' where I need to. There's no need for this code to be particularly platform independent.

<rdar://problem/10663409>

Created attachment 121683 [details] Convert Cygwin paths to Windows paths before passing them to DRT

Comment on attachment 121683 [details] Convert Cygwin paths to Windows paths before passing them to DRT I think you probably could have imported webkitpy.common.system.path in webkit_unittest and then overridden cygpath to point to a mocked-out function, but I think this is okay, too.

(since it'll be tested heavily by nrwt.

(In reply to comment #29) > (From update of attachment 121683 [details]) > I think you probably could have imported webkitpy.common.system.path in webkit_unittest and then overridden cygpath to point to a mocked-out function, but I think this is okay, too. I tried that but it didn't seem to work. The real cygpath was being called, not my mocked-out version.

(In reply to comment #31) > (In reply to comment #29) > > (From update of attachment 121683 [details] [details]) > > I think you probably could have imported webkitpy.common.system.path in webkit_unittest and then overridden cygpath to point to a mocked-out function, but I think this is okay, too. > > I tried that but it didn't seem to work. The real cygpath was being called, not my mocked-out version. Hm. I'll have to dig into that at some point to figure out what was going on.

Comment on attachment 121683 [details] Convert Cygwin paths to Windows paths before passing them to DRT Clearing flags on attachment: 121683 Committed r104483: <http://trac.webkit.org/changeset/104483>

All reviewed patches have been landed. Closing bug.