Bug 89882 - [Qt][NRWT] Baseline and skipped file search path cleanup
Summary: [Qt][NRWT] Baseline and skipped file search path cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: János Badics
URL:
Keywords: Qt, QtTriaged
Depends on: 89108 89966
Blocks: 89880
  Show dependency treegraph
 
Reported: 2012-06-25 08:43 PDT by Csaba Osztrogonác
Modified: 2012-06-28 15:15 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (5.73 KB, patch)
2012-06-26 01:12 PDT, János Badics
no flags Details | Formatted Diff | Diff
proposed patch (5.73 KB, patch)
2012-06-26 01:18 PDT, János Badics
no flags Details | Formatted Diff | Diff
proposed patch (7.89 KB, patch)
2012-06-28 05:09 PDT, János Badics
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-06-25 08:43:26 PDT
Qt port uses same search paths for Skipped file and baseline 
search path. (And will for TestExpectations too)

We should get rid of copy/paste code for determining paths.
Comment 1 János Badics 2012-06-26 01:12:00 PDT
Created attachment 149476 [details]
proposed patch
Comment 2 WebKit Review Bot 2012-06-26 01:15:21 PDT
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.
Comment 3 János Badics 2012-06-26 01:18:29 PDT
Created attachment 149478 [details]
proposed patch

Corrected my previous patch
Comment 4 Csaba Osztrogonác 2012-06-26 02:36:17 PDT
Comment on attachment 149478 [details]
proposed patch

LGTM, r=me.
Comment 5 WebKit Review Bot 2012-06-26 02:42:24 PDT
Comment on attachment 149478 [details]
proposed patch

Clearing flags on attachment: 149478

Committed r121244: <http://trac.webkit.org/changeset/121244>
Comment 6 WebKit Review Bot 2012-06-26 02:42:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 WebKit Review Bot 2012-06-26 03:08:10 PDT
Re-opened since this is blocked by 89966
Comment 8 Csaba Osztrogonác 2012-06-26 03:14:22 PDT
(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 9 Csaba Osztrogonác 2012-06-26 03:17:41 PDT
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)
Comment 10 János Badics 2012-06-28 05:09:35 PDT
Created attachment 149929 [details]
proposed patch
Comment 11 Csaba Osztrogonác 2012-06-28 07:29:56 PDT
Comment on attachment 149929 [details]
proposed patch

Nice cleanup, r=me.
Comment 12 Csaba Osztrogonác 2012-06-28 07:33:23 PDT
Comment on attachment 149929 [details]
proposed patch

Clearing flags on attachment: 149929

Committed r121430: <http://trac.webkit.org/changeset/121430>
Comment 13 Csaba Osztrogonác 2012-06-28 07:33:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Dirk Pranke 2012-06-28 14:27:23 PDT
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.
Comment 15 Csaba Osztrogonác 2012-06-28 14:33:34 PDT
(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.
Comment 16 Dirk Pranke 2012-06-28 15:15:07 PDT
(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.