Swap fallback order to search LayoutTests directory

Swap fallback order to search LayoutTests directory

Some old trybots have invalid WebKit/LayoutTests directory
which is not up to date. If the query function found the
invalid old directory, all layout tests fail as a result.

Also Some developers have an old LayoutTests directory because
they follow a wiki instruction and disable syncing it for speed-up.

Essentially, fallback order is not a problem, but reversed order
must be safer than current order and save developers from
in stuck of useless trouble.

BUG=107836, 105104
TEST=browser_tests; ui_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129815

[Takashi Toyoshima, 2012-01-12 08:43:06]

Hi

This CL is a minor change of CL http://codereview.chromium.org/8641002
you guys reviewed.
After the change, I notice that some old trybots have obsoleted
WebKit/LayoutTests directory. I think reversed search order will be safe
and easy way to avoid this issue.

Thanks,

[Paweł Hajdan Jr., 2012-01-12 09:54:53]

And how about the correct way? ;-)

If the directory is stale and incorrect, how about getting it cleaned/removed?
I'm pretty sure M-A (maruel) has some ideas how to do it.

[Takashi Toyoshima, 2012-01-13 10:06:54]

OK, I'll ask maruel about this issue.

Thanks

[Takashi Toyoshima, 2012-01-18 02:05:43]

Close this CL because maruel@ removed invalid WebKit/LayoutTests directory in
all try slaves.

[Takashi Toyoshima, 2012-03-26 07:31:26]

Hi Paweł,

I'd like to revive this CL.
Actually, we already resolved this issue by removing invalid files in try bots.
But, some developers may be in stuck of this issue.

Essentially, the fallback order is not a problem. But, reversed order must
reduce chance of this kind of trouble. Now I feel this CL is worth to be
submitted. It never make things worse than now.

FYI; jam reported this in
http://code.google.com/p/chromium/issues/detail?id=105104

[Paweł Hajdan Jr., 2012-03-26 16:22:14]

I'm still pretty sure this should _not_ be done.

Here's why: now those devs have a problem that is _hidden_ on their systems,
which means it can't be reproduced with a fresh checkout, and with a sane
checkout that everyone else has. Such a confusion would be even worse.

We need to fix the dev setups too. I see two possible choices:

1. There was some script like diagnose-me.py or similar, and the fixing logic
could be added to it.

2. gclient hook

How does that sound to you?

[Takashi Toyoshima, 2012-03-28 07:11:22]

Your proposal is considerable, but looks a little overkill to me. Why you want
to hold current wrong fallback order? When I submitted original code, this order
looks making no difference, then I decided current order by chance. But now it
appears that the reversed one is better. The reason is very simple. No one has
invalid test/data/layout_tests/LayoutTests, but someone have invalid
third_party/WebKit/LayoutTests. If this change is not enough, you can apply your
approach later. But I think there is no almighty checker because someone want to
have invalid WebKit/LayoutTests for their productivity and they are not
minority. That's another reason why I hesitate to introduce such kind of
checker.

[Paweł Hajdan Jr., 2012-03-28 16:08:26]

On 2012/03/28 07:11:22, toyoshim wrote:
> Your proposal is considerable, but looks a little overkill to me. Why you want
> to hold current wrong fallback order? When I submitted original code, this
order
> looks making no difference, then I decided current order by chance.

No it's not. First we test the new location, and then fall back to the old one.

> But now it
> appears that the reversed one is better. The reason is very simple. No one has
> invalid test/data/layout_tests/LayoutTests, but someone have invalid
> third_party/WebKit/LayoutTests.

That's bad. Reversing the order will hide the issue, leading to the same
problem, later. Now at least we know what's going on. Later it's likely to be
harder to debug, and overlap with other issues.

> If this change is not enough, you can apply your
> approach later.

After hiding the problem that way there will be no incentive to fix it.

> But I think there is no almighty checker because someone want to
> have invalid WebKit/LayoutTests for their productivity and they are not
> minority. That's another reason why I hesitate to introduce such kind of
> checker.

I don't get it how having something broken can help productivity.

Sorry, maybe I just don't understand all of the context here, but please also
try to focus on doing it right, not committing the first thing that seems to
work at this moment.

[Takashi Toyoshima, 2012-03-29 02:54:08]

http://code.google.com/p/chromium/wiki/UsingNewGit#Initial_checkout

This is the reason why I can't rely on third_party/WebKit/LayoutTests.
The official instruction proposes to omit LayoutTests synchronization for speed
up.

No one has invalid test/data/layout_tests because it exists only when developer
uses svn work flow. And there is no instruction to omit its synchronization. So
it's always fresh.

[jam, 2012-03-30 05:25:04]

On 2012/03/29 02:54:08, toyoshim wrote:
> http://code.google.com/p/chromium/wiki/UsingNewGit#Initial_checkout
> 
> This is the reason why I can't rely on third_party/WebKit/LayoutTests.
> The official instruction proposes to omit LayoutTests synchronization for
speed
> up.
> 
> No one has invalid test/data/layout_tests because it exists only when
developer
> uses svn work flow. And there is no instruction to omit its synchronization.
So
> it's always fresh.

lgtm

I completely agree we should do this. This isn't a question of masking an error
at all; phrasing it that way ignores the fact that right now on some (many?)
developer machines the tests will fail. This is very hard to track down. Having
a python script to detect this is not useful, because if someone knows the
problem (i.e. to run this) then they would know to delete the directory. Not
doing this means wasting more developer time trying to track this down.

[commit-bot: I haz the power, 2012-03-30 05:40:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/9186033/15002

[commit-bot: I haz the power, 2012-03-30 09:45:44]

Change committed as 129815

[M-A Ruel, 2012-06-20 19:42:58]

http://codereview.chromium.org/9186033/diff/15002/content/common/content_path...
File content/common/content_paths.cc (right):

http://codereview.chromium.org/9186033/diff/15002/content/common/content_path...
content/common/content_paths.cc:45: cur =
cur.Append(FILE_PATH_LITERAL("LayoutTests"));
It's been 2 months and content/test/data/layout_tests/LayoutTests still doesn't
exist. Is that expected?

[jabdelmalek, 2012-06-20 19:55:33]

http://codereview.chromium.org/9186033/diff/15002/content/common/content_path...
File content/common/content_paths.cc (right):

http://codereview.chromium.org/9186033/diff/15002/content/common/content_path...
content/common/content_paths.cc:45: cur =
cur.Append(FILE_PATH_LITERAL("LayoutTests"));
On 2012/06/20 19:42:59, Marc-Antoine Ruel wrote:
> It's been 2 months and content/test/data/layout_tests/LayoutTests still
doesn't
> exist. Is that expected?

It would be nice to check these into content/test/data instead of
chrome/test/data for consistency (since tests in content should get their data
from content/test/data). It just needs someone to update the DEPS file and use
content_test_data in path service. That is orthogonal from this change though.