Bug 89882 – [Qt][NRWT] Baseline and skipped file search path cleanup

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

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.

[János Badics]

Created attachment 149476 [details]
proposed patch

[WebKit Review Bot]

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.

[János Badics]

Created attachment 149478 [details]
proposed patch

Corrected my previous patch

[Csaba Osztrogonác]

Comment on attachment 149478 [details]
proposed patch

LGTM, r=me.

[WebKit Review Bot]

Comment on attachment 149478 [details]
proposed patch

Clearing flags on attachment: 149478

Committed r121244: <http://trac.webkit.org/changeset/121244>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 89966

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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)

[János Badics]

Created attachment 149929 [details]
proposed patch

[Csaba Osztrogonác]

Comment on attachment 149929 [details]
proposed patch

Nice cleanup, r=me.

[Csaba Osztrogonác]

Comment on attachment 149929 [details]
proposed patch

Clearing flags on attachment: 149929

Committed r121430: <http://trac.webkit.org/changeset/121430>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

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.

[Csaba Osztrogonác]

(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.

[Dirk Pranke]

(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.