Bug 220180

Created attachment 416826 [details]
proposed patch

<rdar://problem/72782271>

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

rs=me

> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:158
>            test_dir: absolute path to the LayoutTests directory.

Should delete this line as well.

> Tools/Scripts/webkitpy/tool/commands/upload.py:-511
> -        bug_id = tool.bugs.create_bug(bug_title, comment_text, options.component, diff, "Patch", cc=options.cc, mark_for_review=options.review, mark_for_commit_queue=options.request_commit)

Maybe we could add a log statement here to log the bug_id (either user visible or at DEBUG level). Not sure how helpful that would be.

Comment on attachment 416826 [details]
proposed patch

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

I don't think we should be changing some of the style checker ones without just removing the classes in question

> Tools/Scripts/webkitpy/style/checkers/jsonchecker.py:34
> +    def __init__(self, handle_style_error):

If this class isn't accepting a file path, what is it even doing? Seems like the better approach on this one would be to make it actually work, which I think would be be done by reading the file in, then printing the json with an indent of 4 and comparing.

> Tools/Scripts/webkitpy/style/checkers/watchlist.py:40
> +    def __init__(self, handle_style_error):

If this class isn't accepting a file path, what is it even doing?

> Tools/Scripts/webkitpy/style/checkers/xml.py:34
>          self._handle_style_error = handle_style_error

If this class isn't accepting a file_path, what is it even doing?

> Tools/Scripts/webkitpy/tool/steps/suggestreviewers_unittest.py:48
> +        OutputCapture()

Should just remove this entirely.

> If this class isn't accepting a file path, what is it even doing?

These checkers use the lines argument, and don't need a file path. There can be an argument for keeping a uniform interface for all checkers though.

I'm going to revert this part for now, because it's also incorrect - JSONChecker has subclasses that are still created with a file path argument, and are thus completely broken by this patch. And there are zero unit tests for any of them, so test-webkitpy didn't tell me about the mistake.

Committed https://trac.webkit.org/r271158

This broke `run-builtins-generator-tests`:

Traceback (most recent call last):
  File "./Tools/Scripts/run-builtins-generator-tests", line 48, in <module>
    sys.exit(main(sys.argv))
  File "./Tools/Scripts/run-builtins-generator-tests", line 45, in main
    return BuiltinsGeneratorTests(reset_results, executive.Executive()).main()
  File "/Volumes/Data/slave/catalina-release-tests-wk2/build/Tools/Scripts/webkitpy/codegen/main.py", line 161, in main
    if not self.run_tests(input_directory, reference_directory):
  File "/Volumes/Data/slave/catalina-release-tests-wk2/build/Tools/Scripts/webkitpy/codegen/main.py", line 148, in run_tests
    if not self.run_test(reference_directory, "WebCoreJSBuiltins.h", separately_generated_files, self.wrappers_builtin_test):
  File "/Volumes/Data/slave/catalina-release-tests-wk2/build/Tools/Scripts/webkitpy/codegen/main.py", line 114, in run_test
    if generate_builtin_callback(test_name, test_files, work_directory):
TypeError: wrappers_builtin_test() takes exactly 3 arguments (4 given)


https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Tests/builds/10897/steps/builtins-generator-tests/logs/stdio

(In reply to Ryan Haddad from comment #7)
> This broke `run-builtins-generator-tests`
Reverted the relevant part of r271158 in r271173.