Summary: | [Qt][NRWT] Baseline and skipped file search path cleanup | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Csaba Osztrogonác <ossy> | ||||||||
Component: | Tools / Tests | Assignee: | János Badics <jbadics> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, dpranke, ojan, ossy, webkit.review.bot | ||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 89108, 89966 | ||||||||||
Bug Blocks: | 89880 | ||||||||||
Attachments: |
|
Description
Csaba Osztrogonác
2012-06-25 08:43:26 PDT
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. |