Patchwork [4,of,4] hook: don't crash on syntax errors in python hooks

login
register
mail settings
Submitter Jun Wu
Date Feb. 29, 2016, 2:20 p.m.
Message ID <56D453C2.1070000@fb.com>
Download mbox | patch
Permalink /patch/13468/
State Not Applicable
Headers show

Comments

Jun Wu - Feb. 29, 2016, 2:20 p.m.
I have been getting "ERROR: test-check-commit.t output changed" for a
while and traced back to this.



It seems that the patch in the email is correct while the version in
clowncopter is wrong. Added '$' to the end of line to clarify:

Email:

+      $
+     ^$

hg export b892e424f88c

+      $
+      ^$

On 02/13/2016 03:54 PM, Pierre-Yves David wrote:
>
>
> On 02/12/2016 10:53 PM, Siddharth Agarwal wrote:
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0@fb.com>
>> # Date 1455317410 28800
>> #      Fri Feb 12 14:50:10 2016 -0800
>> # Node ID d046c71f86718d41b9c5e9f601ca0f0d87606239
>> # Parent  c9fcff42975be8331c4e10add62d3bca983d155e
>> # Available At
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__42.netv6.net_sid0-2Dwip_hg_&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=goC_KEU1hoLmVFdaD4QgHA&m=2roKYcRlftufTeEFQJ2pReGriNxoZamITzBBTUtsYy0&s=aXhfCjZ8MgH5H5LclBIo1bpX7xBJjVqx7LLHbVLumg0&e=
>> #              hg pull
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__42.netv6.net_sid0-2Dwip_hg_&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=goC_KEU1hoLmVFdaD4QgHA&m=2roKYcRlftufTeEFQJ2pReGriNxoZamITzBBTUtsYy0&s=aXhfCjZ8MgH5H5LclBIo1bpX7xBJjVqx7LLHbVLumg0&e=
>> -r d046c71f8671
>> hook: don't crash on syntax errors in python hooks
>
> Pushed to the clowncopter. Thanks.
>
> (thanks for the pull url)
>
Yuya Nishihara - Feb. 29, 2016, 4:19 p.m.
On Mon, 29 Feb 2016 14:20:50 +0000, Jun Wu wrote:
> I have been getting "ERROR: test-check-commit.t output changed" for a
> while and traced back to this.
> 
> --- test-hook.t
> +++ test-hook.t.err
> @@ -536,7 +536,7 @@
>     exception from first failed import attempt:
>     Traceback (most recent call last):
> 
> -      ^
> +     ^

Perhaps it depends on Python versions where the syntax error is detected.
Do we need these lines in tests?
Martijn Pieters - March 4, 2016, 3:04 p.m.
On 29 February 2016 at 16:19, Yuya Nishihara <yuya@tcha.org> wrote:
> Perhaps it depends on Python versions where the syntax error is detected.

Looks like it. I get the test failure when using Python 2.6, not with
Python 2.7.

And that's surprising, because a plain test file with `(foo` on a line
gives the exact same syntax error output in 2.6 or 2.7. So this is may
be an issue with the `traceback` module in 2.6.

Perhaps a different syntax error can be devised that doesn't exhibit this issue.
Pierre-Yves David - March 8, 2016, 12:13 p.m.
On 03/04/2016 04:04 PM, Martijn Pieters wrote:
> On 29 February 2016 at 16:19, Yuya Nishihara <yuya@tcha.org> wrote:
>> Perhaps it depends on Python versions where the syntax error is detected.
>
> Looks like it. I get the test failure when using Python 2.6, not with
> Python 2.7.
>
> And that's surprising, because a plain test file with `(foo` on a line
> gives the exact same syntax error output in 2.6 or 2.7. So this is may
> be an issue with the `traceback` module in 2.6.
>
> Perhaps a different syntax error can be devised that doesn't exhibit this issue.

We should probably just (glob) it.
Siddharth Agarwal - March 10, 2016, 12:18 a.m.
On 3/8/16 04:13, Pierre-Yves David wrote:
>
>
> On 03/04/2016 04:04 PM, Martijn Pieters wrote:
>> On 29 February 2016 at 16:19, Yuya Nishihara <yuya@tcha.org> wrote:
>>> Perhaps it depends on Python versions where the syntax error is 
>>> detected.
>>
>> Looks like it. I get the test failure when using Python 2.6, not with
>> Python 2.7.
>>
>> And that's surprising, because a plain test file with `(foo` on a line
>> gives the exact same syntax error output in 2.6 or 2.7. So this is may
>> be an issue with the `traceback` module in 2.6.
>>
>> Perhaps a different syntax error can be devised that doesn't exhibit 
>> this issue.
>
> We should probably just (glob) it.
>

Just sent a patch filtering this out.

Patch

--- test-hook.t
+++ test-hook.t.err
@@ -536,7 +536,7 @@ 
    exception from first failed import attempt:
    Traceback (most recent call last):

-      ^
+     ^
    SyntaxError: invalid syntax
    exception from second failed import attempt:
    Traceback (most recent call last):