64468 2011-07-13 11:18:05 -0700 WIN: DumpRenderTree hangs under NRWT 2012-01-09 13:16:39 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC Windows 7 RESOLVED FIXED InRadar P2 Normal --- 38756 1 aroben webkit-unassigned abarth dpranke eric ojan tony webkit-bug-importer webkit.review.bot oldest_to_newest 436534 0 aroben 2011-07-13 11:18:05 -0700 To reproduce: 1. new-run-webkit-tests DRT launches and hangs. The problem is that we're passing Cygwin-style paths to DRT. We need to pass Windows-style paths instead. 436713 1 eric 2011-07-13 15:41:12 -0700 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. 436718 2 dpranke 2011-07-13 15:47:09 -0700 (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. 436725 3 eric 2011-07-13 15:52:54 -0700 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. 436727 4 eric 2011-07-13 15:55:04 -0700 Hmm... I'll have to check to see what ORWT does for http tests on windows. 437084 5 aroben 2011-07-14 07:41:31 -0700 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. 437085 6 aroben 2011-07-14 07:43:10 -0700 And of course my above change completely ignores using abstraction. :-) A new method like Port.prepare_path_for_driver could do the cygpath conversion. 437266 7 dpranke 2011-07-14 12:51:02 -0700 (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) 437269 8 aroben 2011-07-14 12:53:58 -0700 Agreed. 530308 9 aroben 2012-01-05 08:25:09 -0800 (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. 530317 10 aroben 2012-01-05 08:39:43 -0800 (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 :-) 530501 11 dpranke 2012-01-05 11:58:09 -0800 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. 530503 12 dpranke 2012-01-05 11:59:13 -0800 that is, assuming webkit-based DRTs expects filenames and not URIs :). 530512 13 aroben 2012-01-05 12:05:19 -0800 (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. 530520 14 dpranke 2012-01-05 12:13:00 -0800 (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. 530521 15 dpranke 2012-01-05 12:13:26 -0800 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 ... 530531 16 aroben 2012-01-05 12:20:05 -0800 (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 530532 17 dpranke 2012-01-05 12:22:21 -0800 (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 :). 530533 18 aroben 2012-01-05 12:24:02 -0800 (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! 530534 19 aroben 2012-01-05 12:24:29 -0800 (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. 530542 20 eric 2012-01-05 12:29:35 -0800 (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. 530544 21 dpranke 2012-01-05 12:34:01 -0800 (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. 530564 22 aroben 2012-01-05 12:58:11 -0800 (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? 530587 23 dpranke 2012-01-05 13:24:49 -0800 (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. 530624 24 dpranke 2012-01-05 14:17:20 -0800 check out https://bugs.webkit.org/show_bug.cgi?id=75648 . 531058 25 aroben 2012-01-06 06:35:36 -0800 (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? 531139 26 dpranke 2012-01-06 10:13:49 -0800 (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. 532291 27 webkit-bug-importer 2012-01-09 10:33:44 -0800 <rdar://problem/10663409> 532297 28 121683 aroben 2012-01-09 10:36:05 -0800 Created attachment 121683 Convert Cygwin paths to Windows paths before passing them to DRT 532416 29 121683 dpranke 2012-01-09 12:20:22 -0800 Comment on attachment 121683 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. 532418 30 dpranke 2012-01-09 12:20:51 -0800 (since it'll be tested heavily by nrwt. 532422 31 aroben 2012-01-09 12:24:59 -0800 (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. 532424 32 dpranke 2012-01-09 12:27:20 -0800 (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. 532466 33 121683 webkit.review.bot 2012-01-09 13:16:33 -0800 Comment on attachment 121683 Convert Cygwin paths to Windows paths before passing them to DRT Clearing flags on attachment: 121683 Committed r104483: <http://trac.webkit.org/changeset/104483> 532467 34 webkit.review.bot 2012-01-09 13:16:39 -0800 All reviewed patches have been landed. Closing bug. 121683 2012-01-09 10:36:05 -0800 2012-01-09 13:16:33 -0800 Convert Cygwin paths to Windows paths before passing them to DRT bug-64468-20120109103604.patch text/plain 2078 aroben U3VidmVyc2lvbiBSZXZpc2lvbjogMTA0NDYyCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggZmVhNzNjOWJkOWUzOGQwNjRmNzM3NmM2NGM2MTk5ZGZi NzcwOTAyMS4uYjRjMTkxNGY0MTQwNzJlN2Y2NjExYTljMGU2OGJkMmMzM2I5Y2RjNyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5 IEBACisyMDEyLTAxLTA5ICBBZGFtIFJvYmVuICA8YXJvYmVuQGFwcGxlLmNvbT4KKworICAgICAg ICBDb252ZXJ0IEN5Z3dpbiBwYXRocyB0byBXaW5kb3dzIHBhdGhzIGJlZm9yZSBwYXNzaW5nIHRo ZW0gdG8gRFJUCisKKyAgICAgICAgRml4ZXMgPGh0dHA6Ly93ZWJraXQub3JnL2IvNjQ0Njg+IDxy ZGFyOi8vcHJvYmxlbS8xMDY2MzQwOT4gV0lOOiBEdW1wUmVuZGVyVHJlZSBoYW5ncyB1bmRlcgor ICAgICAgICBOUldUCisKKyAgICAgICAgSSBjb3VsZG4ndCBmaW5kIGEgd2F5IHRvIHRlc3QgdGhp cyBiZWNhdXNlIEkgY291bGRuJ3QgZmlndXJlIG91dCBob3cgdG8gb3ZlcnJpZGUgdGhlCisgICAg ICAgIGN5Z3BhdGggZnVuY3Rpb24gYXMgaXQgaXMgdXNlZCBieSBXZWJLaXREcml2ZXIuCisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBTY3JpcHRzL3dl YmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L3dlYmtpdC5weToKKyAgICAgICAgKFdlYktpdERyaXZl ci5fY29tbWFuZF9mcm9tX2RyaXZlcl9pbnB1dCk6IE9uIEN5Z3dpbiwgY29udmVydCB0aGUgdGVz dCdzIHBhdGggdG8gYQorICAgICAgICBXaW5kb3dzLXN0eWxlIHBhdGggYmVmb3JlIHBhc3Npbmcg aXQgdG8gRFJULgorCiAyMDEyLTAxLTA5ICBHYWJvciBSYXBjc2FueWkgIDxyZ2Fib3JAd2Via2l0 Lm9yZz4KIAogICAgICAgICBbR3RrXVtBUk1dIENyb3NzIGNvbXBpbGVyIGJ1aWxkYm90IGlzIGZh aWxpbmcgYWZ0ZXIgcjEwMzA0MApkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9s YXlvdXRfdGVzdHMvcG9ydC93ZWJraXQucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91 dF90ZXN0cy9wb3J0L3dlYmtpdC5weQppbmRleCBlYWFiOWFlZjZjYTMxYTNlOWE3ZGJhMTE2NzA4 NmU2OWFiMjQ2N2E0Li5jZGQ2YWYyZDZiNjY2MDg5MzdhOWU0OGQ5Yjk1OWIxNmU2YzMzNWFiIDEw MDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L3dlYmtp dC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L3dlYmtp dC5weQpAQCAtNDMsNiArNDMsNyBAQCBmcm9tIHdlYmtpdHB5LmNvbW1vbi5tZW1vaXplZCBpbXBv cnQgbWVtb2l6ZWQKIGZyb20gd2Via2l0cHkuY29tbW9uLm5ldC5idWlsZGJvdCBpbXBvcnQgQnVp bGRCb3QKIGZyb20gd2Via2l0cHkuY29tbW9uLnN5c3RlbS5lbnZpcm9ubWVudCBpbXBvcnQgRW52 aXJvbm1lbnQKIGZyb20gd2Via2l0cHkuY29tbW9uLnN5c3RlbS5leGVjdXRpdmUgaW1wb3J0IEV4 ZWN1dGl2ZSwgU2NyaXB0RXJyb3IKK2Zyb20gd2Via2l0cHkuY29tbW9uLnN5c3RlbS5wYXRoIGlt cG9ydCBjeWdwYXRoCiBmcm9tIHdlYmtpdHB5LmxheW91dF90ZXN0cy5wb3J0IGltcG9ydCBidWls ZGVycywgc2VydmVyX3Byb2Nlc3MsIFBvcnQsIERyaXZlciwgRHJpdmVyT3V0cHV0CiAKIApAQCAt NTI1LDYgKzUyNiw4IEBAIGNsYXNzIFdlYktpdERyaXZlcihEcml2ZXIpOgogICAgICAgICAgICAg Y29tbWFuZCA9IHNlbGYudGVzdF90b191cmkoZHJpdmVyX2lucHV0LnRlc3RfbmFtZSkKICAgICAg ICAgZWxzZToKICAgICAgICAgICAgIGNvbW1hbmQgPSBzZWxmLl9wb3J0LmFic3BhdGhfZm9yX3Rl c3QoZHJpdmVyX2lucHV0LnRlc3RfbmFtZSkKKyAgICAgICAgICAgIGlmIHN5cy5wbGF0Zm9ybSA9 PSAnY3lnd2luJzoKKyAgICAgICAgICAgICAgICBjb21tYW5kID0gY3lncGF0aChjb21tYW5kKQog CiAgICAgICAgIGlmIGRyaXZlcl9pbnB1dC5pbWFnZV9oYXNoOgogICAgICAgICAgICAgIyBGSVhN RTogV2h5IHRoZSBsZWFkaW5nIHF1b3RlPwo=