Bug 91089 – [NRWT] runs platform specific tests that it shouldn't with --force

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Balazs Kelemen]

I'm using this with a Qt build (Qt 5):
Tools/Scripts/run-webkit-tests --no-new-test-results --pixel-tests -2 --new-baseline --force LayoutTests/compositing/

And I get results like this:
Writing new expected result "platform/qt-5.0-wk2/platform/chromium/compositing/3d-corners-expected.txt

Before that, I got the results for the generic tests in compositing as expected, but than it starts to run tests from platform/chromium/compositing. This seems to be a bug in platform detection.

Using --additional-platform-directory=my/wk/path/LayoutTests/platform/qt-5.0-wk2 as Ossy adviced to me does not help.

[Balazs Kelemen]

Hm, that's even worst, it's not just an issue with --new-baseline but generally if I pass LayoutTests/compositing nrwt runs tests from LayoutTests/platform/chromium/compositing.

[Dirk Pranke]

I think this was probably introduced when I started running the platform-specific additions to the generic directories in bug 37956. Will take a look.

[Dirk Pranke]

Ah, I see what's going on .

The way things are implemented in NRWT, we find *all* of the tests in the tree, and then add SKIP expectations for the tests we don't want to run (as returned by Port.skipped_layout_tests). That means that there is a SKIP expectation for platform/chromium.

However, you're running with --force, which says to run all tests, even if they're skipped :).

I'm not sure if I consider this a bug so much as an unfortunate interaction of the three features (finding platform-specific versions of tests for a generic directory, implicitly skipping platforms other than your own, and --force).

I think you can work around this by -i/--ignore-tests platform/chromium, but --force might actually trump everything, and I'm not sure that that's bad.

Thoughts?

[Balazs Kelemen]

I see, I agree it's actually not a bug. It seems the right behavior of  --force to run these tests. However, when using --force to check what can be removed from skip list, this behavior is a bit uncomfortable. I don't think ports want to run tests from LayoutTests/platform other than the ones that belongs to them. Yesterday I made this tiny change locally:

diff --git a/Tools/Scripts/webkitpy/layout_tests/port/base.py b/Tools/Scripts/webkitpy/layout_tests/port/base.py
index fbf0b93..3d28dbc 100755
--- a/Tools/Scripts/webkitpy/layout_tests/port/base.py
+++ b/Tools/Scripts/webkitpy/layout_tests/port/base.py
@@ -506,7 +506,7 @@ class Port(object):
             expanded_paths.append(path)
             if self.test_isdir(path) and not path.startswith('platform'):
                 for platform_dir in all_platform_dirs:
-                    if fs.isdir(fs.join(platform_dir, path)):
+                    if fs.isdir(fs.join(platform_dir, path)) and platform_dir in self.baseline_search_path():
                         expanded_paths.append(self.relative_test_filename(fs.join(platform_dir, path)))
 
         return expanded_paths


With this change I believe we are still be able to run other platform's tests with --additional-platform-directory. So, the question is whether it is worth to mess up the semantic of --force a bit in favor of easier skip list maintenance. I'm curious about other's opinion.

[Dirk Pranke]

Personally I find the "run the platform-specific versions of the generic directory, too" feature a little goofy, but then that's probably because I spend my time hacking webkitpy and not webkit :).

Your suggested change had occurred to me as well; it doesn't actually change what --force does, and I'm fine with it, so if we want to make that change, great! I agree that people aren't likely to complain, since if you really wanted to run platform/chromium/compositing all you'd have to do would be to add it on the command line (or use --additional-platform-dir, as you say).

[Eric Seidel (no email)]

We should just get rid of these tests in platform/  Move them to nicely named direcxtories and skip them everywhere they're not needed.  having tests in platform/ is a very old solution to this problem, which I think causes more confusion than help.

[Dirk Pranke]

I'm inclined to WONTFIX this ... any objections?

[Balazs Kelemen]

(In reply to comment #7)
> I'm inclined to WONTFIX this ... any objections?

I would like to see this fixed. I couldn't get sort of it yet. Btw, do we have a consensus that we should remove tests from platform directories? It's much more comfortable to add a test to a platform directory than skipping on every other platform, although it is a questionable behavior to add a test for only one platform.

[Dirk Pranke]

(In reply to comment #8)
> (In reply to comment #7)
> > I'm inclined to WONTFIX this ... any objections?
> 
> I would like to see this fixed. I couldn't get sort of it yet. Btw, do we have a consensus that we should remove tests from platform directories? It's much more comfortable to add a test to a platform directory than skipping on every other platform, although it is a questionable behavior to add a test for only one platform.

Whoops, sorry, I forgot abou the change you suggested in comment #4. I'm still fine with that, if you want to make it.

As to getting rid of platform directories, I would leave that for a separate discussion.

[Dirk Pranke]

Oh, hmm, I thought of something I don't like about your suggestion. The way things are currently implemented, tests() returns the same list independent of the port; that way the statistics we report for total # of files is the same across ports, and just the # of tests that skip varies, and so changing that seems less good. In particular, when you have multiple variants of the same port (e.g., chromium, or qt), it's nice to be able to compare how many tests are being skipped on each variant.

Although, re-thinking it, this isn't actually quite true with the addition of virtual test suite, since ports with different lists of virtual suites will have different numbers of tests. Currently, the only port that uses virtual tests is chromium, but at least there all of the variants use the same list of suites, so at least I keep the last sentence true.

So, I still have a slight preference for leaving things the way they are, I guess. But if you'd strongly prefer to make the change, I won't be that upset by it.

[Balazs Kelemen]

(In reply to comment #10)
> Oh, hmm, I thought of something I don't like about your suggestion. The way things are currently implemented, tests() returns the same list independent of the port; that way the statistics we report for total # of files is the same across ports, and just the # of tests that skip varies, and so changing that seems less good. In particular, when you have multiple variants of the same port (e.g., chromium, or qt), it's nice to be able to compare how many tests are being skipped on each variant.
> 
> Although, re-thinking it, this isn't actually quite true with the addition of virtual test suite, since ports with different lists of virtual suites will have different numbers of tests. Currently, the only port that uses virtual tests is chromium, but at least there all of the variants use the same list of suites, so at least I keep the last sentence true.
> 
> So, I still have a slight preference for leaving things the way they are, I guess. But if you'd strongly prefer to make the change, I won't be that upset by it.

Ok, than I think we should get rid of platform tests instead.

[Dirk Pranke]

(In reply to comment #11)
> (In reply to comment #10)
> > Oh, hmm, I thought of something I don't like about your suggestion. The way things are currently implemented, tests() returns the same list independent of the port; that way the statistics we report for total # of files is the same across ports, and just the # of tests that skip varies, and so changing that seems less good. In particular, when you have multiple variants of the same port (e.g., chromium, or qt), it's nice to be able to compare how many tests are being skipped on each variant.
> > 
> > Although, re-thinking it, this isn't actually quite true with the addition of virtual test suite, since ports with different lists of virtual suites will have different numbers of tests. Currently, the only port that uses virtual tests is chromium, but at least there all of the variants use the same list of suites, so at least I keep the last sentence true.
> > 
> > So, I still have a slight preference for leaving things the way they are, I guess. But if you'd strongly prefer to make the change, I won't be that upset by it.
> 
> Ok, than I think we should get rid of platform tests instead.

I actually like the platform tests concept, and it feels ugly to have to explicitly list in files tests that should be Skipped by definition (even though that's what Chromium has historically done). 

If WONTFIXing this isn't acceptable, I'd prefer making the change in comment #4 to having to explicitly move the platform-specific tests into a "not-platform dir but really it's a platform dir" and then skip things. That feels like a bad workaround.

[Balazs Kelemen]

 > I actually like the platform tests concept, and it feels ugly to have to explicitly list in files tests that should be Skipped by definition (even though that's what Chromium has historically done). 
> 
> If WONTFIXing this isn't acceptable, I'd prefer making the change in comment #4 to having to explicitly move the platform-specific tests into a "not-platform dir but really it's a platform dir" and then skip things. That feels like a bad workaround.

It's the matter of how important it is to make using --force comfortable. I would like to hear other's opinion, so Ossy, Jeez, please share your opinion. I will ping you tomorrow anyway if you don't :)

[Csaba Osztrogonác]

(In reply to comment #7)
> I'm inclined to WONTFIX this ... any objections?
Yes. This is a valid and annoying bug.

Qt platform never should run tests under platform/chromium, platform/mac, ... directory. We don't want to run them with and without --force. 

I think handling of tests_for_other_platforms() is absolutely incorrect. 
These tests shouldn't have SKIP expectations, they shouldn't exist for
other ports at all.

[Dirk Pranke]

sigh ...

thinking about this again, I realized I forgot that we were talking about which platform directories to run *when you specify a generic directory*. So, this won't affect the stats when running all the tests.

I am totally fine with rwt --qt compositing not running platform/chromium/compositing.

Let 's do this :). Sorry for the back-and-forth.

[Balazs Kelemen]

Created attachment 156670 [details]
Patch

[Dirk Pranke]

Comment on attachment 156670 [details]
Patch

thanks! since this is a functional change in how the tool works, it would be nice to have a test for it in run_webkit_tests_integrationtest.py; can you add a fixme to this or a stubbed out test, at least?

[Balazs Kelemen]

(In reply to comment #17)
> (From update of attachment 156670 [details])
> thanks! since this is a functional change in how the tool works, it would be nice to have a test for it in run_webkit_tests_integrationtest.py; can you add a fixme to this or a stubbed out test, at least?

No need to add a new test because it already regress run_webkit_tests_integrationtest.MainTest.test_platform_tests_are_found :)
I will look into fixing it.

[Balazs Kelemen]

Created attachment 156718 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156718 [details]
Patch

Attachment 156718 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13439681

[Dirk Pranke]

Comment on attachment 156718 [details]
Patch

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

r+ w/ nits. Feel free to land if you can figure out why the  CQ is failing ...

> Tools/Scripts/webkitpy/layout_tests/port/test.py:225
>      tests.add('platform/test-snow-leopard/websocket/test.html')

hm ... this is probably a bug, the baseline_search_path() checks for test-mac-snowleopard but here we have test-snow-leopard. I should fix that.

It might be clearer if we added a test/test-win-win7/http/test.html, since win-win7 shouldn't be in the path for either mac-leopard or mac-snowleopard.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:899
> +        # FIXME: we should not rely on knowleadge about the default port.

If you changed line 897 to  get_tests_run(['--platform', 'test-mac-leopard', 'http'], ...) you can get rid of the FIXME.

[Balazs Kelemen]

Landed in http://trac.webkit.org/changeset/124862.