Bug 171424 – check-webkit-style should keep JavaScript test functions in sync

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

JavaScript test methods like shouldBe(), shouldNotBe(), shouldNotThrow() and shouldThrow() (among many others) have been copied into multiple source files:

JSTests/stress/resources/standalone-pre.js
LayoutTests/http/tests/resources/js-test-pre.js
LayoutTests/resources/js-test-pre.js
LayoutTests/resources/js-test.js
LayoutTests/resources/standalone-pre.js

Additionally, js-test-pre.js has actually been copied from LayoutTests/resources/ to LayoutTests/http/tests/resources/ so that http tests may use it.

The check-webkit-style script should try to keep these multiple copies in sync until a better solution is available:

$ ./Tools/Scripts/check-webkit-style --git-index 
ERROR: JSTests/stress/resources/standalone-pre.js:0:  Changes to function shouldBe() should be kept in sync with LayoutTests/http/tests/resources/js-test-pre.js.  [jstest/function_equality] [5]
ERROR: LayoutTests/resources/js-test-pre.js:0:  Changes should be kept in sync with LayoutTests/http/tests/resources/js-test-pre.js.  [jstest/resource_equality] [5]
ERROR: LayoutTests/resources/js-test-pre.js:0:  Changes to function shouldBe() should be kept in sync with LayoutTests/http/tests/resources/js-test-pre.js.  [jstest/function_equality] [5]
ERROR: LayoutTests/resources/js-test-pre.js:0:  Changes to function shouldNotBe() should be kept in sync with LayoutTests/http/tests/resources/js-test-pre.js.  [jstest/function_equality] [5]
ERROR: LayoutTests/resources/js-test-pre.js:0:  Changes to function shouldNotBe() should be kept in sync with LayoutTests/resources/js-test.js.  [jstest/function_equality] [5]
ERROR: LayoutTests/resources/js-test-pre.js:0:  Changes to function shouldNotBe() should be kept in sync with LayoutTests/resources/standalone-pre.js.  [jstest/function_equality] [5]
ERROR: LayoutTests/resources/standalone-pre.js:0:  Changes to function shouldBe() should be kept in sync with LayoutTests/http/tests/resources/js-test-pre.js.  [jstest/function_equality] [5]
ERROR: LayoutTests/resources/standalone-pre.js:0:  Changes to function shouldNotBe() should be kept in sync with LayoutTests/resources/js-test.js.  [jstest/function_equality] [5]
Total errors found: 8 in 6 files

[David Kilzer (:ddkilzer)]

Created attachment 308660 [details]
Patch v1 for EWS

[David Kilzer (:ddkilzer)]

Created attachment 308661 [details]
Patch v1 for Human Review

Created with 'git diff --patience --ignore-space-change -W' and then edited for ChangeLog sanity.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #1)
> Created attachment 308660 [details]
> Patch v1 for EWS

I suspect there may be a handful of test failures here (mostly from the stringify() change to shouldBe() in js-test.js), so please don't mark cq+ until all builds have finished.

[Build Bot]

Comment on attachment 308660 [details]
Patch v1 for EWS

Attachment 308660 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3648810

New failing tests:
fast/dom/Window/setTimeout-no-arguments.html

[Build Bot]

Created attachment 308713 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 308661 [details]
Patch v1 for Human Review

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

r=me

> Tools/Scripts/webkitpy/style/checkers/jstest.py:54
> +    function_name_regex = re.compile('^(?P<name>[_a-zA-Z][_a-zA-Z0-9]*)\s*\(.*', flags=re.MULTILINE)

Nit: The `.*` at the end of this regular expression can be dropped. It can match nothing, or anything, and is unused. So I think just ending in a \( is good enough.

This approach is a bit peculiar:

  (1) Split of `function\s+`
  (2) remove trailing blank lines and comments that are the dead space between when this function ended and the next function

But I suppose its fine given we don't want to be dealing with balancing braces and the format of these tests files is unlikely to change.

> Tools/Scripts/webkitpy/style/checkers/jstest.py:63
> +def strip_blank_lines_and_comments(function):

These only strip them from the end, so maybe rename this to strip_trailing_...

> Tools/Scripts/webkitpy/style/checkers/jstest.py:124
> +                                                 "Changes to function {0}() should be kept in sync with {1}.".format(
> +                                                     function_name, path))

Nit: I don't find this line break particularly helpful.

[David Kilzer (:ddkilzer)]

Comment on attachment 308661 [details]
Patch v1 for Human Review

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

>> Tools/Scripts/webkitpy/style/checkers/jstest.py:54
>> +    function_name_regex = re.compile('^(?P<name>[_a-zA-Z][_a-zA-Z0-9]*)\s*\(.*', flags=re.MULTILINE)
> 
> Nit: The `.*` at the end of this regular expression can be dropped. It can match nothing, or anything, and is unused. So I think just ending in a \( is good enough.
> 
> This approach is a bit peculiar:
> 
>   (1) Split of `function\s+`
>   (2) remove trailing blank lines and comments that are the dead space between when this function ended and the next function
> 
> But I suppose its fine given we don't want to be dealing with balancing braces and the format of these tests files is unlikely to change.

Yes, this was a compromise (that worked) since writing a full JS parser seemed to be overkill just to keep these in sync.

Let me know if you have a better suggestion for parsing individual functions out of a .js file.

Removed `.*` from the regex.

>> Tools/Scripts/webkitpy/style/checkers/jstest.py:63
>> +def strip_blank_lines_and_comments(function):
> 
> These only strip them from the end, so maybe rename this to strip_trailing_...

Done.

>> Tools/Scripts/webkitpy/style/checkers/jstest.py:124
>> +                                                     function_name, path))
> 
> Nit: I don't find this line break particularly helpful.

It was to keep the source from breaking past 120 columns.

I'll extract a local string variable instead.  :)

[Joseph Pecoraro]

> >> Tools/Scripts/webkitpy/style/checkers/jstest.py:124
> >> +                                                     function_name, path))
> > 
> > Nit: I don't find this line break particularly helpful.
> 
> It was to keep the source from breaking past 120 columns.
> 
> I'll extract a local string variable instead.  :)

Is 120 a python code style guideline? I didn't realize we had such rules.

[David Kilzer (:ddkilzer)]

CC-ing Ryan and Matt in case this causes some issues on the bots.  (I don't anticipate any, but I'm making a few minor changes to core JavaScript test code.)

[David Kilzer (:ddkilzer)]

(In reply to Joseph Pecoraro from comment #8)
> > >> Tools/Scripts/webkitpy/style/checkers/jstest.py:124
> > >> +                                                     function_name, path))
> > > 
> > > Nit: I don't find this line break particularly helpful.
> > 
> > It was to keep the source from breaking past 120 columns.
> > 
> > I'll extract a local string variable instead.  :)
> 
> Is 120 a python code style guideline? I didn't realize we had such rules.

Actually it's 79 characters if we were to adopt Python's own formatting rules:

<https://www.python.org/dev/peps/pep-0008/#maximum-line-length>

Extracting a variable for the error message basically resolved this.

[David Kilzer (:ddkilzer)]

I also added a small test case before landing because I realized jstest.py didn't have its own unit test, and map_functions_to_dict() really deserved one since it takes a fast-and-loose approach to parsing functions out of JavaScript source files.

[David Kilzer (:ddkilzer)]

Committed r216090: <http://trac.webkit.org/changeset/216090>

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #9)
> CC-ing Ryan and Matt in case this causes some issues on the bots.  (I don't
> anticipate any, but I'm making a few minor changes to core JavaScript test
> code.)

NOTE: There were no changes to test results when WK2 tests were run locally with and without this patch.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #12)
> Committed r216090: <http://trac.webkit.org/changeset/216090>

Rolled out Tools/Scripts/webkitpy/style/checkers/jstest_unittest.py in r216091:
<http://trac.webkit.org/changeset/216091>

I'll post it in a separate patch.