Bug 91754

I think something like this is a good idea. I don't think I'd like it to be a command-line argument, though, mostly because I don't want to have to specify it every time. Perhaps it should be an entry in the TestExpectations file, or a separate port-specific list instead.

Balazs, you assigned this to yourself; does that mean that you want to work on it? If not, I'd be happy to work up something.

(In reply to comment #2) > Balazs, you assigned this to yourself; does that mean that you want to work on it? If not, I'd be happy to work up something. Yes, I started implementing this. I will upload something tomorrow :)

Created attachment 153526 [details] first attempt - with command line argument

Comment on attachment 153526 [details] first attempt - with command line argument Attachment 153526 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13303380

Created attachment 153534 [details] buildfixed

(In reply to comment #1) > I think something like this is a good idea. I don't think I'd like it to be a command-line argument, though, mostly because I don't want to have to specify it every time. > > Perhaps it should be an entry in the TestExpectations file, or a separate port-specific list instead. I guess you mean developers wants to run the same set of pixel tests that are runned on the bots, right? It's a good point but still we have to allow to run all pixel tests. I can think about something like that: https://gist.github.com/3152176. So by default you run the same set of pixel tests that are defined by the platform (so executed on bots as well), but you can get the current behavior of --pixel-tests by adding "--pixel-test-directories all". It could still be a bit strange for people at first, hopefully we can make it understood by discussing the change on the mail list. So, do you think this is a reasonable approach (a question for everybody who reads this)?

Comment on attachment 153534 [details] buildfixed Generally this approach looks pretty good, and is more-or-less how I was thinking of implementing it as well. I'm glad you're adding the per-test-arg support for pixel-tests as well (although we could theoretically split that into a second patch). We should add some tests for this to run_webkit_tests_integrationtest.py as well. View in context: https://bugs.webkit.org/attachment.cgi?id=153534&action=review > Tools/ChangeLog:43 > + and Qt DRT currently. This supposed to exist only temporary until other ports don't nit: remove the "don't" at the end of the line :). > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:746 > + m_dumpPixelsForAllTests = dump; Do we need this flag? Maybe we should pass --pixel-test/--no-pixel-test for every test instead to be explicit, and not have a process-wide default? Maybe we want this for compatibility w/ ORWT, but it might be better to just change ORWT instead? > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:101 > + for directory in self._options.pixel_test_directories) I think I would make this whole call a method on the port() object, so that the logic is encapsulated there. That way the list of pixel_test_directories can also be hard-coded on the port if so desired, and also, more generally, I like to push everything that is optional or port-specific behind the port() interface, so that there are fewer of these complicated rules in the non-port code. > Tools/Scripts/webkitpy/layout_tests/port/base.py:616 > + return directory in test_name.split(self.TEST_PATH_SEPARATOR) Are you sure this doesn't need to be test_name.startswith() ? For example, if you have '--pixel-test-directories compositing', this'll pick up compositing/foo.html *and* platform/qt/compositing/foo.html. Perhaps that is intentional (I can see an argument for this)? However, this would also mean that if I had '--pixel-test-directories qt' we'd run *every* platform-specific qt directory with pixel tests, which seems less good. > Tools/Scripts/webkitpy/layout_tests/port/base.py:619 > + return re.sub(self.layout_tests_dir() + str(self.TEST_PATH_SEPARATOR), '', test_name) I think this is reimplementing port.relative_test_filename(). > Tools/Scripts/webkitpy/layout_tests/port/driver.py:298 > + if pixel_tests and not self._port.supports_switching_pixel_tests_per_test(): I'm not sure if this interacts correctly with the 'shouldDumpAllPixelTests' option you've added, which is partially why I'm wondering if we should just get rid of shouldDumpAll() ... Also, we should check that this interacts correctly with DriverProxy, so that if the port doesn't support switching per test, --pixel-test-directories will still work and we'll just switch between the two DRTs, like we do for reftests.

I should have mentioned that I'm not the most familiar with DRT/WTR code, so while the change looks good to me, I don't know if there's a better way to implement it so it might be nice if someone else chimed in.

> > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:746 > > + m_dumpPixelsForAllTests = dump; > > Do we need this flag? Maybe we should pass --pixel-test/--no-pixel-test for every test instead to be explicit, and not have a process-wide default? > Maybe we want this for compatibility w/ ORWT, but it might be better to just change ORWT instead? I kept it for compatibility with orwt and also it could be useful sometimes when you have to debug something related to pixel tests. It's controversial whether it is worth to keep it, but I rather feel like we should keep it until every port can handle per test specification. > > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:101 > > + for directory in self._options.pixel_test_directories) > > I think I would make this whole call a method on the port() object, so that the logic is encapsulated there. That way the list of pixel_test_directories can also be hard-coded on the port if so desired, and also, more generally, I like to push everything that is optional or port-specific behind the port() interface, so that there are fewer of these complicated rules in the non-port code. Good idea, I will consider this in the next patch. > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:616 > > + return directory in test_name.split(self.TEST_PATH_SEPARATOR) > > Are you sure this doesn't need to be test_name.startswith() ? For example, if you have '--pixel-test-directories compositing', this'll pick up compositing/foo.html *and* platform/qt/compositing/foo.html. Perhaps that is intentional (I can see an argument for this)? However, this would also mean that if I had '--pixel-test-directories qt' we'd run *every* platform-specific qt directory with pixel tests, which seems less good. It was not intentional, so I will change it. > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:619 > > + return re.sub(self.layout_tests_dir() + str(self.TEST_PATH_SEPARATOR), '', test_name) > > I think this is reimplementing port.relative_test_filename(). > > > Tools/Scripts/webkitpy/layout_tests/port/driver.py:298 > > + if pixel_tests and not self._port.supports_switching_pixel_tests_per_test(): > > I'm not sure if this interacts correctly with the 'shouldDumpAllPixelTests' option you've added, which is partially why I'm wondering if we should just get rid of shouldDumpAll() ... It should interact correctly. I did not add an option just renamed the variable holding whether --pixel-tests have been added, and I don't push this parameter to the driver if per test switching is supported. > Also, we should check that this interacts correctly with DriverProxy, so that if the port doesn't support switching per test, --pixel-test-directories will still work and we'll just switch between the two DRTs, like we do for reftests. DriverProxy seems like a bit of a hack to me. Can't we just get rid of it after every ports support per test pixel test setting?

Comment on attachment 153534 [details] buildfixed Need another round.

View in context: https://bugs.webkit.org/attachment.cgi?id=153534&action=review Just a python nit: > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:192 > + sanitized = [] Why don't you use set() here?

One more thing. > I think I would make this whole call a method on the port() object, so that the logic is encapsulated there. That way the list of pixel_test_directories can also be hard-coded on the port if so desired, and also, more generally, I like to push everything that is optional or port-specific behind the port() interface, so that there are fewer of these complicated rules in the non-port code. > I think we need more flexibility than just hard coding these directories in the port object. A hard coded default is reasonable, but I think it should be possible to override it with the command line arg. I will include this in the next patch.

(In reply to comment #10) > > > > > Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:746 > > > + m_dumpPixelsForAllTests = dump; > > > > Do we need this flag? Maybe we should pass --pixel-test/--no-pixel-test for every test instead to be explicit, and not have a process-wide default? > > Maybe we want this for compatibility w/ ORWT, but it might be better to just change ORWT instead? > > I kept it for compatibility with orwt and also it could be useful sometimes when you have to debug something related to pixel tests. It's controversial whether it is worth to keep it, but I rather feel like we should keep it until every port can handle per test specification. > Okay. I don't feel strongly about this, I just wanted to mention the option. > > Also, we should check that this interacts correctly with DriverProxy, so that if the port doesn't support switching per test, --pixel-test-directories will still work and we'll just switch between the two DRTs, like we do for reftests. > > DriverProxy seems like a bit of a hack to me. Can't we just get rid of it after every ports support per test pixel test setting? Yes, definitely, that's the plan. I just wanted the feature to work in the meantime. (In reply to comment #13) > One more thing. > > > I think I would make this whole call a method on the port() object, so that the logic is encapsulated there. That way the list of pixel_test_directories can also be hard-coded on the port if so desired, and also, more generally, I like to push everything that is optional or port-specific behind the port() interface, so that there are fewer of these complicated rules in the non-port code. > > > > I think we need more flexibility than just hard coding these directories in the port object. A hard coded default is reasonable, but I think it should be possible to override it with the command line arg. I will include this in the next patch. I agree completely; I did not mean to suggest we didn't also want the command line flag.

Created attachment 154015 [details] wip patch, test is missing yet

> > > > DriverProxy seems like a bit of a hack to me. Can't we just get rid of it after every ports support per test pixel test setting? > > Yes, definitely, that's the plan. I just wanted the feature to work in the meantime. > For me it seems easier to just implement it in every DRT, so I did not tried to make it work with DriverProxy. With the test I have some trouble, so I need some more time to find my way in integration tests and write a test for this feature.

Created attachment 154101 [details] proposed patch, test included

Comment on attachment 154101 [details] proposed patch, test included View in context: https://bugs.webkit.org/attachment.cgi?id=154101&action=review getting close ... > Tools/ChangeLog:15 > + with these drivers. So, the idea is that if you don't pass --pixel-test-directories and the port's _in_default_pixel_test_directory() always returns false then we will run all pixel tests, right? > Tools/Scripts/webkitpy/layout_tests/port/base.py:755 > + return relative_path if self._filesystem.exists(self._filesystem.join(self.layout_tests_dir(), relative_path)) else None It looks like you're adding this function so that we can support one of three ways of specifying tests ... 1) absolute paths, e.g. '/checkout/LayoutTests/foo 2) paths relative to the checkout, e.g., 'LayoutTests/foo' 3) paths relative to 'LayoutTests', e.g., 'foo' right? I don't know that supporting (1) is worth it or needed, but maybe you have some need for it I'm not aware of? Most of the other ways of specifying tests and directories don't support this method, so I would prefer not to support it, for consistency. Supporting (2) is done for NRWT in the manager code (in collect_tests() and strip_test_dir_prefixes()). It would be good to combine that code with this. Can we constrain this patch to just supporting (3), and then support (2) in a separate patch? If you would prefer to leave things the way they are, can you at least add a FIXME: comment to combine the code later? Lastly, this name is too similar to relative_test_filename(); I don't think callers would know when to use one or the other, so we should find a different name. We should also figure out if we can just combine the two routines - does anything *rely* on relatlive_test_filename only having the third type of filename passed as input? > Tools/Scripts/webkitpy/layout_tests/port/base.py:-746 > - # filenames with backslashes in them. Why did you delete this comment? > Tools/Scripts/webkitpy/layout_tests/port/driver.py:344 > + command += "'" + '--pixel-test' We should add a comment noting that we are intentionally passing the flag even if the driver was started with --pixel-tests (and to be clear, I think passing the flag is the right thing to do). > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:348 > + Example: \'animations compositing\'.'''), typically we either just specify the command line argument multiple times and do action='append' (as in --additional-platform-directory) or we would do that and also optionally have the list be comma-separated (NRWT doesn't use this style, but webkit-patch does in places). Can we either just start with specifying the argument multiple times or support that plus using commas instead, to be consistent?

(In reply to comment #16) > For me it seems easier to just implement it in every DRT, so I did not tried to make it work with DriverProxy. > Fair enough. However, I'd expect it to almost just work (if not just work). What happens if you run this and specify some directories and just set _supports_switching_pixel_tests_per_test() to return False?

(In reply to comment #18) > (From update of attachment 154101 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154101&action=review > > getting close ... > > > Tools/ChangeLog:15 > > + with these drivers. > > So, the idea is that if you don't pass --pixel-test-directories and the port's _in_default_pixel_test_directory() always returns false then we will run all pixel tests, right? It was messed up a bit, so I changed it. > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:755 > > + return relative_path if self._filesystem.exists(self._filesystem.join(self.layout_tests_dir(), relative_path)) else None > > It looks like you're adding this function so that we can support one of three ways of specifying tests ... > > 1) absolute paths, e.g. '/checkout/LayoutTests/foo > 2) paths relative to the checkout, e.g., 'LayoutTests/foo' > 3) paths relative to 'LayoutTests', e.g., 'foo' > > right? > > I don't know that supporting (1) is worth it or needed, but maybe you have some need for it I'm not aware of? Most of the other ways of specifying tests and directories don't support this method, so I would prefer not to support it, for consistency. > > Supporting (2) is done for NRWT in the manager code (in collect_tests() and strip_test_dir_prefixes()). It would be good to combine that code with this. > > Can we constrain this patch to just supporting (3), and then support (2) in a separate patch? If you would prefer to leave things the way they are, can you at least add a FIXME: comment to combine the code later? > > Lastly, this name is too similar to relative_test_filename(); I don't think callers would know when to use one or the other, so we should find a different name. We should also figure out if we can just combine the two routines - does anything *rely* on relatlive_test_filename only having the third type of filename passed as input? I think collect_tests supports all the three way, so I was thinking I should do the same. Yes, it's better to leave it to a follow-up patch, so now I restricted to the third form as you suggested. > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:-746 > > - # filenames with backslashes in them. > > Why did you delete this comment? By accident. > > > Tools/Scripts/webkitpy/layout_tests/port/driver.py:344 > > + command += "'" + '--pixel-test' > > We should add a comment noting that we are intentionally passing the flag even if the driver was started with --pixel-tests (and to be clear, I think passing the flag is the right thing to do). We should pass --pixel-tests to the driver when it is started, or pass --pixel-test for each test that should run as pixel test, but not both. I added a comment and an assert to make it clearer. > > > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:348 > > + Example: \'animations compositing\'.'''), > > typically we either just specify the command line argument multiple times and do action='append' (as in --additional-platform-directory) or we would do that and also optionally have the list be comma-separated (NRWT doesn't use this style, but webkit-patch does in places). Can we either just start with specifying the argument multiple times or support that plus using commas instead, to be consistent? Ok, I restricted it to accept a single arg multiple times. Probably I can look into accepting a comma separated list later. (In reply to comment #19) > (In reply to comment #16) > > For me it seems easier to just implement it in every DRT, so I did not tried to make it work with DriverProxy. > > > > Fair enough. However, I'd expect it to almost just work (if not just work). What happens if you run this and specify some directories and just set _supports_switching_pixel_tests_per_test() to return False? This would not work correctly with the previous patch. I changed it so if per test switch is not supported we don't try to handle the option.

Comment on attachment 154101 [details] proposed patch, test included View in context: https://bugs.webkit.org/attachment.cgi?id=154101&action=review r-'ing this since it sounds like you've got another patch on the way. >>> Tools/Scripts/webkitpy/layout_tests/port/base.py:755 >>> + return relative_path if self._filesystem.exists(self._filesystem.join(self.layout_tests_dir(), relative_path)) else None >> >> It looks like you're adding this function so that we can support one of three ways of specifying tests ... >> >> 1) absolute paths, e.g. '/checkout/LayoutTests/foo >> 2) paths relative to the checkout, e.g., 'LayoutTests/foo' >> 3) paths relative to 'LayoutTests', e.g., 'foo' >> >> right? >> >> I don't know that supporting (1) is worth it or needed, but maybe you have some need for it I'm not aware of? Most of the other ways of specifying tests and directories don't support this method, so I would prefer not to support it, for consistency. >> >> Supporting (2) is done for NRWT in the manager code (in collect_tests() and strip_test_dir_prefixes()). It would be good to combine that code with this. >> >> Can we constrain this patch to just supporting (3), and then support (2) in a separate patch? If you would prefer to leave things the way they are, can you at least add a FIXME: comment to combine the code later? >> >> Lastly, this name is too similar to relative_test_filename(); I don't think callers would know when to use one or the other, so we should find a different name. We should also figure out if we can just combine the two routines - does anything *rely* on relatlive_test_filename only having the third type of filename passed as input? > > I think collect_tests supports all the three way, so I was thinking I should do the same. Yes, it's better to leave it to a follow-up patch, so now I restricted to the third form as you suggested. Nope, collect_tests() won't accept absolute paths (at least in the case I just tested, and I didn't think it worked, either). We could certainly change that, but I've never heard someone actually request it.

(In reply to comment #21) > Nope, collect_tests() won't accept absolute paths (at least in the case I just tested, and I didn't think it worked, either). We could certainly change that, but I've never heard someone actually request it. Argh, I'm totally wrong. It does work, my test was bad. I guess this is an unintentional feature. For the record, I have nothing against this working, but I should probably add a test or something to make sure it keeps working :)

Created attachment 154412 [details] Patch

Comment on attachment 154412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154412&action=review Looks good enough. A few comments, but nothing show-stopping ... Do you think we can get rid of --skip-pixel-test-if-no-baseline at some point given this? > Tools/Scripts/webkitpy/layout_tests/port/base.py:1278 > + # --pixel-test-directoy is not specified. Ports can override this according to their default nit: typo: --pixel-test-directo*r*y. I would probably also reword the last sentence as "Ports can override this to specify a default subset of all of the tests to run as pixel tests" and maybe rename the function to _should_run_as_pixel_test() to follow the pattern for _supports_switching etc. > Tools/Scripts/webkitpy/layout_tests/port/test.py:518 > + # It should be supported by all platform in the near future anyway. Nit: I'd probably remove this comment > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:190 > + warnings.append("--pixel-test-directories has no effect without --pixel-tests") Nit: maybe --pixel-test-directories should just imply --pixel-tests (i.e., set pixel_tests to True)? > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:193 > + sanitized = set() I'd probably not used "sanitized" as the name here. Maybe "verified_dirs" or "valid_dirs"? > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:201 > + warnings.append("--pixel-test-directories a directory but got: %s" % str(directory)) I'd reword this to "--pixel-test-directories was passed '%s', which doesn't seem to be a directory", or something like that.

(In reply to comment #24) > (From update of attachment 154412 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154412&action=review > > Looks good enough. A few comments, but nothing show-stopping ... > > Do you think we can get rid of --skip-pixel-test-if-no-baseline at some point given this? Yes, it seems like not really useful. Fixed all the nits. Committed in http://trac.webkit.org/changeset/123729.

And unreviewed buildfixes landed in http://trac.webkit.org/changeset/123736 and http://trac.webkit.org/changeset/123738 . Unfortunately it made all tests crash on Qt debug mode without crash log. Maybe they aren't real crash, but serious bug in this patch. r123728 - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/24111 r123729 - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/24108 I tried to roll out these patches, but I can't because of conflicts. I don't have time to try to resolve conflicts manually because of your buggy patch. Please roll it out as soon as possible and please watch the bots next time after landing.

(In reply to comment #26) > And unreviewed buildfixes landed in http://trac.webkit.org/changeset/123736 and http://trac.webkit.org/changeset/123738 . > > Unfortunately it made all tests crash on Qt debug mode without crash log. Maybe they aren't real crash, but serious bug in this patch. > > r123728 - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/24111 > r123729 - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/24108 > > I tried to roll out these patches, but I can't because of conflicts. I don't have time to try to resolve conflicts manually because of your buggy patch. Please roll it out as soon as possible and please watch the bots next time after landing. I am trying to fix. I made a rollout patch but first I would like to take a try fixing it.

(In reply to comment #27) > (In reply to comment #26) > > And unreviewed buildfixes landed in http://trac.webkit.org/changeset/123736 and http://trac.webkit.org/changeset/123738 . > > > > Unfortunately it made all tests crash on Qt debug mode without crash log. Maybe they aren't real crash, but serious bug in this patch. > > > > r123728 - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/24111 > > r123729 - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/24108 > > > > I tried to roll out these patches, but I can't because of conflicts. I don't have time to try to resolve conflicts manually because of your buggy patch. Please roll it out as soon as possible and please watch the bots next time after landing. > > I am trying to fix. I made a rollout patch but first I would like to take a try fixing it. I made a debug build but I cannot reproduce the issue. It's mistery, this code should have no effect without -p - if there is no serious bug in it, but I don't see where it is. I'm going to upload the rollout after double-checking that it works.

Created attachment 154915 [details] rollout patch

Comment on attachment 154915 [details] rollout patch rs=me View in context: https://bugs.webkit.org/attachment.cgi?id=154915&action=review > Tools/ChangeLog:8 > + Rollout r123729 because it made Qt debug bots crasy. Crazy

I had to rollout the rollout because somehow it made pixel-tests turned on by default. Now debugging it.

Found it. This is the fix: --- a/Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp +++ b/Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp @@ -696,7 +696,7 @@ void DumpRenderTree::processLine(const QString &input) m_expectedHash = QString(); // single quote marks the pixel dump hash int indexOfFirstSeparator = line.indexOf('\''); - int indexOfSecondSeparator = line.indexOf('\'', indexOfFirstSeparator); + int indexOfSecondSeparator = line.indexOf('\'', indexOfFirstSeparator + 1); if (indexOfFirstSeparator > -1) { int indexOfPixelHash = indexOfFirstSeparator + 1; but now svn is down. I will commit asap.

Hopefully r123877 will fix the issues.

Bots are ok now. Sorry for the ridiculous amount of noise.