Add WPTEXPORT footer automatically in DepsUpdater and imported/PRESUBMIT

Add WPTEXPORT footer automatically in DepsUpdater and imported/PRESUBMIT

BUG=657117
WPTEXPORT=false

[jeffcarp, 2016-11-16 23:58:22]

Description was changed from

==========
Add WPTEXPORT footer automatically in DepsUpdater and imported/PRESUBMIT

BUG=
==========

to

==========
Add WPTEXPORT footer automatically in DepsUpdater and imported/PRESUBMIT

BUG=
WPTEXPORT=true
==========

[jeffcarp, 2016-11-17 00:00:28]

Description was changed from

==========
Add WPTEXPORT footer automatically in DepsUpdater and imported/PRESUBMIT

BUG=
WPTEXPORT=true
==========

to

==========
Add WPTEXPORT footer automatically in DepsUpdater and imported/PRESUBMIT

BUG=
WPTEXPORT=false
==========

[jeffcarp, 2016-11-17 00:00:28]

jeffcarp@chromium.org changed reviewers:
+ foolip@chromium.org, qyearsley@chromium.org

[jeffcarp, 2016-11-17 00:02:31]

Looks like it's working, it automatically added WPTEXPORT=true to this CL
because it modifies files in LayoutTests/imported.

[jeffcarp, 2016-11-17 00:03:10]

Description was changed from

==========
Add WPTEXPORT footer automatically in DepsUpdater and imported/PRESUBMIT

BUG=
WPTEXPORT=false
==========

to

==========
Add WPTEXPORT footer automatically in DepsUpdater and imported/PRESUBMIT

BUG=657117
WPTEXPORT=false
==========

[qyearsley, 2016-11-17 00:23:27]

Looks good - many presubmits don't have tests, but I think it's nice to have
tests :-)

Also, is it too late to propose an alternate names for this marker? What about
WPT_EXPORTABLE rather than WPTEXPORT? It's a bit longer but a bit more explicit,
and people won't normally have to write it by hand so length doesn't matter too
much.

Many of these all-caps "description flags" are short with no spaces, like NOTRY,
but some are longer, like CQ_INCLUDE_TRYBOTS. WDYT?

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/imported/PRESUBMIT.py (right):

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT.py:8: def PostUploadHook(cl,
change, output_api):  # pylint: disable=C0103
For pylint disable pragma, you can use the symbolic name of the pylint rule.
Also optionally you can explain why it's necessary, e.g.
# Function name dictated by depot_tools/presubmit_support - pylint:
disable=invalid-name

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT.py:12: to better help the WPT
export script identify exportable commits.
The second sentence could be changed to note what WPTEXPORT signifies, e.g.
"WPTEXPORT indicates whether or not the commit should be automatically exported
to the web-platform-tests repo."

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/imported/PRESUBMIT_test.py (right):

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT_test.py:8: # from
PRESUBMIT_test_mocks import MockOutputApi  # pylint: disable=F0401
I think you'd have to add something to the effect of:

sys.path.append(os.path.join('..', '..', '..', '..'))

Does that work? (Also, the `import unittest` would usually go at the top I
think)

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT_test.py:24: # TODO: Couldn't
figure out how to mock this without using a global variable
How about making the MockCL constructor take a description - then the above
methods would look more like:

    def test_doesnt_add_WPTEXPORT_if_exists(self):
        mock_cl = MockCL(description='mock description\nWPTEXPORT=false')
        PostUploadHook(mock_cl, None, MockOutputAPI())
        self.assertIn('WPTEXPORT=false', mock_cl.description)

[foolip, 2016-11-17 09:59:14]

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/imported/PRESUBMIT.py (right):

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT.py:9: """git cl upload will
call this hook after the issue is created/modified.
What about things in LT/imported/csswg-tests and wpt_automation? Can this script
avoid changing those CLs?

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT.py:11: This hook adds
WPTEXPORT=true to a CL if it doesn't already exist. This is
Whether this is useful depends on what you intend for the export script to do.
If it finds a change in LT/imported/wpt/ with no WPTEXPORT flag, what will it
do? That will still happen, e.g. if there's a sheriff revert using
NOPRESUBMIT=true.

Maybe that situation would require human intervention?

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/imported/PRESUBMIT_test.py (right):

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT_test.py:7: # TODO: how to run
the tests so this is imported correctly?
TODO(jeffcarp)

[qyearsley, 2016-11-17 17:44:50]

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/imported/PRESUBMIT.py (right):

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT.py:11: This hook adds
WPTEXPORT=true to a CL if it doesn't already exist. This is
On 2016/11/17 at 09:59:14, foolip wrote:
> Whether this is useful depends on what you intend for the export script to do.
If it finds a change in LT/imported/wpt/ with no WPTEXPORT flag, what will it
do? That will still happen, e.g. if there's a sheriff revert using
NOPRESUBMIT=true.
> 
> Maybe that situation would require human intervention?

I think I'd expect that the default would be for the exporter to try to export
if there's no WPTEXPORT flag given.

In this case, another way that this could be done would be to not add the flag
in the presubmit, but check for a flag "NOEXPORT=true" added to import CLs, and
optionally added to other CLs. That way is pretty consistent with the way that
NOTRY and NOPRESUBMIT are, as a means of disabling a default behavior.

[jeffcarp, 2016-11-17 20:49:58]

Re: qyearsley's comment - maybe it makes sense just to have the export script
look for a NOEXPORT=true flag?

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/imported/PRESUBMIT.py (right):

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT.py:9: """git cl upload will
call this hook after the issue is created/modified.
On 2016/11/17 at 09:59:14, foolip wrote:
> What about things in LT/imported/csswg-tests and wpt_automation? Can this
script avoid changing those CLs?

Yeah it sounds possible to filter those out (in theory)

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT.py:11: This hook adds
WPTEXPORT=true to a CL if it doesn't already exist. This is
On 2016/11/17 at 17:44:50, qyearsley wrote:
> On 2016/11/17 at 09:59:14, foolip wrote:
> > Whether this is useful depends on what you intend for the export script to
do. If it finds a change in LT/imported/wpt/ with no WPTEXPORT flag, what will
it do? That will still happen, e.g. if there's a sheriff revert using
NOPRESUBMIT=true.
> > 
> > Maybe that situation would require human intervention?
> 
> I think I'd expect that the default would be for the exporter to try to export
if there's no WPTEXPORT flag given.
> 
> In this case, another way that this could be done would be to not add the flag
in the presubmit, but check for a flag "NOEXPORT=true" added to import CLs, and
optionally added to other CLs. That way is pretty consistent with the way that
NOTRY and NOPRESUBMIT are, as a means of disabling a default behavior.

That sounds like a cleaner way of implementing this behavior - and it requires
no modifications to the presubmit process, only modifications to the export
script.

[foolip, 2016-11-17 20:52:34]

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/imported/PRESUBMIT.py (right):

https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/imported/PRESUBMIT.py:11: This hook adds
WPTEXPORT=true to a CL if it doesn't already exist. This is
On 2016/11/17 20:49:58, jeffcarp wrote:
> On 2016/11/17 at 17:44:50, qyearsley wrote:
> > On 2016/11/17 at 09:59:14, foolip wrote:
> > > Whether this is useful depends on what you intend for the export script to
> do. If it finds a change in LT/imported/wpt/ with no WPTEXPORT flag, what will
> it do? That will still happen, e.g. if there's a sheriff revert using
> NOPRESUBMIT=true.
> > > 
> > > Maybe that situation would require human intervention?
> > 
> > I think I'd expect that the default would be for the exporter to try to
export
> if there's no WPTEXPORT flag given.
> > 
> > In this case, another way that this could be done would be to not add the
flag
> in the presubmit, but check for a flag "NOEXPORT=true" added to import CLs,
and
> optionally added to other CLs. That way is pretty consistent with the way that
> NOTRY and NOPRESUBMIT are, as a means of disabling a default behavior.
> 
> That sounds like a cleaner way of implementing this behavior - and it requires
> no modifications to the presubmit process, only modifications to the export
> script.

Yeah, that sounds good. I was thinking WPTEXPORT=false to avoid the negation,
but NOEXPORT=true would fit nicely in with existing flags, so let's do that. Not
sure if there's a "flag owner", but might be worth checking who added some
existing flags and if they think the name is good.

[jeffcarp, 2016-11-28 17:33:24]

On 2016/11/17 at 20:52:34, foolip wrote:
>
https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
> File third_party/WebKit/LayoutTests/imported/PRESUBMIT.py (right):
> 
>
https://codereview.chromium.org/2512563002/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/imported/PRESUBMIT.py:11: This hook adds
WPTEXPORT=true to a CL if it doesn't already exist. This is
> On 2016/11/17 20:49:58, jeffcarp wrote:
> > On 2016/11/17 at 17:44:50, qyearsley wrote:
> > > On 2016/11/17 at 09:59:14, foolip wrote:
> > > > Whether this is useful depends on what you intend for the export script
to
> > do. If it finds a change in LT/imported/wpt/ with no WPTEXPORT flag, what
will
> > it do? That will still happen, e.g. if there's a sheriff revert using
> > NOPRESUBMIT=true.
> > > > 
> > > > Maybe that situation would require human intervention?
> > > 
> > > I think I'd expect that the default would be for the exporter to try to
export
> > if there's no WPTEXPORT flag given.
> > > 
> > > In this case, another way that this could be done would be to not add the
flag
> > in the presubmit, but check for a flag "NOEXPORT=true" added to import CLs,
and
> > optionally added to other CLs. That way is pretty consistent with the way
that
> > NOTRY and NOPRESUBMIT are, as a means of disabling a default behavior.
> > 
> > That sounds like a cleaner way of implementing this behavior - and it
requires
> > no modifications to the presubmit process, only modifications to the export
> > script.
> 
> Yeah, that sounds good. I was thinking WPTEXPORT=false to avoid the negation,
but NOEXPORT=true would fit nicely in with existing flags, so let's do that. Not
sure if there's a "flag owner", but might be worth checking who added some
existing flags and if they think the name is good.

Closing this CL in favor of adding NOEXPORT=true automatically in the WPT
importer.