Bug 131727 – Fix the commit-log-editor after r167243 and add more unit tests

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

[Csaba Osztrogonác]

http://trac.webkit.org/changeset/167243 broke a webkitpy 
unit test and update landed in http://trac.webkit.org/r167296

Sorry for causing troubles, I didn't know if there is a webkitpy
unit test for the Tools/Scripts/commit-log-editor perl script.
I checked the webkitperl directory and haven't seen any unit
test for this script, that's why I didn't want to expect the
author to implement a unit test from the scratch for a script
which can't be mockable and relies on filesystem changes.

But know we know that we have an existing unit test for 
it in webkitpy, so we should add unit tests for r167243.

Additionally I think the new behaviour which caused the
unit test failure isn't the expected result, we didn't
want to remove the extra newline after directory names.
I think it is a minor bug which can be fixed easily.

[Csaba Osztrogonác]

I think I got the problem. Check this small changelog entry:
------------------------------------------------------------------------------
2014-04-14  Peter Molnar  <[e-mail]>

        Fix incorrect indentations in CodeGeneratorJS.pm introduced in r165521
        https://bugs.webkit.org/show_bug.cgi?id=131613

        Reviewed by Csaba Osztrogonác.

        * bindings/scripts/CodeGeneratorJS.pm:
        (GenerateImplementation):
        Fixed 5-space indentation.
        * bindings/scripts/test/JS/JSTestNondeterministic.cpp:
        Updated the tests accordingly.

------------------------------------------------------------------------------



In this case the common prefix was:
------------------------------------------------------------------------------
2014-04-14  Peter Molnar  <[e-mail]>


        Fix incorrect indentations in CodeGeneratorJS.pm introduced in r165521
        https://bugs.webkit.org/show_bug.cgi?id=131613

        Reviewed by Csaba Osztrogonác.

------------------------------------------------------------------------------



And the non-common part was:
------------------------------------------------------------------------------


        * bindings/scripts/CodeGeneratorJS.pm:
        (GenerateImplementation):
        Fixed 5-space indentation.
        * bindings/scripts/test/JS/JSTestNondeterministic.cpp:
        Updated the tests accordingly.

------------------------------------------------------------------------------

The problem was that commit-log-editor relied on the extra \n\n in non-common
parts and it didn't add any \n after the name of the label, eg: "WebCore:"

[Csaba Osztrogonác]

Created attachment 229436 [details]
Patch

WIP patch. It works for the mentioned case, but I haven't tested it on other cases. Additionally I'm not sure if it is the best fix, because the non-common part still starts with a \n. We really need more unit tests for it. Maybe we should rewrite commit-log-editor in python and make it mockable not to depend on local file system changes.

[Csaba Osztrogonác]

I won't be online until next tuesday, I can fix it properly after that.
If somebody can play with my WIP patch, feel free to pick it up.

[Éva Balázsfalvi]

Created attachment 235692 [details]
Patch

[WebKit Commit Bot]

Attachment 235692 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:202:  [CommitMessageForThisCommitTest.test_commit_message_for_unreviewed_changelogs_with_different_messages] Passing unexpected keyword argument 'return_stderr' in function call  [pylint/E1123] [5]
ERROR: Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:223:  [CommitMessageForThisCommitTest.test_commit_message_for_one_reviewed_changelog] Passing unexpected keyword argument 'return_stderr' in function call  [pylint/E1123] [5]
ERROR: Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:277:  [CommitMessageForThisCommitTest.test_commit_message_for_changelogs_with_different_messages] Passing unexpected keyword argument 'return_stderr' in function call  [pylint/E1123] [5]
ERROR: Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:304:  [CommitMessageForThisCommitTest.test_commit_message_for_one_rollout_changelog] Passing unexpected keyword argument 'return_stderr' in function call  [pylint/E1123] [5]
ERROR: Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:330:  [CommitMessageForThisCommitTest.test_commit_message_for_rollout_changelogs_with_different_directories] Passing unexpected keyword argument 'return_stderr' in function call  [pylint/E1123] [5]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Éva Balázsfalvi]

Created attachment 235811 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 235811 [details]
Patch

Clearing flags on attachment: 235811

Committed r172029: <http://trac.webkit.org/changeset/172029>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

This change caused a regression for me:
<https://webkit.org/b/135744> commit-log-message has extra blank line at the top of pre-populated message