Bug 72489 – [NRWT] Fix --platform=qt-5.0 --new-baseline combo

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Csaba Osztrogonác]

Now NRWT ignores --platform option and generate new results into platform/qt-linux directory.

Explicit --platform=qt, --platform=qt-5.0, --platform=qt-wk2 options 
don't work. But -2 option makes NRWT to generate results into platform/qt-wk2 
directory irrespectively of --platform option

[Eric Seidel (no email)]

This is a bug in the QtPort implementation in qt.py.  The bug shouldn't really be *there* specifically, since the platform parsing should not be a responsibility of the ports themselves, but that's how the current design works.  Qt.__init__ needs to learn how to parse the platform name like other ports do.

Eventually I will come along and rip all that out of the __init__ methods and move it to PortFactory (port/factory.py) where it belongs.

[Eric Seidel (no email)]

Compare:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/qt.py#L62

with say:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/apple.py#L59

and you'll see what I mean.  Note the FIXME's in apple.py.

I would start by writing unittests in qt_unittest.py which have your desired behavior.  Then figure out where in the port hierarchy to fix what you want.

[Balazs Ankes]

Thank you the help!

[Balazs Ankes]

Created attachment 116964 [details]
proposed fix

Firstly I modified the qt.py then I had to modify run-webkit-tests file.

[Eric Seidel (no email)]

Comment on attachment 116964 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=116964&action=review

> Tools/Scripts/run-webkit-tests:108
> +    my $isPlatformSet = 0;
> +    for (@ARGV){
> +        # Pass --qt if platform isn't passed explicitly (eg. qt-5.0, qt-wk2, ...)
> +        if(/^--platform.*/){
> +            $isPlatformSet = 1;
> +        }
> +    }

I'm not sure I understand this part of the change.

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:77
> +        if port_name != self.port_name:
> +            self._name = port_name
> +        else:
> +            name_components = [self.port_name]
> +            if self._operating_system:
> +                name_components.append(self._operating_system)
> +            self._name = "-".join(name_components)

But this part looks good.  It's unfortunate this all has to go in teh Port class itself.  But for now that's teh curent design.

[Csaba Osztrogonác]

Comment on attachment 116964 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=116964&action=review

>> Tools/Scripts/run-webkit-tests:108
>> +    }
> 
> I'm not sure I understand this part of the change.

If you execute run-webkit-tests without this change, it will pass the explicitly added 
--platform=qt-5.0 and --qt too to the NRWT. And unfortunately --qt overrides --platform=qt-5.0

[Eric Seidel (no email)]

Comment on attachment 116964 [details]
proposed fix

Yes, makes sense.  I suspect se should just pass --platform=qt always instead of using the --qt alias.  But we can clean that up later.

[Csaba Osztrogonác]

Comment on attachment 116964 [details]
proposed fix

Clearing flags on attachment: 116964

Committed r101451: <http://trac.webkit.org/changeset/101451>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Rafael Brandao]

This doesn't seem to work anymore. Reopening bug.

[Csaba Osztrogonác]

(In reply to comment #10)
> This doesn't seem to work anymore. Reopening bug.
Thanks for noticing it. We will check it tomorrow with Balázs.

[Rafael Brandao]

Created attachment 127361 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127361 [details]
Patch

Clearing flags on attachment: 127361

Committed r110115: <http://trac.webkit.org/changeset/110115>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

Reopening as it breaks the unit tests:

======================================================================
FAIL: test_baseline_search_path (webkitpy.layout_tests.port.qt_unittest.QtPortTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/slave/webkitgtk/gtk-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py", line 71, in test_baseline_search_path
    self._assert_search_path(['qt-5.0-wk2', 'qt-mac', 'qt-5.0', 'qt'], 'mac', use_webkit2=True, qt_version='5.0')
  File "/home/slave/webkitgtk/gtk-linux-64-release/build/Tools/Scripts/webkitpy/layout_tests/port/qt_unittest.py", line 54, in _assert_search_path
    self.assertEquals(port.baseline_search_path(), absolute_search_paths)
AssertionError: ['/mock-checkout/LayoutTests/platform/qt-mac', '/mock-checkout/LayoutTests/platform/qt-5.0', '/mock-checkout/LayoutTests/platform/qt'] != ['/mock-checkout/LayoutTests/platform/qt-5.0-wk2', '/mock-checkout/LayoutTests/platform/qt-mac', '/mock-checkout/LayoutTests/platform/qt-5.0', '/mock-checkout/LayoutTests/platform/qt']

----------------------------------------------------------------------

[Rafael Brandao]

Created attachment 130868 [details]
Patch

Sorry about the trouble, I didnt know about these tests. Now the expected results are updated.

[WebKit Review Bot]

Comment on attachment 130868 [details]
Patch

Clearing flags on attachment: 130868

Committed r110250: <http://trac.webkit.org/changeset/110250>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because I had to rollout again: http://trac.webkit.org/changeset/110273

The problem was this patch broke Qt5-WK2 tests. 

Before this patch:
-------------------
INFO Baseline search path: qt-5.0-wk2 -> qt-linux -> qt-5.0 -> qt -> generic
INFO Parsing expectations ...
DEBUG Using test_expectations.txt: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt/test_expectations.txt
DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-linux/Skipped
DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/wk2/Skipped
DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt/Skipped
DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-5.0-wk2/Skipped
DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-5.0/Skipped
INFO Preparing tests ...
INFO Found:  28641 tests
INFO Expect: 23150 passes   (23150 now, 0 wontfix)
INFO Expect:   137 failures (  137 now, 0 wontfix)
INFO Expect:     0 flaky    (    0 now, 0 wontfix)
INFO Expect:  5354 skipped  ( 5354 now, 0 wontfix)

After this patch:
------------------
INFO Baseline search path: qt-5.0-wk2 -> qt-5.0 -> qt -> generic
INFO Parsing expectations ...
DEBUG Using test_expectations.txt: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt/test_expectations.txt
DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/wk2/Skipped
DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt/Skipped
DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-5.0-wk2/Skipped
DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-5.0/Skipped
INFO Found:  28641 tests
INFO Expect: 23283 passes   (23283 now, 0 wontfix)
INFO Expect:     0 failures (    0 now, 0 wontfix)
INFO Expect:     0 flaky    (    0 now, 0 wontfix)
INFO Expect:  5358 skipped  ( 5358 now, 0 wontfix)


The problem is that test_expectations.txt (137 tests markes as expected to fail) and qt-linux platform is absolutely ignored (skipped list and baseline search path too) after this patch. I don't know why, but it needs more investigation and more testing, and maybe more unit tests, because it seems the test coverage isn't so good now.

[Dirk Pranke]

Grr. Sorry! I need to be more careful in reviewing these things, I guess.

[Dirk Pranke]

Okay, having stared at this ... the patch is pretty clear that qt-linux gets replaced w/ qt-5.0 (see line 62 in qt.py). If qt-linux was supposed to stay in the baseline search path, then baseline_search_path() will need to be modified as well.

[Rafael Brandao]

(In reply to comment #19)
> Reopen, because I had to rollout again: http://trac.webkit.org/changeset/110273
> 
> The problem was this patch broke Qt5-WK2 tests. 
> 
> Before this patch:
> -------------------
> INFO Baseline search path: qt-5.0-wk2 -> qt-linux -> qt-5.0 -> qt -> generic
> INFO Parsing expectations ...
> DEBUG Using test_expectations.txt: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt/test_expectations.txt
> DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-linux/Skipped
> DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/wk2/Skipped
> DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt/Skipped
> DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-5.0-wk2/Skipped
> DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-5.0/Skipped
> INFO Preparing tests ...
> INFO Found:  28641 tests
> INFO Expect: 23150 passes   (23150 now, 0 wontfix)
> INFO Expect:   137 failures (  137 now, 0 wontfix)
> INFO Expect:     0 flaky    (    0 now, 0 wontfix)
> INFO Expect:  5354 skipped  ( 5354 now, 0 wontfix)
> 
> After this patch:
> ------------------
> INFO Baseline search path: qt-5.0-wk2 -> qt-5.0 -> qt -> generic
> INFO Parsing expectations ...
> DEBUG Using test_expectations.txt: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt/test_expectations.txt
> DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/wk2/Skipped
> DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt/Skipped
> DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-5.0-wk2/Skipped
> DEBUG Using Skipped file: /ramdisk/qt-linux-32-release-webkit2/build/LayoutTests/platform/qt-5.0/Skipped
> INFO Found:  28641 tests
> INFO Expect: 23283 passes   (23283 now, 0 wontfix)
> INFO Expect:     0 failures (    0 now, 0 wontfix)
> INFO Expect:     0 flaky    (    0 now, 0 wontfix)
> INFO Expect:  5358 skipped  ( 5358 now, 0 wontfix)
> 
> 
> The problem is that test_expectations.txt (137 tests markes as expected to fail) and qt-linux platform is absolutely ignored (skipped list and baseline search path too) after this patch. I don't know why, but it needs more investigation and more testing, and maybe more unit tests, because it seems the test coverage isn't so good now.

Ossy, thanks for the info and sorry for the trouble again. I've been checking how the baseline behaves before this patch, and it seems that we don't have qt-5.0-wk1 on baseline search path when we run tests using qt-5.0 and wk1, but I believe I should fix this into a separate patch. Right now I just don't want to change any behavior we have so far.

[Rafael Brandao]

Created attachment 131950 [details]
Patch

I've added test cases to check our coverage in different Qt versions and/or OS (they are in QtPortTest.test_skipped_file_search_paths). Any feedback will be nice.

[Dirk Pranke]

Comment on attachment 131950 [details]
Patch

The code looks fine to me. I'll let one of the QT experts make sure that it does what they want and r+ it ...

[Rafael Brandao]

Ossy would you mind to take a look on this? Just to make sure the bot won't complain this time.

[Rafael Brandao]

Comment on attachment 131950 [details]
Patch

This patch doesnt work anymore, I'm working on a new version of it.

[János Badics]

Created attachment 148088 [details]
Proposed patch

I made some modifications on qt.py and it seems to be working for me. Could someone have a look at it please?

[WebKit Review Bot]

Attachment 148088 [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/Scripts/webkitpy/layout_tests/port/qt.py:126:  missing whitespace after ','  [pep8/E231] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rafael Brandao]

Comment on attachment 148088 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148088&action=review

Are you using Qt 5.0? I believe your patch is incomplete because it will use the platform path qt-5.0-wk1 (or wk2) rather the one you've specified with platform. Sometimes you want the test results made by one specific setup like qt5 wk2 to be dropped into qt-5.0 folder, or qt so it will affect the results for a more generic setting. I believe running for example qt5 wk2, putting platform as qt and using new-baseline won't put generated results on qt folder.

> Tools/ChangeLog:3
> +        [Qt] Fixed an issue with --platform and new-baseline options set.

You should use the same title of the bug.

[Peter Gal]

View in context: https://bugs.webkit.org/attachment.cgi?id=148088&action=review

> Tools/Scripts/webkitpy/layout_tests/port/qt.py:125
> +        if hasattr(self._options, "new_baseline"):

This way if the new_baseline is False you'll add the platform to the search_path. Maybe you should use getattr(..).

[Csaba Osztrogonác]

Comment on attachment 148088 [details]
Proposed patch

We shouldn't change search_paths, we need better way to fix it.

[Rafael Brandao]

Dirk Pranke, maybe you can give some input here. My attempts so far are taking advantage of the auto-detection of the platform, so we can use the proper qt test related classes and then we can handle special platform values. But is this really correct? Should "platform" value be handled as the destination folder for test results/rebaselines? I couldn't figure out yet how we manage to detect we're on qt platform while running the tests even if we specify completely different platforms, but I wonder if for some reason it doesn't work and we end up being forced to pass "--qt" explicitly... what do you think? Do we need a new flag for it?

[Dirk Pranke]

(In reply to comment #32)
> Dirk Pranke, maybe you can give some input here. My attempts so far are taking advantage of the auto-detection of the platform, so we can use the proper qt test related classes and then we can handle special platform values. But is this really correct? Should "platform" value be handled as the destination folder for test results/rebaselines? I couldn't figure out yet how we manage to detect we're on qt platform while running the tests even if we specify completely different platforms, but I wonder if for some reason it doesn't work and we end up being forced to pass "--qt" explicitly... what do you think? Do we need a new flag for it?

This whole situation is sadly complicated, and it's actually changed a fair amount since this bug was first filed and people started working on it.

Now, there are five ways to control where baselines are placed. The first, using --reset-results, updates existing baselines in place. We'll ignore that one.

There are four other classes of locations for baselines, and which location is used where depends on the kind of baseline and the flags in question, but the options are:

1) alongside the test; this is the location used by default for .wav files and non-render-tree text files.

2) in the 'implementation' directory, e.g., 'platform/mac', 'platform/chromium', 'platform/qt'. This is the value that port.baseline_platform_dir() [it's actually a bit unclear to me if the correct value to use for this is qt/chromium or qt/chromium-$OS, but at the moment, it should be 'qt']. This is the location used by default if the result is expected to be platform specific (render trees and pixel results).

3) in the 'version-specific' directory, e.g., 'platform/mac-snowleopard'. In the case of Qt, where you have different versions of Qt but not different operating-system versions, it's not clear to me what the right value to use here should be; you guys can decide which would be most useful, e.g., qt-mac, or qt-mac-4.8. I suppose probably the directory that has the most results is useful. This is the value returned by port.baseline_version_dir(). This value is only every used if you pass --add-platform-exceptions. The default implementation for this is to use the *first* directory in the baseline search path, but if the first directory for you is a WK2-specific dir or a QT-version-dir, you might not want to use this. 

4) somewhere else. This is how you could generate baselines automatically into some other directory; this would be done by using --add-platform-exceptions and --additional-platform-directory. In this case, for, example, if you wanted to put results into qt-4.8, you could pass --add-platform-exceptions --additional-platform-directory=LayoutTests/platform/qt-4.8 (even if qt-4.8 was already in the search path). Note that this has the side effect of picking up qt-4.8 results before anything else when running the test as well.

This whole thing is probably too complicated. I think we're overloading too many functions at once.

Did that help anything?

Maybe a better question to ask is,

1) Do you just want to match whatever ORWT does? Does ORWT support the same set of configurations that NRWT does? If so, we can reverse-engineer what the desired thing to do here would be.

2) Or, is there some other thing you'd actually prefer to have happen?

[Rafael Brandao]

(In reply to comment #33)
> (In reply to comment #32)
> > Dirk Pranke, maybe you can give some input here. My attempts so far are taking advantage of the auto-detection of the platform, so we can use the proper qt test related classes and then we can handle special platform values. But is this really correct? Should "platform" value be handled as the destination folder for test results/rebaselines? I couldn't figure out yet how we manage to detect we're on qt platform while running the tests even if we specify completely different platforms, but I wonder if for some reason it doesn't work and we end up being forced to pass "--qt" explicitly... what do you think? Do we need a new flag for it?
> 
> This whole situation is sadly complicated, and it's actually changed a fair amount since this bug was first filed and people started working on it.
> 
> Now, there are five ways to control where baselines are placed. The first, using --reset-results, updates existing baselines in place. We'll ignore that one.
> 
> There are four other classes of locations for baselines, and which location is used where depends on the kind of baseline and the flags in question, but the options are:
> 
> 1) alongside the test; this is the location used by default for .wav files and non-render-tree text files.
> 
> 2) in the 'implementation' directory, e.g., 'platform/mac', 'platform/chromium', 'platform/qt'. This is the value that port.baseline_platform_dir() [it's actually a bit unclear to me if the correct value to use for this is qt/chromium or qt/chromium-$OS, but at the moment, it should be 'qt']. This is the location used by default if the result is expected to be platform specific (render trees and pixel results).
> 
> 3) in the 'version-specific' directory, e.g., 'platform/mac-snowleopard'. In the case of Qt, where you have different versions of Qt but not different operating-system versions, it's not clear to me what the right value to use here should be; you guys can decide which would be most useful, e.g., qt-mac, or qt-mac-4.8. I suppose probably the directory that has the most results is useful. This is the value returned by port.baseline_version_dir(). This value is only every used if you pass --add-platform-exceptions. The default implementation for this is to use the *first* directory in the baseline search path, but if the first directory for you is a WK2-specific dir or a QT-version-dir, you might not want to use this. 
> 
> 4) somewhere else. This is how you could generate baselines automatically into some other directory; this would be done by using --add-platform-exceptions and --additional-platform-directory. In this case, for, example, if you wanted to put results into qt-4.8, you could pass --add-platform-exceptions --additional-platform-directory=LayoutTests/platform/qt-4.8 (even if qt-4.8 was already in the search path). Note that this has the side effect of picking up qt-4.8 results before anything else when running the test as well.

This is probably what I was looking for, I guess. If we use 'new-baseline', does it matters the result we pick? We're generating new results to replace, isn't? If not, then I would want something without this side effect.

> 
> This whole thing is probably too complicated. I think we're overloading too many functions at once.

I agree. Part of this over complicated situation is our fault, we have far too many options on our port and a not very clear consensus of how the platform paths should be named or results should be placed.

> 
> Did that help anything?

Sure, thanks.

> 
> Maybe a better question to ask is,
> 
> 1) Do you just want to match whatever ORWT does? Does ORWT support the same set of configurations that NRWT does? If so, we can reverse-engineer what the desired thing to do here would be.

I've talked to Ossy a long time ago, and I was asking him if there was a way to put results of tests in a particular folder: running with qt5 wk2 but putting the results on qt-4.8 for example. The intention is to generate those results and then afterwards run the tests using qt-4.8 and checking which test results differs from qt-5.0. Then he pointed me this combo, so maybe it was working on ORWT, but I'm not sure. Ossy could you give some input?

> 
> 2) Or, is there some other thing you'd actually prefer to have happen?

I think you've pointed out the solution already. I believe this bug should be marked as invalid then. :-)

[Dirk Pranke]

(In reply to comment #34)
> > 
> > 2) Or, is there some other thing you'd actually prefer to have happen?
> 
> I think you've pointed out the solution already. I believe this bug should be marked as invalid then. :-)

Having to use --additional-platform-directory for this (and having the side effect that it also messes with the search path) is kind of kludgy. 

It sounds like maybe we need a "--save-baselines-under=X" flag instead.

[Rafael Brandao]

(In reply to comment #35)
> (In reply to comment #34)
> > > 
> > > 2) Or, is there some other thing you'd actually prefer to have happen?
> > 
> > I think you've pointed out the solution already. I believe this bug should be marked as invalid then. :-)
> 
> Having to use --additional-platform-directory for this (and having the side effect that it also messes with the search path) is kind of kludgy. 
> 
> It sounds like maybe we need a "--save-baselines-under=X" flag instead.

That would be perfect. We can safely close this bug then but also create one under "Polish NRWT until it shines.". I've added the tips we've learned here on https://trac.webkit.org/wiki/NewRunWebKitTests.