Created attachment 149476 [details] proposed patch

Attachment 149476 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 149478 [details] proposed patch Corrected my previous patch

Comment on attachment 149478 [details] proposed patch LGTM, r=me.

Comment on attachment 149478 [details] proposed patch Clearing flags on attachment: 149478 Committed r121244: <http://trac.webkit.org/changeset/121244>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 89966

(In reply to comment #5) > (From update of attachment 149478 [details]) > Clearing flags on attachment: 149478 > > Committed r121244: <http://trac.webkit.org/changeset/121244> Rolled out by http://trac.webkit.org/changeset/121247, because skip list path is incorrect. We need one more round.

Comment on attachment 149478 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=149478&action=review > Tools/Scripts/webkitpy/layout_tests/port/qt.py:140 > + skipped_path = self._search_paths() > + skipped_path.append('wk2') > + return skipped_path wk2 shouldn't be added unconditionally. We should add it if self.qt_version() is 5.0 and self.get_option('webkit_test_runner') is true. And it seems we don't have unittest for _skipped_file_search_paths. Could you add it too? (without copy/pasting the full test_baseline_search_path)

Created attachment 149929 [details] proposed patch

Comment on attachment 149929 [details] proposed patch Nice cleanup, r=me.

Comment on attachment 149929 [details] proposed patch Clearing flags on attachment: 149929 Committed r121430: <http://trac.webkit.org/changeset/121430>

All reviewed patches have been landed. Closing bug.

Do you actually need a qt-5.0-wk1 directory as well as a qt-5.0-wk2 directory? Can't you just wk1-specific failures in qt-5.0? We don't have a wk1-specific directories for any other ports.

(In reply to comment #14) > Do you actually need a qt-5.0-wk1 directory as well as a qt-5.0-wk2 directory? Can't you just wk1-specific failures in qt-5.0? > > We don't have a wk1-specific directories for any other ports. Yes, we need them. If a test fails only on qt-5.0-wk1 platform, we should skip it only on qt-5.0-wk1. If we added it to qt-5.0, it would be skipped on qt-5.0-wk2 too, it would be incorrect.

(In reply to comment #15) > (In reply to comment #14) > > Do you actually need a qt-5.0-wk1 directory as well as a qt-5.0-wk2 directory? Can't you just wk1-specific failures in qt-5.0? > > > > We don't have a wk1-specific directories for any other ports. > > Yes, we need them. If a test fails only on qt-5.0-wk1 platform, > we should skip it only on qt-5.0-wk1. If we added it to qt-5.0, > it would be skipped on qt-5.0-wk2 too, it would be incorrect. I see. I had been thinking of this just from the point of view of storing baselines, and not from managing TestExpectations/Skipped lists. We should probably be more explicit about which directories are allowed to actually contain baselines and which should only have TestExpectations/Skipped files. When I implemented the TestExpectations cascade, that is actually done through a separate function (port.expectations_files()). Maybe we need to duplicate some of the baseline_search_path() logic there instead and use that for Skipped files and TestExpectations files.