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.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jason Marcell]

Modify prepare-Changelog to be aware of files that are marked as tests as well as files that are marked as requiring corresponding tests.

[Jason Marcell]

Created attachment 259995 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 259995 [details]
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.)

[Jason Marcell]

Created attachment 260192 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 260192 [details]
Patch

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

r=me if you fix the @directoryPartss typo.

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

Nit:  @directoryPartss => @directoryParts

[Jason Marcell]

Created attachment 260238 [details]
Patch

[Jason Marcell]

I uploaded a new patch to fix the typo.

[Jason Marcell]

Comment on attachment 260192 [details]
Patch

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

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

I addressed this issue in the most recent patch.

[Jason Marcell]

Comment on attachment 259995 [details]
Patch

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

>> Tools/ChangeLog:4
>> +        that are marked as requiring corresponding tests.
> 
> The second line seems like duplicate text that can be removed.

I addressed this.

>> Tools/ChangeLog:11
>> +        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.

I ran the tests before and after my patch and diffed the results. I didn't see any change.

>> Tools/Scripts/prepare-ChangeLog:1815
>> +        my (@dirparts) = File::Spec->splitdir($file);
> 
> @dirparts should be named @dirParts or @directoryParts.

I addressed this.

>> 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.)

When I was developing on the command line I was using standard Unix tools. I meant to convert to native PERL upon incorporating this code into the code base. As such I've changed this to use chomp in a later patch.

>> Tools/Scripts/prepare-ChangeLog:2032
>> +                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.)

I decided to work with strings internally but have attributeCommand return an integer (1 for true, 0 for false). Users should store a "1" for a given SVN property or Git attribute to indicate true.

[David Kilzer (:ddkilzer)]

Comment on attachment 260238 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 260238 [details]
Patch

Clearing flags on attachment: 260238

Committed r189195: <http://trac.webkit.org/changeset/189195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.