Bug 73208 – prepare-ChangeLog can recognize a here-document in Perl

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

[Kentaro Hara]

Currently prepare-ChangeLog cannot recognize a hear document in Perl, which results in wrong subroutine names in ChangeLogs.

[Kentaro Hara]

Created attachment 116742 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 116742 [details]
Patch

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

> Tools/ChangeLog:3
> +        Support hear documents in prepare-ChangeLog

It's "here-document", not "hear document". You should fix this throughout the patch.

[Kentaro Hara]

Created attachment 116752 [details]
Patch

[Kentaro Hara]

(In reply to comment #2)
> (From update of attachment 116742 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116742&action=review
> 
> > Tools/ChangeLog:3
> > +        Support hear documents in prepare-ChangeLog
> 
> It's "here-document", not "hear document". You should fix this throughout the patch.

Sorry, fixed it.

[Kentaro Hara]

aroben: Would you please take a look?

[David Kilzer (:ddkilzer)]

Can you write a test for the get_function_line_ranges_for_perl() subroutine using the pattern in r100174 and r100287 for run-leaks?  (I think Adam Roben tried this for another patch recently, but I wasn't sure if it didn't work due to how globals were used in the function he wanted to test, or how globals were used in general in prepare-ChangeLog.)

[Kentaro Hara]

(In reply to comment #6)
> Can you write a test for the get_function_line_ranges_for_perl() subroutine using the pattern in r100174 and r100287 for run-leaks?  (I think Adam Roben tried this for another patch recently, but I wasn't sure if it didn't work due to how globals were used in the function he wanted to test, or how globals were used in general in prepare-ChangeLog.)

Thanks, David. I do not know the convention of how to write tests for prepare-ChangeLog.

- Create a new directory ./Tools/Scripts/webkitperl/prepare-ChangeLog-unittest.
- Add a new test ./Tools/Scripts/webkitperl/prepare-ChangeLog-unittest/perl-file.pl, which is a perl script to compare the generated change logs by ../../prepare-ChangeLog and the expected change logs.

Sounds fine?

[Adam Roben (:aroben)]

(In reply to comment #6)
> Can you write a test for the get_function_line_ranges_for_perl() subroutine using the pattern in r100174 and r100287 for run-leaks?  (I think Adam Roben tried this for another patch recently, but I wasn't sure if it didn't work due to how globals were used in the function he wanted to test, or how globals were used in general in prepare-ChangeLog.)

It was how globals were used in general that caused it not to be loadable as a module.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (In reply to comment #6)
> > Can you write a test for the get_function_line_ranges_for_perl() subroutine using the pattern in r100174 and r100287 for run-leaks?  (I think Adam Roben tried this for another patch recently, but I wasn't sure if it didn't work due to how globals were used in the function he wanted to test, or how globals were used in general in prepare-ChangeLog.)
> 
> Thanks, David. I do not know the convention of how to write tests for prepare-ChangeLog.
> 
> - Create a new directory ./Tools/Scripts/webkitperl/prepare-ChangeLog-unittest.
> - Add a new test ./Tools/Scripts/webkitperl/prepare-ChangeLog-unittest/perl-file.pl, which is a perl script to compare the generated change logs by ../../prepare-ChangeLog and the expected change logs.
> 
> Sounds fine?

The idea is that by including prepare-ChangeLog as a perl "module", you can unit-test individual subroutines without testing the whole process.

(In reply to comment #8)
> It was how globals were used in general that caused it not to be loadable as a module.

Based on arboen's comment, though, it sounds like this won't work until prepare-ChangeLog is changed to use fewer global variables (or use them differently), so we can't write tests for it immediately.

[Kentaro Hara]

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #6)
> (In reply to comment #8)
> > It was how globals were used in general that caused it not to be loadable as a module.
> 
> Based on arboen's comment, though, it sounds like this won't work until prepare-ChangeLog is changed to use fewer global variables (or use them differently), so we can't write tests for it immediately.

aroben, ddkilzer: Thanks. I uploaded a patch to make prepare-ChangeLog a loadable Perl module (bug 73531). Would you please take a look at it?

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > (In reply to comment #8)
> > > It was how globals were used in general that caused it not to be loadable as a module.
> > 
> > Based on arboen's comment, though, it sounds like this won't work until prepare-ChangeLog is changed to use fewer global variables (or use them differently), so we can't write tests for it immediately.
> 
> aroben, ddkilzer: Thanks. I uploaded a patch to make prepare-ChangeLog a loadable Perl module (bug 73531). Would you please take a look at it?

Thanks for trying to make prepare-ChangeLog testable, but I think the initial patch in Bug 73531 does too much at once.  It should be refactored in smaller steps, with tests written for each step.  Or we should modify prepare-ChangeLog just enough so it can be loaded as a module like run-leaks.

[Kentaro Hara]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > (In reply to comment #8)
> > > > It was how globals were used in general that caused it not to be loadable as a module.
> > > 
> > > Based on arboen's comment, though, it sounds like this won't work until prepare-ChangeLog is changed to use fewer global variables (or use them differently), so we can't write tests for it immediately.
> > 
> > aroben, ddkilzer: Thanks. I uploaded a patch to make prepare-ChangeLog a loadable Perl module (bug 73531). Would you please take a look at it?
> 
> Thanks for trying to make prepare-ChangeLog testable, but I think the initial patch in Bug 73531 does too much at once.  It should be refactored in smaller steps, with tests written for each step.  Or we should modify prepare-ChangeLog just enough so it can be loaded as a module like run-leaks.

OK. I'll do it step by step (please see comments in bug 73531 for more details).

### Other than fixing this Perl here-document bug, I'd like to add an --overwrite option to prepare-ChangeLog. The --overwrite option would update the latest ChangeLog without deleting the descriptions that I've already wrote. This change definitely requires unit test cases, which require prepare-ChangeLog to be a loadable Perl module.

[Kentaro Hara]

Created attachment 120510 [details]
Patch

[Kentaro Hara]

Uploaded a patch for review. It was a long way but this is the bug I originally wanted to fix:-)

[Kentaro Hara]

Created attachment 121097 [details]
rebased patch for review

[David Kilzer (:ddkilzer)]

Comment on attachment 121097 [details]
rebased patch for review

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

r=me if you fix the function regex.  Thanks for all the work getting to this point!

> Tools/Scripts/prepare-ChangeLog:1294
> +            if (/^sub\s+([\w\d]+)/) {

Technically a subroutine in Perl can't start with a number but can contain underscores, so this should be something like this (untested):

            if (/^sub\s+([\w_][\w\d_]*)/) {

[Kentaro Hara]

Committed r104401: <http://trac.webkit.org/changeset/104401>

[Kentaro Hara]

(In reply to comment #16)
> (From update of attachment 121097 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121097&action=review
> > Tools/Scripts/prepare-ChangeLog:1294
> > +            if (/^sub\s+([\w\d]+)/) {
> 
> Technically a subroutine in Perl can't start with a number but can contain underscores, so this should be something like this (untested):
> 
>             if (/^sub\s+([\w_][\w\d_]*)/) {

Fixed it and committed. Thanks!