Modify prepare-Changelog to be aware of files that are marked as tests as well as files that are marked as requiring corresponding tests.
Created attachment 259995 [details] Patch
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.)
Created attachment 260192 [details] Patch
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
Created attachment 260238 [details] Patch
I uploaded a new patch to fix the typo.
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.
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.
Comment on attachment 260238 [details] Patch r=me
Comment on attachment 260238 [details] Patch Clearing flags on attachment: 260238 Committed r189195: <http://trac.webkit.org/changeset/189195>
All reviewed patches have been landed. Closing bug.