Patchwork test: remove dependency on $PYTHONDIR

login
register
mail settings
Submitter Durham Goode
Date Feb. 17, 2016, 11:18 p.m.
Message ID <5cef3483a31ecd1f1808.1455751138@dev8486.prn1.facebook.com>
Download mbox | patch
Permalink /patch/13252/
State Superseded
Delegated to: Yuya Nishihara
Headers show

Comments

Durham Goode - Feb. 17, 2016, 11:18 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1455750820 28800
#      Wed Feb 17 15:13:40 2016 -0800
# Node ID 5cef3483a31ecd1f18083fc8da3a7fbcd4a22b5b
# Parent  4f99a5826e88fa6b4305336556cc939ca2cdaff1
test: remove dependency on $PYTHONDIR

The PYTHONDIR used in the test may not match the one set in the environment, so
let's remove this depdency and replace it with a glob.

Our internal test automation caught this, since we muck with the PYTHONPATH to
allow testing many of our non-installed extensions together.
timeless - Feb. 18, 2016, 12:24 a.m.
So, I intentionally added PYTHONDIR to try to reduce the number of
globs floating around.

Does this apply to everywhere that you might do this?

I have some plans to try to improve the diff munging, but basically
each time something changes and there's a glob, the user is forced to
deal w/ it.

I'd really rather some other approach than what you're doing here...

Could we introduce a different variable/allow run-tests to know about yours?

On Wed, Feb 17, 2016 at 6:18 PM, Durham Goode <durham@fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1455750820 28800
> #      Wed Feb 17 15:13:40 2016 -0800
> # Node ID 5cef3483a31ecd1f18083fc8da3a7fbcd4a22b5b
> # Parent  4f99a5826e88fa6b4305336556cc939ca2cdaff1
> test: remove dependency on $PYTHONDIR
>
> The PYTHONDIR used in the test may not match the one set in the environment, so
> let's remove this depdency and replace it with a glob.
>
> Our internal test automation caught this, since we muck with the PYTHONPATH to
> allow testing many of our non-installed extensions together.
>
> diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
> --- a/tests/test-devel-warnings.t
> +++ b/tests/test-devel-warnings.t
> @@ -83,27 +83,27 @@
>    $ hg buggylocking --traceback
>    devel-warn: transaction with no lock at:
>     */hg:* in * (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in run (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in dispatch (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcatch (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in _dispatch (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in runcommand (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcommand (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in checkargs (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in <lambda> (glob)
> -   $PYTHONDIR/mercurial/util.py:* in check (glob)
> +   */mercurial/dispatch.py:* in run (glob)
> +   */mercurial/dispatch.py:* in dispatch (glob)
> +   */mercurial/dispatch.py:* in _runcatch (glob)
> +   */mercurial/dispatch.py:* in _dispatch (glob)
> +   */mercurial/dispatch.py:* in runcommand (glob)
> +   */mercurial/dispatch.py:* in _runcommand (glob)
> +   */mercurial/dispatch.py:* in checkargs (glob)
> +   */mercurial/dispatch.py:* in <lambda> (glob)
> +   */mercurial/util.py:* in check (glob)
>     $TESTTMP/buggylocking.py:* in buggylocking (glob)
>    devel-warn: "wlock" acquired after "lock" at:
>     */hg:* in * (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in run (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in dispatch (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcatch (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in _dispatch (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in runcommand (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcommand (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in checkargs (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in <lambda> (glob)
> -   $PYTHONDIR/mercurial/util.py:* in check (glob)
> +   */mercurial/dispatch.py:* in run (glob)
> +   */mercurial/dispatch.py:* in dispatch (glob)
> +   */mercurial/dispatch.py:* in _runcatch (glob)
> +   */mercurial/dispatch.py:* in _dispatch (glob)
> +   */mercurial/dispatch.py:* in runcommand (glob)
> +   */mercurial/dispatch.py:* in _runcommand (glob)
> +   */mercurial/dispatch.py:* in checkargs (glob)
> +   */mercurial/dispatch.py:* in <lambda> (glob)
> +   */mercurial/util.py:* in check (glob)
>     $TESTTMP/buggylocking.py:* in buggylocking (glob)
>    $ hg properlocking
>    $ hg nowaitlocking
> @@ -118,7 +118,7 @@
>    [255]
>
>    $ hg log -r "oldstyle()" -T '{rev}\n'
> -  devel-warn: revset "oldstyle" use list instead of smartset, (upgrade your code) at: $PYTHONDIR/mercurial/revset.py:* (mfunc) (glob)
> +  devel-warn: revset "oldstyle" use list instead of smartset, (upgrade your code) at: */mercurial/revset.py:* (mfunc) (glob)
>    0
>    $ hg oldanddeprecated
>    devel-warn: foorbar is deprecated, go shopping
> @@ -128,14 +128,14 @@
>    devel-warn: foorbar is deprecated, go shopping
>    (compatibility will be dropped after Mercurial-42.1337, update your code.) at:
>     */hg:* in <module> (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in run (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in dispatch (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcatch (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in _dispatch (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in runcommand (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcommand (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in checkargs (glob)
> -   $PYTHONDIR/mercurial/dispatch.py:* in <lambda> (glob)
> -   $PYTHONDIR/mercurial/util.py:* in check (glob)
> +   */mercurial/dispatch.py:* in run (glob)
> +   */mercurial/dispatch.py:* in dispatch (glob)
> +   */mercurial/dispatch.py:* in _runcatch (glob)
> +   */mercurial/dispatch.py:* in _dispatch (glob)
> +   */mercurial/dispatch.py:* in runcommand (glob)
> +   */mercurial/dispatch.py:* in _runcommand (glob)
> +   */mercurial/dispatch.py:* in checkargs (glob)
> +   */mercurial/dispatch.py:* in <lambda> (glob)
> +   */mercurial/util.py:* in check (glob)
>     $TESTTMP/buggylocking.py:* in oldanddeprecated (glob)
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Durham Goode - Feb. 18, 2016, 12:39 a.m.
Our path is derived from the PYTHONPATH we're setting.  We could set 
another environment variable to inform the test runner of what the 
PYTHONDIR should be, but I feel like the tests shouldn't be making 
assumptions about where the tests are running (and don't really need to).

I'm not sure what you're asking with "Does this apply everywhere that 
you might do this?"  Does what apply where?

On 2/17/16 4:24 PM, timeless wrote:
> So, I intentionally added PYTHONDIR to try to reduce the number of
> globs floating around.
>
> Does this apply to everywhere that you might do this?
>
> I have some plans to try to improve the diff munging, but basically
> each time something changes and there's a glob, the user is forced to
> deal w/ it.
>
> I'd really rather some other approach than what you're doing here...
>
> Could we introduce a different variable/allow run-tests to know about yours?
>
> On Wed, Feb 17, 2016 at 6:18 PM, Durham Goode <durham@fb.com> wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1455750820 28800
>> #      Wed Feb 17 15:13:40 2016 -0800
>> # Node ID 5cef3483a31ecd1f18083fc8da3a7fbcd4a22b5b
>> # Parent  4f99a5826e88fa6b4305336556cc939ca2cdaff1
>> test: remove dependency on $PYTHONDIR
>>
>> The PYTHONDIR used in the test may not match the one set in the environment, so
>> let's remove this depdency and replace it with a glob.
>>
>> Our internal test automation caught this, since we muck with the PYTHONPATH to
>> allow testing many of our non-installed extensions together.
>>
>> diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
>> --- a/tests/test-devel-warnings.t
>> +++ b/tests/test-devel-warnings.t
>> @@ -83,27 +83,27 @@
>>     $ hg buggylocking --traceback
>>     devel-warn: transaction with no lock at:
>>      */hg:* in * (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in run (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in dispatch (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcatch (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in _dispatch (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in runcommand (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcommand (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in checkargs (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in <lambda> (glob)
>> -   $PYTHONDIR/mercurial/util.py:* in check (glob)
>> +   */mercurial/dispatch.py:* in run (glob)
>> +   */mercurial/dispatch.py:* in dispatch (glob)
>> +   */mercurial/dispatch.py:* in _runcatch (glob)
>> +   */mercurial/dispatch.py:* in _dispatch (glob)
>> +   */mercurial/dispatch.py:* in runcommand (glob)
>> +   */mercurial/dispatch.py:* in _runcommand (glob)
>> +   */mercurial/dispatch.py:* in checkargs (glob)
>> +   */mercurial/dispatch.py:* in <lambda> (glob)
>> +   */mercurial/util.py:* in check (glob)
>>      $TESTTMP/buggylocking.py:* in buggylocking (glob)
>>     devel-warn: "wlock" acquired after "lock" at:
>>      */hg:* in * (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in run (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in dispatch (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcatch (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in _dispatch (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in runcommand (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcommand (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in checkargs (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in <lambda> (glob)
>> -   $PYTHONDIR/mercurial/util.py:* in check (glob)
>> +   */mercurial/dispatch.py:* in run (glob)
>> +   */mercurial/dispatch.py:* in dispatch (glob)
>> +   */mercurial/dispatch.py:* in _runcatch (glob)
>> +   */mercurial/dispatch.py:* in _dispatch (glob)
>> +   */mercurial/dispatch.py:* in runcommand (glob)
>> +   */mercurial/dispatch.py:* in _runcommand (glob)
>> +   */mercurial/dispatch.py:* in checkargs (glob)
>> +   */mercurial/dispatch.py:* in <lambda> (glob)
>> +   */mercurial/util.py:* in check (glob)
>>      $TESTTMP/buggylocking.py:* in buggylocking (glob)
>>     $ hg properlocking
>>     $ hg nowaitlocking
>> @@ -118,7 +118,7 @@
>>     [255]
>>
>>     $ hg log -r "oldstyle()" -T '{rev}\n'
>> -  devel-warn: revset "oldstyle" use list instead of smartset, (upgrade your code) at: $PYTHONDIR/mercurial/revset.py:* (mfunc) (glob)
>> +  devel-warn: revset "oldstyle" use list instead of smartset, (upgrade your code) at: */mercurial/revset.py:* (mfunc) (glob)
>>     0
>>     $ hg oldanddeprecated
>>     devel-warn: foorbar is deprecated, go shopping
>> @@ -128,14 +128,14 @@
>>     devel-warn: foorbar is deprecated, go shopping
>>     (compatibility will be dropped after Mercurial-42.1337, update your code.) at:
>>      */hg:* in <module> (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in run (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in dispatch (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcatch (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in _dispatch (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in runcommand (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in _runcommand (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in checkargs (glob)
>> -   $PYTHONDIR/mercurial/dispatch.py:* in <lambda> (glob)
>> -   $PYTHONDIR/mercurial/util.py:* in check (glob)
>> +   */mercurial/dispatch.py:* in run (glob)
>> +   */mercurial/dispatch.py:* in dispatch (glob)
>> +   */mercurial/dispatch.py:* in _runcatch (glob)
>> +   */mercurial/dispatch.py:* in _dispatch (glob)
>> +   */mercurial/dispatch.py:* in runcommand (glob)
>> +   */mercurial/dispatch.py:* in _runcommand (glob)
>> +   */mercurial/dispatch.py:* in checkargs (glob)
>> +   */mercurial/dispatch.py:* in <lambda> (glob)
>> +   */mercurial/util.py:* in check (glob)
>>      $TESTTMP/buggylocking.py:* in oldanddeprecated (glob)
>>     $ cd ..
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=SbYWjQoYToRV1j0bT1EswNd47RS_FToZMhSapUB2edw&s=RUWGUHBsVVmZjut38JZu_ILMOhjeByTDutGkY9FP61A&e=
Sean Farley - Feb. 18, 2016, 1:57 a.m.
Durham Goode <durham@fb.com> writes:

> Our path is derived from the PYTHONPATH we're setting.  We could set 
> another environment variable to inform the test runner of what the 
> PYTHONDIR should be, but I feel like the tests shouldn't be making 
> assumptions about where the tests are running (and don't really need to).

This seems perfectly reasonable to me. Why does it matter to reduce
globs? I don't really follow how the user is forced to deal with it in
this case.
timeless - Feb. 18, 2016, 2:16 a.m.
Durham Goode  wrote:
> Our path is derived from the PYTHONPATH we're setting.  We could set another
> environment variable to inform the test runner of what the PYTHONDIR should
> be, but I feel like the tests shouldn't be making assumptions about where
> the tests are running (and don't really need to).

The tests don't need to, but to have a chance at automatically dealing
w/ changes in the test output, it's really helpful for the output to
not require globs.

If we have a line like:

*: 50 (glob)

then when the line becomes
foo/bar/baz: 51

you end up w/ a diff of:

-*: 50 (glob)
+foo/bar/baz: 51

You can't accept the
And you're stuck manually rebuilding any globs (for the example here,
it's incredibly annoying).

OTOH, if the line is:

$PYTHONDIR: 50

Then the output is:
$PYTHONDIR: 51

And you can say "yes" to the -i prompt to accept the changes.
Durham Goode - Feb. 18, 2016, 2:42 a.m.
On 2/17/16 6:16 PM, timeless wrote:
> Durham Goode  wrote:
>> Our path is derived from the PYTHONPATH we're setting.  We could set another
>> environment variable to inform the test runner of what the PYTHONDIR should
>> be, but I feel like the tests shouldn't be making assumptions about where
>> the tests are running (and don't really need to).
> The tests don't need to, but to have a chance at automatically dealing
> w/ changes in the test output, it's really helpful for the output to
> not require globs.
>
> If we have a line like:
>
> *: 50 (glob)
>
> then when the line becomes
> foo/bar/baz: 51
>
> you end up w/ a diff of:
>
> -*: 50 (glob)
> +foo/bar/baz: 51
>
> You can't accept the
> And you're stuck manually rebuilding any globs (for the example here,
> it's incredibly annoying).
>
> OTOH, if the line is:
>
> $PYTHONDIR: 50
>
> Then the output is:
> $PYTHONDIR: 51
>
> And you can say "yes" to the -i prompt to accept the changes.
If we want to reduce the chance of needing to change globs in this test 
case, we could grep away most of the output, and only verify that a 
basic stack trace was produced (like only look for dispatch.run).
Sean Farley - Feb. 18, 2016, 2:59 a.m.
timeless <timeless@gmail.com> writes:

> Durham Goode  wrote:
>> Our path is derived from the PYTHONPATH we're setting.  We could set another
>> environment variable to inform the test runner of what the PYTHONDIR should
>> be, but I feel like the tests shouldn't be making assumptions about where
>> the tests are running (and don't really need to).
>
> The tests don't need to, but to have a chance at automatically dealing
> w/ changes in the test output, it's really helpful for the output to
> not require globs.
>
> If we have a line like:
>
> *: 50 (glob)
>
> then when the line becomes
> foo/bar/baz: 51
>
> you end up w/ a diff of:
>
> -*: 50 (glob)
> +foo/bar/baz: 51
>
> You can't accept the
> And you're stuck manually rebuilding any globs (for the example here,
> it's incredibly annoying).
>
> OTOH, if the line is:
>
> $PYTHONDIR: 50
>
> Then the output is:
> $PYTHONDIR: 51
>
> And you can say "yes" to the -i prompt to accept the changes.

To be fair to Durham, I think getting -i to work with glob is outside
the scope of this patch. I much prefer getting rid of $PYTHONPATH than
getting -i to work with globs.

I could be wrong here, though.
timeless - Feb. 18, 2016, 3:03 a.m.
Getting -i to work is *why* I added this.

He's reverting something which I'd just put right back.

The general approach as I understand it is not to just revert
something w/o planning forward.

On Wed, Feb 17, 2016 at 9:59 PM, Sean Farley <sean@farley.io> wrote:
>
> timeless <timeless@gmail.com> writes:
>
>> Durham Goode  wrote:
>>> Our path is derived from the PYTHONPATH we're setting.  We could set another
>>> environment variable to inform the test runner of what the PYTHONDIR should
>>> be, but I feel like the tests shouldn't be making assumptions about where
>>> the tests are running (and don't really need to).
>>
>> The tests don't need to, but to have a chance at automatically dealing
>> w/ changes in the test output, it's really helpful for the output to
>> not require globs.
>>
>> If we have a line like:
>>
>> *: 50 (glob)
>>
>> then when the line becomes
>> foo/bar/baz: 51
>>
>> you end up w/ a diff of:
>>
>> -*: 50 (glob)
>> +foo/bar/baz: 51
>>
>> You can't accept the
>> And you're stuck manually rebuilding any globs (for the example here,
>> it's incredibly annoying).
>>
>> OTOH, if the line is:
>>
>> $PYTHONDIR: 50
>>
>> Then the output is:
>> $PYTHONDIR: 51
>>
>> And you can say "yes" to the -i prompt to accept the changes.
>
> To be fair to Durham, I think getting -i to work with glob is outside
> the scope of this patch. I much prefer getting rid of $PYTHONPATH than
> getting -i to work with globs.
>
> I could be wrong here, though.
timeless - Feb. 18, 2016, 3:05 a.m.
Durham Goode <durham@fb.com> wrote:
> If we want to reduce the chance of needing to change globs in this test
> case, we could grep away most of the output, and only verify that a basic
> stack trace was produced (like only look for dispatch.run).

I want something that works for this too:
-  devel-warn: revset "oldstyle" use list instead of smartset,
(upgrade your code) at: $PYTHONDIR/mercurial/revset.py:* (mfunc)
(glob)

The regexp to reform PYTHONDIR worked pretty nicely.
Durham Goode - Feb. 18, 2016, 6:07 a.m.
On 2/17/16 7:05 PM, timeless wrote:
> Durham Goode <durham@fb.com> wrote:
>> If we want to reduce the chance of needing to change globs in this test
>> case, we could grep away most of the output, and only verify that a basic
>> stack trace was produced (like only look for dispatch.run).
> I want something that works for this too:
> -  devel-warn: revset "oldstyle" use list instead of smartset,
> (upgrade your code) at: $PYTHONDIR/mercurial/revset.py:* (mfunc)
> (glob)
>
> The regexp to reform PYTHONDIR worked pretty nicely.
I understand the frustration with fixing glob'd lines, but won't these 
lines need manual fixing even with the $PYTHONDIR change? Because the 
line number is glob'd?

Perhaps, instead of hard coding $PYTHONDIR in the tests, you could 
post-process the -i output, and replace any occurrence of the python 
directory path with a glob and add (glob) at the end of the line. That 
way -i works correctly for anyone with a standard python path set up, 
and we're not hard coding expected python paths in the tests.  From 
looking at the code in run-tests.py:addOutputMismatch(), it might be as 
simple as a regex.replace().

Such post processing logic could also eventually recognize 
"file.py:LINE" patterns and auto-glob the LINE as well.
timeless - Feb. 19, 2016, 4:07 a.m.
Durham Goode wrote:
> I understand the frustration with fixing glob'd lines, but won't these lines
> need manual fixing even with the $PYTHONDIR change? Because the line number
> is glob'd?


>
> Perhaps, instead of hard coding $PYTHONDIR in the tests, you could
> post-process the -i output, and replace any occurrence of the python
> directory path with a glob and add (glob) at the end of the line. That way
> -i works correctly for anyone with a standard python path set up, and we're
> not hard coding expected python paths in the tests.  From looking at the
> code in run-tests.py:addOutputMismatch(), it might be as simple as a
> regex.replace().
>
> Such post processing logic could also eventually recognize "file.py:LINE"
> patterns and auto-glob the LINE as well.
timeless - Feb. 19, 2016, 4:11 a.m.
Durham Goode wrote:
> I understand the frustration with fixing glob'd lines, but won't these lines
> need manual fixing even with the $PYTHONDIR change? Because the line number
> is glob'd?

The line number wouldn't need to be globbed, but if it's globbed, then
there shouldn't be a change.

The problem is the combination of the line changing and the file path
not matching and needing to be reglobbed.

> Perhaps, instead of hard coding $PYTHONDIR in the tests, you could
> post-process the -i output, and replace any occurrence of the python
> directory path with a glob and add (glob) at the end of the line.

Would this do the right thing for your system? (I'm not sure I care,
but since that is what caused you to change things, I want to
understand it better)
And is the path here a set of paths, or a single directory-path?

> That way
> -i works correctly for anyone with a standard python path set up, and we're
> not hard coding expected python paths in the tests.  From looking at the
> code in run-tests.py:addOutputMismatch(), it might be as simple as a
> regex.replace().
>
> Such post processing logic could also eventually recognize "file.py:LINE"
> patterns and auto-glob the LINE as well.

That sounds promising. But not this week.
Durham Goode - Feb. 19, 2016, 6:01 p.m.
On 2/18/16, 8:11 PM, "timeless.bmo1@gmail.com on behalf of timeless" <timeless.bmo1@gmail.com on behalf of timeless@gmail.com> wrote:

>Durham Goode wrote:

>> I understand the frustration with fixing glob'd lines, but won't these lines

>> need manual fixing even with the $PYTHONDIR change? Because the line number

>> is glob'd?

>

>The line number wouldn't need to be globbed, but if it's globbed, then

>there shouldn't be a change.

>

>The problem is the combination of the line changing and the file path

>not matching and needing to be reglobbed.


I don't understand.  Currently the lines are:

$PYTHONDIR/mercurial/util.py:* in check (glob)


Previously they were:

*/mercurial/util.py:* in check (glob)


In both cases, the only time you need to worry about -i is if the stack changes (a new stack entry, or the removal of one).  In both cases, using -i will un-glob the the line number and require you to do manual re-globbing, just like you would have to do for the PYTHONDIR before.  So I'm not sure I see PYTHONDIR actually saving much effort, aside from a few vim key strokes to replace the path with a *.

You could say we shouldn't glob the line numbers, but having hard coded line numbers in our tests seems like an even worse practice, since then the test would fail every time you edit dispatch/util/etc.

>

>> Perhaps, instead of hard coding $PYTHONDIR in the tests, you could

>> post-process the -i output, and replace any occurrence of the python

>> directory path with a glob and add (glob) at the end of the line.

>

>Would this do the right thing for your system? (I'm not sure I care,

>but since that is what caused you to change things, I want to

>understand it better)

>And is the path here a set of paths, or a single directory-path?


Nope, this wouldn't work for us.  But I'm fine manually fixing globs every now and then if it just affects us.
Yuya Nishihara - Feb. 20, 2016, 8:32 a.m.
On Wed, 17 Feb 2016 15:18:58 -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1455750820 28800
> #      Wed Feb 17 15:13:40 2016 -0800
> # Node ID 5cef3483a31ecd1f18083fc8da3a7fbcd4a22b5b
> # Parent  4f99a5826e88fa6b4305336556cc939ca2cdaff1
> test: remove dependency on $PYTHONDIR

I'm inclined to drop the PYTHONDIR patch based on cost/benefit ratio discussed
in this thread. timeless, can you rework on it later?

http://hg.netv6.net/clowncopter/rev/2e52e543cdbb
Yuya Nishihara - Feb. 22, 2016, 1:27 p.m.
On Sat, 20 Feb 2016 17:32:12 +0900, Yuya Nishihara wrote:
> On Wed, 17 Feb 2016 15:18:58 -0800, Durham Goode wrote:
> > # HG changeset patch
> > # User Durham Goode <durham@fb.com>
> > # Date 1455750820 28800
> > #      Wed Feb 17 15:13:40 2016 -0800
> > # Node ID 5cef3483a31ecd1f18083fc8da3a7fbcd4a22b5b
> > # Parent  4f99a5826e88fa6b4305336556cc939ca2cdaff1
> > test: remove dependency on $PYTHONDIR
> 
> I'm inclined to drop the PYTHONDIR patch based on cost/benefit ratio discussed
> in this thread. timeless, can you rework on it later?
> 
> http://hg.netv6.net/clowncopter/rev/2e52e543cdbb

I've dropped the patch "tests: replace python dir with $PYTHONDIR in test
output" from the clowncopter. Sorry for the trouble.

Patch

diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t
+++ b/tests/test-devel-warnings.t
@@ -83,27 +83,27 @@ 
   $ hg buggylocking --traceback
   devel-warn: transaction with no lock at:
    */hg:* in * (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in run (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in dispatch (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in _runcatch (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in _dispatch (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in runcommand (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in _runcommand (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in checkargs (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in <lambda> (glob)
-   $PYTHONDIR/mercurial/util.py:* in check (glob)
+   */mercurial/dispatch.py:* in run (glob)
+   */mercurial/dispatch.py:* in dispatch (glob)
+   */mercurial/dispatch.py:* in _runcatch (glob)
+   */mercurial/dispatch.py:* in _dispatch (glob)
+   */mercurial/dispatch.py:* in runcommand (glob)
+   */mercurial/dispatch.py:* in _runcommand (glob)
+   */mercurial/dispatch.py:* in checkargs (glob)
+   */mercurial/dispatch.py:* in <lambda> (glob)
+   */mercurial/util.py:* in check (glob)
    $TESTTMP/buggylocking.py:* in buggylocking (glob)
   devel-warn: "wlock" acquired after "lock" at:
    */hg:* in * (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in run (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in dispatch (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in _runcatch (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in _dispatch (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in runcommand (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in _runcommand (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in checkargs (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in <lambda> (glob)
-   $PYTHONDIR/mercurial/util.py:* in check (glob)
+   */mercurial/dispatch.py:* in run (glob)
+   */mercurial/dispatch.py:* in dispatch (glob)
+   */mercurial/dispatch.py:* in _runcatch (glob)
+   */mercurial/dispatch.py:* in _dispatch (glob)
+   */mercurial/dispatch.py:* in runcommand (glob)
+   */mercurial/dispatch.py:* in _runcommand (glob)
+   */mercurial/dispatch.py:* in checkargs (glob)
+   */mercurial/dispatch.py:* in <lambda> (glob)
+   */mercurial/util.py:* in check (glob)
    $TESTTMP/buggylocking.py:* in buggylocking (glob)
   $ hg properlocking
   $ hg nowaitlocking
@@ -118,7 +118,7 @@ 
   [255]
 
   $ hg log -r "oldstyle()" -T '{rev}\n'
-  devel-warn: revset "oldstyle" use list instead of smartset, (upgrade your code) at: $PYTHONDIR/mercurial/revset.py:* (mfunc) (glob)
+  devel-warn: revset "oldstyle" use list instead of smartset, (upgrade your code) at: */mercurial/revset.py:* (mfunc) (glob)
   0
   $ hg oldanddeprecated
   devel-warn: foorbar is deprecated, go shopping
@@ -128,14 +128,14 @@ 
   devel-warn: foorbar is deprecated, go shopping
   (compatibility will be dropped after Mercurial-42.1337, update your code.) at:
    */hg:* in <module> (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in run (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in dispatch (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in _runcatch (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in _dispatch (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in runcommand (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in _runcommand (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in checkargs (glob)
-   $PYTHONDIR/mercurial/dispatch.py:* in <lambda> (glob)
-   $PYTHONDIR/mercurial/util.py:* in check (glob)
+   */mercurial/dispatch.py:* in run (glob)
+   */mercurial/dispatch.py:* in dispatch (glob)
+   */mercurial/dispatch.py:* in _runcatch (glob)
+   */mercurial/dispatch.py:* in _dispatch (glob)
+   */mercurial/dispatch.py:* in runcommand (glob)
+   */mercurial/dispatch.py:* in _runcommand (glob)
+   */mercurial/dispatch.py:* in checkargs (glob)
+   */mercurial/dispatch.py:* in <lambda> (glob)
+   */mercurial/util.py:* in check (glob)
    $TESTTMP/buggylocking.py:* in oldanddeprecated (glob)
   $ cd ..