Bug 77450 - [Qt][WK2] Refactor on Qt5 Layout tests' structure
Summary: [Qt][WK2] Refactor on Qt5 Layout tests' structure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 77380
  Show dependency treegraph
 
Reported: 2012-01-31 10:18 PST by Jesus Sanchez-Palencia
Modified: 2012-02-10 05:05 PST (History)
3 users (show)

See Also:


Attachments
Patch (82.67 KB, patch)
2012-01-31 11:37 PST, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
updated patch with svn cp/mv to keep the history (14.64 KB, patch)
2012-02-09 08:49 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2012-01-31 10:18:44 PST
The current structure is inconsistent since we can skip tests for Qt5 + WK2 only but for Qt5 + WK1 only. Also, there are lots of tests replicated across the skipped lists and maintenance cost of these lists is rapidly increasing.

I will upload a patch soon that proposes a way to refactor this structure as needed, starting with Qt5 (wk1 and wk2). It can be later applied to other permutations of our testing matrix (qt 4 or 5, wk1 or 2, OS win, linux or mac, arm or not, ...).
Comment 1 Jesus Sanchez-Palencia 2012-01-31 11:37:55 PST
Created attachment 124786 [details]
Patch
Comment 2 Jesus Sanchez-Palencia 2012-01-31 11:39:29 PST
(In reply to comment #1)
> Created an attachment (id=124786) [details]
> Patch

Ossy, it would be cool if you could review this. I didn't include the whole "git mv qt-wk2 qt-5.0-wk2" on this patch because its size was getting to 6.2 MB.

If I get the r+ on this I will land it all together manually.
Comment 3 WebKit Review Bot 2012-01-31 11:40:42 PST
Attachment 124786 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/WebCore.exp.in
Auto-merging Source/WebKit/mac/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Csaba Osztrogonác 2012-02-01 05:01:41 PST
Now we can skip tests on different ways:
- qt-4.8: Qt4.8-WK1
- qt-5.0: Qt5.0-WK1 and Qt5.0-WK2
- qt-wk1: Qt4.8-WK1 and Qt5.0-WK1
- qt-wk2: Qt5.0-WK2

Which is missing that we can't skip a test for only Qt5.0-WK1.

With your change we can skip tests on different ways:
- qt-4.8: Qt4.8-WK1
- qt-5.0: Qt5.0-WK1 and Qt5.0-WK2
- qt-5.0-wk1: Qt5.0-WK1
- qt-5.0-wk2: Qt5.0-WK2

But now we can't skip tests for Qt4.8-WK1 and Qt5.0-WK1 (recent qt-wk1),
and you added duplicated entries to qt-4.8 and qt-5.0-wk1 skipped lists.
What if we add qt-5.0-wk1 and qt-5.0-wk2 and don't remove the general qt-wk1?
Comment 5 Csaba Osztrogonác 2012-02-01 05:10:05 PST
Otherwise I think it would be better if we 
- remove obsolete (4 month old) png files (introduced in http://trac.webkit.org/changeset/95412) from qt-wk2 before this patch lands and reland updated png files after https://bugs.webkit.org/show_bug.cgi?id=70484 fixed.
- land the future r+ -ed patch from svn to preserv the Skipped list histories.
(use svn cp, move, etc.)
Comment 6 Jesus Sanchez-Palencia 2012-02-01 06:41:34 PST
(In reply to comment #4)
> But now we can't skip tests for Qt4.8-WK1 and Qt5.0-WK1 (recent qt-wk1),
> and you added duplicated entries to qt-4.8 and qt-5.0-wk1 skipped lists.
> What if we add qt-5.0-wk1 and qt-5.0-wk2 and don't remove the general qt-wk1?

I know it might sound weird but from my brief experience with gardening I'd rather have to replicate this tests in qt-4.8 and qt-5.0-wk1. It seemed to me more natural just to skip or unskip tests directly from a list that actually tells what it is dealing with...

(In reply to comment #5)
> - remove obsolete (4 month old) png files (introduced in http://trac.webkit.org/changeset/95412) from qt-wk2 before this patch lands and reland updated png files after https://bugs.webkit.org/show_bug.cgi?id=70484 fixed.
> - land the future r+ -ed patch from svn to preserv the Skipped list histories.
> (use svn cp, move, etc.)

Not sure I'm following you here... Why do need to remove the png files and add new ones? Should I do that or one of you guys will deal with it when https://bugs.webkit.org/show_bug.cgi?id=70484 is fixed ?


Thanks for the review!
Comment 7 Csaba Osztrogonác 2012-02-07 09:00:33 PST
(In reply to comment #6)
> (In reply to comment #4)
> > But now we can't skip tests for Qt4.8-WK1 and Qt5.0-WK1 (recent qt-wk1),
> > and you added duplicated entries to qt-4.8 and qt-5.0-wk1 skipped lists.
> > What if we add qt-5.0-wk1 and qt-5.0-wk2 and don't remove the general qt-wk1?
> 
> I know it might sound weird but from my brief experience with gardening I'd rather have to replicate this tests in qt-4.8 and qt-5.0-wk1. It seemed to me more natural just to skip or unskip tests directly from a list that actually tells what it is dealing with...

It's all the same for me, I can agree with you if there are only a couple of duplicated tests.
 
> (In reply to comment #5)
> > - remove obsolete (4 month old) png files (introduced in http://trac.webkit.org/changeset/95412) from qt-wk2 before this patch lands and reland updated png files after https://bugs.webkit.org/show_bug.cgi?id=70484 fixed.
> > - land the future r+ -ed patch from svn to preserv the Skipped list histories.
> > (use svn cp, move, etc.)
> 
> Not sure I'm following you here... Why do need to remove the png files and add new ones? Should I do that or one of you guys will deal with it when https://bugs.webkit.org/show_bug.cgi?id=70484 is fixed ?

Because these png files are absolutely obsolete ... And I don't know when will https://bugs.webkit.org/show_bug.cgi?id=70484 be fixed and when can we enable pixel tests. But I don't think if in the near future.

So I prefer removing obsolete png files instead of moving them with a huge patch.
Comment 8 Csaba Osztrogonác 2012-02-07 09:01:35 PST
Balázs, have you got any objection against removing these obsolete png's?
Comment 9 Balazs Kelemen 2012-02-08 10:22:56 PST
(In reply to comment #8)
> Balázs, have you got any objection against removing these obsolete png's?

Please, do that! :)
Comment 10 Csaba Osztrogonác 2012-02-09 08:01:49 PST
(In reply to comment #9)
> (In reply to comment #8)
> > Balázs, have you got any objection against removing these obsolete png's?
> 
> Please, do that! :)
Done. ;)
Comment 11 Csaba Osztrogonác 2012-02-09 08:49:33 PST
Created attachment 126312 [details]
updated patch with svn cp/mv to keep the history
Comment 12 Csaba Osztrogonác 2012-02-09 08:50:41 PST
Comment on attachment 126312 [details]
updated patch with svn cp/mv to keep the history

r=me and I'll land the patch manually from svn.
Comment 13 Jesus Sanchez-Palencia 2012-02-09 10:59:49 PST
(In reply to comment #12)
> (From update of attachment 126312 [details])
> r=me and I'll land the patch manually from svn.

Thank you, Ossy! I'm rushing a few things here but please let me know if there is anything else I need to do after this, ok?

Thanks again!
Comment 14 Csaba Osztrogonác 2012-02-10 05:05:14 PST
Comment on attachment 126312 [details]
updated patch with svn cp/mv to keep the history

Landed in http://trac.webkit.org/changeset/107397 and http://trac.webkit.org/changeset/107398