Bug 56233 – Add --baseline-search-path to NRWT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mihai Parparita]

Add --baseline-search-path to NRWT

[Mihai Parparita]

Created attachment 85554 [details]
Patch

[Tony Chang]

This is for expectations that aren't going to be checked in?  Why can't we check in the baselines?

[Dirk Pranke]

We could check them in, but (a) we expect them to churn pretty frequently and (b) there will probably be a bunch of them, which could make the tree pretty confusing.

[Dirk Pranke]

The downside to not checking them in is that we'll need some other way to figure out what the references should be and how to regenerate the baselines. Mihai is working on that separately.

[Tony Chang]

Maybe we can use gclient to pull in the other baselines so it doesn't make the webkit tree noisy?

[Mihai Parparita]

(In reply to comment #5)
> Maybe we can use gclient to pull in the other baselines so it doesn't make the webkit tree noisy?

There's going to be enough of these baselines (looks like 100/bot and around 10 bots) that I don't think we can manage them as carefully (i.e. version control). The expectation is that when the hardware bots go red, the GPU gardener will do some spot checks to make sure that the changes are expected and then run the webkit tests with --reset-results, which will update these local baselines.

[Tony Chang]

(In reply to comment #6)
> (In reply to comment #5)
> > Maybe we can use gclient to pull in the other baselines so it doesn't make the webkit tree noisy?
> 
> There's going to be enough of these baselines (looks like 100/bot and around 10 bots) that I don't think we can manage them as carefully (i.e. version control). The expectation is that when the hardware bots go red, the GPU gardener will do some spot checks to make sure that the changes are expected and then run the webkit tests with --reset-results, which will update these local baselines.

Can we keep the local baselines in the same place as the other baselines?  E.g., LayoutTests/platform/chromium-gpu-win-foo where foo is part of --platform or some other system check?  We could even svn-ignore the directories so they don't show up.  I'd be nice to avoid adding another command line parameter.

Also, I would at least use a local git repo for each bot's results.  Having the history of changes will probably be useful at some point.  You could write a wrapper script that runs NRWT --reset-results, shows the diffs, and commits the results to a local git repo.

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Maybe we can use gclient to pull in the other baselines so it doesn't make the webkit tree noisy?
> > 
> > There's going to be enough of these baselines (looks like 100/bot and around 10 bots) that I don't think we can manage them as carefully (i.e. version control). The expectation is that when the hardware bots go red, the GPU gardener will do some spot checks to make sure that the changes are expected and then run the webkit tests with --reset-results, which will update these local baselines.
> 
> Can we keep the local baselines in the same place as the other baselines?  E.g., LayoutTests/platform/chromium-gpu-win-foo where foo is part of --platform or some other system check?  We could even svn-ignore the directories so they don't show up.  I'd be nice to avoid adding another command line parameter.
> 

I think the platform field and the baseline logic is complicated enough as it is. 

Also, having it have to know how to parse out some sort of '-local' substring feels wrong to me, since it is not like any of the other modifier components.

[Tony Chang]

(In reply to comment #8)
> I think the platform field and the baseline logic is complicated enough as it is. 
> 
> Also, having it have to know how to parse out some sort of '-local' substring feels wrong to me, since it is not like any of the other modifier components.

I was thinking it would be descriptive rather than just '-local'.  E.g., chromium-gpu-nvidiaXXXX-win or something.  It seems like a small code change to pass in --platform chromium-gpu-nvidiaXXXX and have it get the right results dir.
Alternately, you could add a separate flag (e.g., --gpu-chipset), but that seems less like the other flags.

If all that still sounds worse than --baseline-search-path, I would probably make it at least be a relative path to tempfile.gettempdir() so it's easier to set across platforms (that way on windows you don't have to worry about if it's cygwin or not).

[Mihai Parparita]

Another use-case that occurred me is wanting to temporarily override the fallback path. For example, there are times where I'd like to know if the a test would pass with the mac/ baselines, even though it has chromium-mac/ ones (right now I have to edit chromium_mac.py and temporarily change the fallback path order).

For that to work, we can't force it to always be relative to the tempdir. I think the buildbot script that we use on the (GPU) bots (that wraps NRWT) can set the flag using tempfile.gettempdir() though.

[Tony Chang]

Comment on attachment 85554 [details]
Patch

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

I think this is suboptimal, but I guess we can try it.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:276
> +        baseline_search_path = map(self._filesystem.normpath, self.get_option('baseline_search_path', '').split(':'))

: seems like a poor choice for a delimiter since windows drive letters have : in them.

> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:267
> +            [('/tmp/local-baselines', 'fast/test-expected.txt')])

Can you add some tests for windows paths and for relative paths?

[Dirk Pranke]

Comment on attachment 85554 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:276
>> +        baseline_search_path = map(self._filesystem.normpath, self.get_option('baseline_search_path', '').split(':'))
> 
> : seems like a poor choice for a delimiter since windows drive letters have : in them.

You might be better off declaring the argument as action="append" and specifying it multiple times on the command line, so that it is stored as a list, and not have to use a separator here. Then again, I don't know where you're getting this value from to pass in on the command line.

The only other case I can think of where we do multiple-valued lists on the command line is in the --print option, which uses a comma-separated list of values. I'd rather not have commas for one argument and colons or semicolons for another. I would also rather not change --print to have to be specified multiple times.

>> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:267
>> +            [('/tmp/local-baselines', 'fast/test-expected.txt')])
> 
> Can you add some tests for windows paths and for relative paths?

Because the test is using a MockFileSystem(), it can (and has to) assume hard-coded forward separators, so these tests will actually work across both unix and win32. 

I don't know if there's an easy way to do tests for windows paths and relative paths that will work very well without using a real filesystem. The only other place in this file where we've had to worry about this is in test_filename_to_uri, where it was wrapped in an if block conditional on sys.platform. But that could be cumbersome here. I'm a bit on the fence as to whether writing these tests will be worth the effort needed to ensure they're right (i.e., it's more likely that the tests will be buggy than the code they're testing).

[Mihai Parparita]

Created attachment 88289 [details]
Patch

[Mihai Parparita]

(In reply to comment #12)
> You might be better off declaring the argument as action="append" and specifying it multiple times on the command line, so that it is stored as a list, and not have to use a separator here. Then again, I don't know where you're getting this value from to pass in on the command line.

Changed to append, that seems cleaner.

> I don't know if there's an easy way to do tests for windows paths and relative paths that will work very well without using a real filesystem. The only other place in this file where we've had to worry about this is in test_filename_to_uri, where it was wrapped in an if block conditional on sys.platform. But that could be cumbersome here. I'm a bit on the fence as to whether writing these tests will be worth the effort needed to ensure they're right (i.e., it's more likely that the tests will be buggy than the code they're testing).

I decided that relative paths are probably not what the user wants, so those will not be used. I moved the checking for that and normalization to _set_up_derived_options.

[Tony Chang]

Comment on attachment 88289 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:304
> +                 "Specify multiple times to add multiple search path entries."),

Clever

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:582
> +        (res, buildbot_output, regular_output, user) = logging_run(
> +             ['--baseline-search-path', 'foo'])

Nit: no () needed on the left hand side of the assignment

[Mihai Parparita]

(In reply to comment #15)
> Nit: no () needed on the left hand side of the assignment

Fixed (elsewhere in the file too)

[Mihai Parparita]

Committed r82972: <http://trac.webkit.org/changeset/82972>

[Dirk Pranke]

LGTM as well.