[Webkit-unassigned] [Bug 148498] Modify prepare-Changelog to be aware of files that are marked as tests as well as files that are marked as requiring corresponding tests.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=148498

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #259995|review?, commit-queue?      |review+, commit-queue-
              Flags|                            |

--- Comment #2 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 259995
  --> https://bugs.webkit.org/attachment.cgi?id=259995
Patch

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

r=me if you fix the duplicate text in ChangeLog, run test-webkitperl (and the prepare-ChangeLog tests still pass), and rename @dirparts.

> Tools/ChangeLog:4
> +        Modify prepare-Changelog to be aware of files that are marked as tests as well as files that are marked as requiring corresponding tests.
> +        that are marked as requiring corresponding tests.

The second line seems like duplicate text that can be removed.

> Tools/ChangeLog:11
> +        * Scripts/prepare-ChangeLog: Added "attributeCache" to cache Subversion properties in order to
> +        simulate Git's attribute behevaior.

Note that there are unit tests for prepare-ChangeLog in Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/.

This patch didn't modify any methods that are already tested, but we want to make sure the tests still run with this change (because each unit test imports prepare-ChangeLog as a custom Perl module, so things like global variables can break that technique).

The tests are run thusly:  ./Tools/Scripts/test-webkitperl

Looks like there are a couple of tests that are broken now, but the prepare-ChangeLog tests look fine.

> Tools/Scripts/prepare-ChangeLog:85
> -sub generateNewChangeLogs($$$$$$$$$$$$);
> +sub generateNewChangeLogs($$$$$$$$$$$$$);

Some day, maybe we should change generateNewChangeLogs() to take a single argument that's a hash so we can get pseudo-named arguments in Perl.

You do not have to fix this to land the patch.

> Tools/Scripts/prepare-ChangeLog:1815
> +        my (@dirparts) = File::Spec->splitdir($file);

@dirparts should be named @dirParts or @directoryParts.

> Tools/Scripts/prepare-ChangeLog:1827
> +            my $command = SVN . " propget $attr $subPath | tr -d '\\n'";

Any reason you use 'tr' here instead of chomp()?  (Not necessary to fix to land the patch.)

> Tools/Scripts/prepare-ChangeLog:2032
> +            } elsif (attributeCommand($file, "test") eq "true") {
> +                push @addedRegressionTests, $file;
> +            } elsif (attributeCommand($file, "requiresTests") eq "true") {
> +                push @requiresTests, $file

I find returning string results of 'true' or 'unspecified' kind of unusual.  Is there a reason you couldn't use 1 for 'true' and '0' for unspecified?

Oh, is this to make the svn and git attributes more readable in that you're literally storing 'true' instead of just '1'?

(This doesn't need to be addressed before landing the change, but I'd like to understand the design decision here.)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150827/bef8b164/attachment.html>