Bug 117831 – Develop rebase info tool for EWS

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[János Badics]

When a patch that affects test behavior on a specific platform is uploaded, rebasing is needed on other platforms as well. In most of the cases, this does not happen on uploading, and the bots stays red for a long time, or some discussion with the author is needed.

This tool would get the list of modified tests on the EWS phase after uploading the patch, and notify the developers about ports/tests that would need some revision in a Bugzilla comment. In this way, the author (and the gardeners) can see what tests would need additional expected files, immediately.
Another improvement is to run this rebase automatically and upload this rebase patch to the bug, or maybe unifying the rebase patch with the original one.

[János Badics]

Created attachment 205078 [details]
working progress patch

At the current state, EWS gets the list of new expected files in a given patch for a given port (e.g. Qt). Then, after applying the patch, it runs these tests on its own port (e.g. GTK) and the results/diffs can be found in the layout-test-results directory.

A further goal would be to use these results/diffs to create an instant rebase patch, or maybe to unify it with the original patch.

[Ryosuke Niwa]

Comment on attachment 205078 [details]
working progress patch

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

Thanks for working on this feature but I think some methods and variable aren't named properly.

> Tools/Scripts/webkitpy/tool/bot/earlywarningsystemtask.py:61
> +            return self._run_rebased_tests()

I don't think run_rebased_tests makes sense as "rebased" is an adjective.
I would call it regenerate_expected_results instead.

> Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:55
> +    def _get_test_name_stub(self, expected):

We don't normally use get_ prefixes.

> Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:61
> +        match = re.match(os.path.join('LayoutTests', 'platform', '(.+?)', '(.*)'), absdir)

Why don't we use port.baseline_platform_dir or port.baseline_version_dir instead?

> Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:70
> +    def _get_tests_by_expected(self, port, expected):

I don't understand why this function exits at all. Why can't the caller just call _get_tests_in_dir?
Also, I don't understand why the third argument is called expected.

> Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:83
> +    def _get_first_generic_by_filename(self, port, filename):
> +        paths = self._get_tests_by_expected(port, filename)
> +        filestub = self._get_test_name_stub(filename)
> +        ret = None
> +        for path in paths:
> +            hit = re.match('.*' + filestub + '\..*', path)
> +            if hit:
> +                ret = hit.group(0)
> +                break
> +        return ret

What is this function trying to do?

[Csaba Osztrogonác]

Comment on attachment 205078 [details]
working progress patch

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

>> Tools/Scripts/webkitpy/tool/bot/earlywarningsystemtask.py:61
>> +        if self._delegate.run_tests:
>> +            return self._run_rebased_tests()
> 
> I don't think run_rebased_tests makes sense as "rebased" is an adjective.
> I would call it regenerate_expected_results instead.

I would make this change separated to the new "RunTestsFromPatch" command.
Additionally EWS bots should have three different option in ews.json:
- don't run tests at all (now: GTK, GTK-WK2, EFL, EFL-WK2, Qt, Qt-WK2)
- run only touched tests to help rebasing tasks (if they have limited hw resources)
- run all tests (if they have unlimited hw resources) (now: Win, Mac, Mac-WK2)

> Tools/Scripts/webkitpy/tool/commands/download.py:266
> +    help_text = "Get the list of rebased patches from patch and make new baselines for own"

I prefer the name run-tests-touched-by-attachment and help text:
"Run only tests touched by the given attachment"

> Tools/Scripts/webkitpy/tool/steps/forcetests.py:47
> +class ForceTests(AbstractStep):
> +    @classmethod
> +    def options(cls):
> +        return AbstractStep.options()
> +
> +    def run(self, state):
> +        self._options.test = True
> +        self._options.layout_testlist_only = True
> +        self._options.non_interactive = True

I don't like the idea to have a new ForceTests class to make
RunTestsFromPatch command really run tests independently of
--test command line option. Additionally it still doesn't 
build the attachment before trying to run the tests.

There are similar strangeness with many webkit-patch commands, for example:
- build-and-test doesn't build and doesn't test ... only if --build and --test command line options are passed
- build-attachment doesn't build ... only if --build passed
...

I prefer if AbstractPatchSequencingCommand would have 
_prepare_state function similar to AbstractSequencedCommand.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:113
> +        if state["testlist"]:
> +            args.extend(state["testlist"])

What if the testlist is empty? In this case all tests 
would run, but we don't want to run any layout test.

[Csaba Osztrogonác]

I think we should change this bug to a meta bug with depending bugs like:
- [webkitpy] AbstractPatchSequencingCommand should have _prepare_state
- [webkitpy] Add run-tests-touched-by-attachment command
- Let EWS bots run only touched tests
- webkit-patch needs a apply-ews-results-from-bug command - bug92033

[Csaba Osztrogonác]

(In reply to comment #4)
> - [webkitpy] AbstractPatchSequencingCommand should have _prepare_state
new bug with proposed patch: https://bugs.webkit.org/show_bug.cgi?id=118128

[János Badics]

(In reply to comment #2)
> 
> > Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:61
> > +        match = re.match(os.path.join('LayoutTests', 'platform', '(.+?)', '(.*)'), absdir)
> 
> Why don't we use port.baseline_platform_dir or port.baseline_version_dir instead?

The goal of this function is to return the generic directory path without the LayoutTests prefix. So, if the last parameter is  LayoutTests/platform/efl/css1/basic/containment-expected.txt, then match is (u'efl', u'css1/basic'). This function returns css1/basic in this case. However, port.baseline_platform_dir returns /path/to/WebKit/repository/LayoutTests/platform/qt, if the script runs on a Qt EWS. But I can't use it to get css1/basic from 'LayoutTests/platform/efl/css1/basic/containment-expected.txt'.

> 
> > Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:83
> > +    def _get_first_generic_by_filename(self, port, filename):
> > +        paths = self._get_tests_by_expected(port, filename)
> > +        filestub = self._get_test_name_stub(filename)
> > +        ret = None
> > +        for path in paths:
> > +            hit = re.match('.*' + filestub + '\..*', path)
> > +            if hit:
> > +                ret = hit.group(0)
> > +                break
> > +        return ret
> 
> What is this function trying to do?

According to the description at https://bugs.webkit.org/show_bug.cgi?id=118293 :
_test_relative_dir() determines the relative generic directory path for the given test, e.g.
LayoutTests/platform/qt-5.0-wk1/editing/deleting/delete-cell-contents-expected.txt ->
editing/deleting/
Then _tests_in_dir gets the paths of all the tests found in this directory. As a last step, the loop in _first_generic_by_filename() gets the first match to the given file name.

I am sure it can be solved in a more elegant way; currently this is the only way I can do it.

[Csaba Osztrogonác]

no interest in it anymore