Bug 72489

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.

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.

Thank you the help!

Created attachment 116964 [details] proposed fix Firstly I modified the qt.py then I had to modify run-webkit-tests file.

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.

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

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.

Comment on attachment 116964 [details] proposed fix Clearing flags on attachment: 116964 Committed r101451: <http://trac.webkit.org/changeset/101451>

All reviewed patches have been landed. Closing bug.

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

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

Created attachment 127361 [details] Patch

Comment on attachment 127361 [details] Patch Clearing flags on attachment: 127361 Committed r110115: <http://trac.webkit.org/changeset/110115>

All reviewed patches have been landed. Closing bug.

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'] ----------------------------------------------------------------------

Created attachment 130868 [details] Patch Sorry about the trouble, I didnt know about these tests. Now the expected results are updated.

Comment on attachment 130868 [details] Patch Clearing flags on attachment: 130868 Committed r110250: <http://trac.webkit.org/changeset/110250>

All reviewed patches have been landed. Closing bug.

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.

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

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.

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

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.

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

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

Comment on attachment 131950 [details] Patch This patch doesnt work anymore, I'm working on a new version of it.

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?

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.

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.

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

Comment on attachment 148088 [details] Proposed patch We shouldn't change search_paths, we need better way to fix it.

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?

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

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

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

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