[Webkit-unassigned] [Bug 186359] Add script to update web-platform-tests expectations after import

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 11 13:15:05 PDT 2018


--- Comment #8 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 342430
  --> https://bugs.webkit.org/attachment.cgi?id=342430

I think this is going in the right direction.
I think the most important part might be to update the script so that we can run unit test.
This might require adding some abstraction to how we run webkit test or get results from it as we would probably do a shim in unit test.

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:-460
> -            self._filesystem.remove(results_json_path)

Is this change actually needed?

> Tools/Scripts/webkitpy/w3c/test_updater.py:32
> + This run-webkit-tests and analyzes the results then it attempts to update the

s/This /This runs /

> Tools/Scripts/webkitpy/w3c/test_updater.py:33
> + -expected.txt or the root TestExpecations file for failing test. This script is

s/TestExpecations/TestExpectations/ here and elsewhere

> Tools/Scripts/webkitpy/w3c/test_updater.py:100
> +# Add documentation of argorithm for updating test expectations


> Tools/Scripts/webkitpy/w3c/test_updater.py:102
> +# add unittests

We need some unit tests before landing this script.
Maybe there could be a way to pass a run_webkit_test caller as part of the creation of TestExpectationUpdater.
Then, we could try to get the results of the updated files, such as TestExpectations.

> Tools/Scripts/webkitpy/w3c/test_updater.py:255
> +    def _failing_testharness_test(self, test):

Could be renamed to _update_failing_testharness_test_expectation to improve readability or more generic _handle_failing_testharness_test.
Ditto for above methods, having a verb helps since we are changing some files there.

> Tools/Scripts/webkitpy/w3c/test_updater.py:263
> +        args = ['Tools/Scripts/run-webkit-tests'] + self._option_args

I wonder whether we should try to call python directly if possible since run-webkit-tests is already python.
If this is not straightforward, please add a FIXME.

> Tools/Scripts/webkitpy/w3c/test_updater.py:319
> +            myfile.write('\n%s [ %s ]\n' % (test, expectation))

We might want to use filesystem.py here so that this works in unit tests.
Maybe open_text_file_for_writing would work with should_append equal to true.

I also wonder whether it would not be good to buffer all changes to the set expectation file.
That way, we can log it in the console (debug level maybe) and open/write the file once.

> Tools/Scripts/webkitpy/w3c/test_updater.py:323
> +            if os.path.isfile(path):

We usually use filesystem for that.

> Tools/Scripts/webkitpy/w3c/test_updater.py:324
> +                self._remove_test_expectation_from_path(path, test_name)

We could also check whether the folder is empty and if so remove it.

> Tools/Scripts/webkitpy/w3c/test_updater.py:336
> +        data = self._load_results()

Shouldn't we load the results just once and not everytime _extract_failing_test_result is called?

> Tools/Scripts/webkitpy/w3c/test_updater.py:337
> +        tests = self._pre_process_tests(data['tests'])

Ditto there, it might be better to keep the list of tests once.
Next line seems to suggest we could want to have a matching_tests member that would organize test information keyed by the name.

> Tools/Scripts/webkitpy/w3c/test_updater.py:355
> +            results['name'] = file_name

Maybe _flatten_path could do the update here so that _pre_process_tests will be simpler.
I am not sure _flatten_path is the right method name.
Maybe something like _compute_test_list would be better?

> Tools/Scripts/webkitpy/w3c/test_updater.py:359
> +

Can probably directly return [] here or we could try to merge it with the for-loop above if we want to keep the rest of _pre_process_tests?

> Tools/Scripts/webkitpy/w3c/test_updater.py:368
> +        if result['actual'] in result['expected']:

can just do return:
result['actual'] in result['expected']

> Tools/Scripts/webkitpy/w3c/test_updater.py:378
> +                pass

Why do we need pass here?

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180611/936d0bd8/attachment-0001.html>

More information about the webkit-unassigned mailing list