Bug 174876 – prepare-ChangeLog should not list added layout tests in PAL ChangeLog

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

When a change contains a WebCore change, one or more added layout tests, and a PAL change then prepare-ChangeLog will list the added layout tests in both the WebCore and PAL ChangeLog files. Although platform functionality in PAL may be directly exercised in a layout test I do not get the impression that a layout test can always be used to exercise PAL functionality or at least exercise all edge cases easily. Unit tests seem more appropriate for abstractions in PAL. Therefore, prepare-ChangeLog should not list added layout tests in the PAL ChangeLog.

[Daniel Bates]

Created attachment 316488 [details]
Patch

[Darin Adler]

Comment on attachment 316488 [details]
Patch

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

> Tools/Scripts/prepare-ChangeLog:691
> +        if (unixPath($prefix) =~ m|WebCore(?!/PAL)| || unixPath(`pwd`) =~ m|WebCore(?!/PAL)| || @$requiresTests) {

I don’t think this fix need to be so specific to PAL.

[Daniel Bates]

(In reply to Darin Adler from comment #2)
> Comment on attachment 316488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316488&action=review
> 
> > Tools/Scripts/prepare-ChangeLog:691
> > +        if (unixPath($prefix) =~ m|WebCore(?!/PAL)| || unixPath(`pwd`) =~ m|WebCore(?!/PAL)| || @$requiresTests) {
> 
> I don’t think this fix need to be so specific to PAL.

I am unclear how to avoid making this fix specific to PAL for the second disjunct without breaking the prepare-ChangeLog feature implemented by the second disjunct when the current working directory is some non-PAL subdirectory of Source/WebCore. If we do not need to support this feature then I can remove the second disjunct and write the first disjunct as "unixPath($prefix) =~ m|WebCore/$|" (without double quotes). The purpose of the second disjunct is to always (*) emit "No new tests (OOPS!)." in the WebCore ChangeLog entry whenever a person runs prepare-ChangeLog inside Source/WebCore or a subdirectory of it. I suspect the rationality (**) behind this behavior is to support the workflow of some people that want to generate a ChangeLog and/or patch from a subdirectory of their checkout either because they have unrelated changes in another part of their checkout or as a performance optimization to avoid stat'ing their entire checkout. For people with this workflow prepare-ChangeLog wants to hint to them that they either must by hand remove the "No new tests (OOPS!)." line and list the added tests or provide a reason for the omission of tests OR run prepare-ChangeLog from the top-level WebKit checkout directory so that prepare-ChangeLog can find any added tests. Do we need to support this feature? If so, do we consider the feature meaningful if its limited to when the current working directory is Source/WebCore and not a subdirectory of it? Do you have any insight/suggestions on how to fix up the second disjunct without being specific to PAL or enumerating all other (or most) subdirectories of Source/WebCore?

(*) We will always emit this line because @addedRegressionTests will always be empty since we only add an item to this list if prepare-ChangeLog saw an added file to LayoutTests. And prepare-ChangeLog only sees files that are in the current working directory or subdirectories of it.

(**) This code has existed since prepare-ChangeLog was added by Maciej Stachowiak in <https://trac.webkit.org/changeset/9406>. Maybe he still recalls the reason he added this code. CC'ed Maciej.

[Daniel Bates]

(In reply to Daniel Bates from comment #3)
> For people with this workflow prepare-ChangeLog wants to hint to them ...

This should read:

For people with this workflow and run prepare-ChangeLog from Source/WebCore or a subdirectory of it prepare-ChangeLog wants to hint to them ...

[Darin Adler]

Comment on attachment 316488 [details]
Patch

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

>>> Tools/Scripts/prepare-ChangeLog:691
>>> +        if (unixPath($prefix) =~ m|WebCore(?!/PAL)| || unixPath(`pwd`) =~ m|WebCore(?!/PAL)| || @$requiresTests) {
>> 
>> I don’t think this fix need to be so specific to PAL.
> 
> I am unclear how to avoid making this fix specific to PAL for the second disjunct without breaking the prepare-ChangeLog feature implemented by the second disjunct when the current working directory is some non-PAL subdirectory of Source/WebCore. If we do not need to support this feature then I can remove the second disjunct and write the first disjunct as "unixPath($prefix) =~ m|WebCore/$|" (without double quotes). The purpose of the second disjunct is to always (*) emit "No new tests (OOPS!)." in the WebCore ChangeLog entry whenever a person runs prepare-ChangeLog inside Source/WebCore or a subdirectory of it. I suspect the rationality (**) behind this behavior is to support the workflow of some people that want to generate a ChangeLog and/or patch from a subdirectory of their checkout either because they have unrelated changes in another part of their checkout or as a performance optimization to avoid stat'ing their entire checkout. For people with this workflow prepare-ChangeLog wants to hint to them that they either must by hand remove the "No new tests (OOPS!)." line and list the added tests or provide a reason for the omission of tests OR run prepare-ChangeLog from the top-level WebKit checkout directory so that prepare-ChangeLog can find any added tests. Do we need to support this feature? If so, do we consider the feature meaningful if its limited to when the current working directory is Source/WebCore and not a subdirectory of it? Do you have any insight/suggestions on how to fix up the second disjunct without being specific to PAL or enumerating all other (or most) subdirectories of Source/WebCore?
> 
> (*) We will always emit this line because @addedRegressionTests will always be empty since we only add an item to this list if prepare-ChangeLog saw an added file to LayoutTests. And prepare-ChangeLog only sees files that are in the current working directory or subdirectories of it.
> 
> (**) This code has existed since prepare-ChangeLog was added by Maciej Stachowiak in <https://trac.webkit.org/changeset/9406>. Maybe he still recalls the reason he added this code. CC'ed Maciej.

Yes, I want you to do what you mentioned in the first sentence.

    if (unixPath($prefix) =~ m|/WebCore/$| || @$requiresTests) {

The same bug affects WebCore/platform/gtk/po/ChangeLog and your fix would not help in that case, but mine would.

Can you give an example of what you think this breaks about subdirectories? Just give me a specific example. No need for rationale or philosophy. In what case is the behavior different? Steps to reproduce. I tried and could not reproduce any difference in behavior.

[Daniel Bates]

(In reply to Darin Adler from comment #5)
> Can you give an example of what you think this breaks about subdirectories?
> Just give me a specific example. No need for rationale or philosophy. In
> what case is the behavior different? Steps to reproduce. I tried and could
> not reproduce any difference in behavior.

Never mind. I misread the code in findChangeLogs() and for some reason I thought $prefix was the full path to a modified file.

[Daniel Bates]

Created attachment 316520 [details]
Patch

[Build Bot]

Comment on attachment 316520 [details]
Patch

Attachment 316520 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4194715

New failing tests:
fast/selectors/nth-last-child-of-selector-list-ending-with-never-matching-selectors.html

[Build Bot]

Created attachment 316527 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Daniel Bates]

(In reply to Build Bot from comment #8)
> Comment on attachment 316520 [details]
> Patch
> 
> Attachment 316520 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/4194715
> 
> New failing tests:
> fast/selectors/nth-last-child-of-selector-list-ending-with-never-matching-
> selectors.html

Clearly this failure is unrelated to this patch as this patch does not touch the engine.

[Daniel Bates]

Comment on attachment 316520 [details]
Patch

Clearing flags on attachment: 316520

Committed r220032: <http://trac.webkit.org/changeset/220032>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33601796>