<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add script to update web-platform-tests expectations after import"
   href="https://bugs.webkit.org/show_bug.cgi?id=186359#c8">Comment # 8</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add script to update web-platform-tests expectations after import"
   href="https://bugs.webkit.org/show_bug.cgi?id=186359">bug 186359</a>
              from <span class="vcard"><a class="email" href="mailto:youennf@gmail.com" title="youenn fablet <youennf@gmail.com>"> <span class="fn">youenn fablet</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=342430&action=diff" name="attach_342430" title="Patch">attachment 342430</a> <a href="attachment.cgi?id=342430&action=edit" title="Patch">[details]</a></span>
Patch

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: <a href="https://bugs.webkit.org/attachment.cgi?id=342430&action=review">https://bugs.webkit.org/attachment.cgi?id=342430&action=review</a>

<span class="quote">> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:-460
> -            self._filesystem.remove(results_json_path)</span >

Is this change actually needed?

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

s/This /This runs /

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

s/TestExpecations/TestExpectations/ here and elsewhere

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:100
> +# Add documentation of argorithm for updating test expectations</span >

algorithm.

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:102
> +# add unittests</span >

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.

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:255
> +    def _failing_testharness_test(self, test):</span >

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.

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

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.

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

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.

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:323
> +            if os.path.isfile(path):</span >

We usually use filesystem for that.

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:324
> +                self._remove_test_expectation_from_path(path, test_name)</span >

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

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:336
> +        data = self._load_results()</span >

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

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

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.

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:355
> +            results['name'] = file_name</span >

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?

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:359
> +</span >

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?

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:368
> +        if result['actual'] in result['expected']:</span >

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

<span class="quote">> Tools/Scripts/webkitpy/w3c/test_updater.py:378
> +                pass</span >

Why do we need pass here?</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>