Patchwork tests: fix import order in test-bdiff

login
register
mail settings
Submitter via Mercurial-devel
Date Jan. 11, 2017, 8:58 p.m.
Message ID <193e2d6cba6f294dca88.1484168333@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/18183/
State Changes Requested
Headers show

Comments

via Mercurial-devel - Jan. 11, 2017, 8:58 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1484168228 28800
#      Wed Jan 11 12:57:08 2017 -0800
# Node ID 193e2d6cba6f294dca88b939a2a1afa5874a9794
# Parent  9823e2f50a935f6170e01235b65b5282680ebdab
tests: fix import order in test-bdiff

Without this, I see the following failure in
test-check-module-imports.t.

@@ -180,3 +180,5 @@
   > -X tests/test-hgweb-no-request-uri.t \
   > -X tests/test-hgweb-non-interactive.t \
   > | sed 's-\\-/-g' | python "$import_checker" -
+  tests/test-bdiff.py:6: imports not lexically sorted: silenttestrunner < unittest
+  [1]
Gregory Szorc - Jan. 12, 2017, 12:48 a.m.
On Wed, Jan 11, 2017 at 12:58 PM, Martin von Zweigbergk via Mercurial-devel
<mercurial-devel@mercurial-scm.org> wrote:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1484168228 28800
> #      Wed Jan 11 12:57:08 2017 -0800
> # Node ID 193e2d6cba6f294dca88b939a2a1afa5874a9794
> # Parent  9823e2f50a935f6170e01235b65b5282680ebdab
> tests: fix import order in test-bdiff
>
> Without this, I see the following failure in
> test-check-module-imports.t.
>
> @@ -180,3 +180,5 @@
>    > -X tests/test-hgweb-no-request-uri.t \
>    > -X tests/test-hgweb-non-interactive.t \
>    > | sed 's-\\-/-g' | python "$import_checker" -
> +  tests/test-bdiff.py:6: imports not lexically sorted: silenttestrunner <
> unittest
> +  [1]
>
> diff -r 9823e2f50a93 -r 193e2d6cba6f tests/test-bdiff.py
> --- a/tests/test-bdiff.py       Sun Jan 08 00:52:54 2017 +0800
> +++ b/tests/test-bdiff.py       Wed Jan 11 12:57:08 2017 -0800
> @@ -1,10 +1,9 @@
>  from __future__ import absolute_import, print_function
>  import collections
> +import silenttestrunner
>  import struct
>  import unittest
>
> -import silenttestrunner
> -
>  from mercurial import (
>      bdiff,
>      mpatch,
>

Hmmm. This is seemingly a bug in the import checker because
silenttestrunner is not part of the Python standard library.

But, uh, I can't reproduce this failure. If I had to take a guess, it would
be that you have a silenttestrunner module installed in your Python install
and that is confusing the import checker into believing it is part of the
stdlib. Does `import silenttestrunner` work from a Python REPL on your
machine?
via Mercurial-devel - Jan. 12, 2017, 1:36 a.m.
On Wed, Jan 11, 2017 at 4:48 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> On Wed, Jan 11, 2017 at 12:58 PM, Martin von Zweigbergk via Mercurial-devel
> <mercurial-devel@mercurial-scm.org> wrote:
>>
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1484168228 28800
>> #      Wed Jan 11 12:57:08 2017 -0800
>> # Node ID 193e2d6cba6f294dca88b939a2a1afa5874a9794
>> # Parent  9823e2f50a935f6170e01235b65b5282680ebdab
>> tests: fix import order in test-bdiff
>>
>> Without this, I see the following failure in
>> test-check-module-imports.t.
>>
>> @@ -180,3 +180,5 @@
>>    > -X tests/test-hgweb-no-request-uri.t \
>>    > -X tests/test-hgweb-non-interactive.t \
>>    > | sed 's-\\-/-g' | python "$import_checker" -
>> +  tests/test-bdiff.py:6: imports not lexically sorted: silenttestrunner <
>> unittest
>> +  [1]
>>
>> diff -r 9823e2f50a93 -r 193e2d6cba6f tests/test-bdiff.py
>> --- a/tests/test-bdiff.py       Sun Jan 08 00:52:54 2017 +0800
>> +++ b/tests/test-bdiff.py       Wed Jan 11 12:57:08 2017 -0800
>> @@ -1,10 +1,9 @@
>>  from __future__ import absolute_import, print_function
>>  import collections
>> +import silenttestrunner
>>  import struct
>>  import unittest
>>
>> -import silenttestrunner
>> -
>>  from mercurial import (
>>      bdiff,
>>      mpatch,
>
>
> Hmmm. This is seemingly a bug in the import checker because silenttestrunner
> is not part of the Python standard library.

So test-verify-repo-operations.py, test-manifest.py and test-lock.py
are also incorrect in that case?

>
> But, uh, I can't reproduce this failure. If I had to take a guess, it would
> be that you have a silenttestrunner module installed in your Python install
> and that is confusing the import checker into believing it is part of the
> stdlib. Does `import silenttestrunner` work from a Python REPL on your
> machine?

Nope, it fails.
Gregory Szorc - Jan. 13, 2017, 4:59 a.m.
On Wed, Jan 11, 2017 at 5:36 PM, Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Wed, Jan 11, 2017 at 4:48 PM, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
> > On Wed, Jan 11, 2017 at 12:58 PM, Martin von Zweigbergk via
> Mercurial-devel
> > <mercurial-devel@mercurial-scm.org> wrote:
> >>
> >> # HG changeset patch
> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> # Date 1484168228 28800
> >> #      Wed Jan 11 12:57:08 2017 -0800
> >> # Node ID 193e2d6cba6f294dca88b939a2a1afa5874a9794
> >> # Parent  9823e2f50a935f6170e01235b65b5282680ebdab
> >> tests: fix import order in test-bdiff
> >>
> >> Without this, I see the following failure in
> >> test-check-module-imports.t.
> >>
> >> @@ -180,3 +180,5 @@
> >>    > -X tests/test-hgweb-no-request-uri.t \
> >>    > -X tests/test-hgweb-non-interactive.t \
> >>    > | sed 's-\\-/-g' | python "$import_checker" -
> >> +  tests/test-bdiff.py:6: imports not lexically sorted:
> silenttestrunner <
> >> unittest
> >> +  [1]
> >>
> >> diff -r 9823e2f50a93 -r 193e2d6cba6f tests/test-bdiff.py
> >> --- a/tests/test-bdiff.py       Sun Jan 08 00:52:54 2017 +0800
> >> +++ b/tests/test-bdiff.py       Wed Jan 11 12:57:08 2017 -0800
> >> @@ -1,10 +1,9 @@
> >>  from __future__ import absolute_import, print_function
> >>  import collections
> >> +import silenttestrunner
> >>  import struct
> >>  import unittest
> >>
> >> -import silenttestrunner
> >> -
> >>  from mercurial import (
> >>      bdiff,
> >>      mpatch,
> >
> >
> > Hmmm. This is seemingly a bug in the import checker because
> silenttestrunner
> > is not part of the Python standard library.
>
> So test-verify-repo-operations.py, test-manifest.py and test-lock.py
> are also incorrect in that case?
>

They look incorrect to me w.r.t. silenttestrunner being in the stdlib
block. test-verify-repo-operations.py is also not even close to using our
import convention, which is why it is excluded by
test-check-module-imports.t.


>
> >
> > But, uh, I can't reproduce this failure. If I had to take a guess, it
> would
> > be that you have a silenttestrunner module installed in your Python
> install
> > and that is confusing the import checker into believing it is part of the
> > stdlib. Does `import silenttestrunner` work from a Python REPL on your
> > machine?
>
> Nope, it fails.
>

I still can't reproduce your failure.

If I run the import checker manually, I do get a different failure:

$ echo tests/test-bdiff.py | python contrib/import-checker.py -
tests/test-bdiff.py:8: direct symbol import bdiff, mpatch from mercurial

And if I run a command inspired from test-check-module-imports.t:

$ hg locate 'set:**.py or grep(r"^#!.*?python")'  'tests/**.t' | python
contrib/import-checker.py -

I don't see a failure for test-bdiff.py. Wat.

I have a hunch import-checker.py is doing something wonky when processing
multiple files.
Kyle Lippincott - Feb. 13, 2017, 9:33 a.m.
On Thu, Jan 12, 2017 at 8:59 PM, Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> On Wed, Jan 11, 2017 at 5:36 PM, Martin von Zweigbergk <
> martinvonz@google.com> wrote:
>
>> On Wed, Jan 11, 2017 at 4:48 PM, Gregory Szorc <gregory.szorc@gmail.com>
>> wrote:
>> > On Wed, Jan 11, 2017 at 12:58 PM, Martin von Zweigbergk via
>> Mercurial-devel
>> > <mercurial-devel@mercurial-scm.org> wrote:
>> >>
>> >> # HG changeset patch
>> >> # User Martin von Zweigbergk <martinvonz@google.com>
>> >> # Date 1484168228 28800
>> >> #      Wed Jan 11 12:57:08 2017 -0800
>> >> # Node ID 193e2d6cba6f294dca88b939a2a1afa5874a9794
>> >> # Parent  9823e2f50a935f6170e01235b65b5282680ebdab
>> >> tests: fix import order in test-bdiff
>> >>
>> >> Without this, I see the following failure in
>> >> test-check-module-imports.t.
>> >>
>> >> @@ -180,3 +180,5 @@
>> >>    > -X tests/test-hgweb-no-request-uri.t \
>> >>    > -X tests/test-hgweb-non-interactive.t \
>> >>    > | sed 's-\\-/-g' | python "$import_checker" -
>> >> +  tests/test-bdiff.py:6: imports not lexically sorted:
>> silenttestrunner <
>> >> unittest
>> >> +  [1]
>> >>
>> >> diff -r 9823e2f50a93 -r 193e2d6cba6f tests/test-bdiff.py
>> >> --- a/tests/test-bdiff.py       Sun Jan 08 00:52:54 2017 +0800
>> >> +++ b/tests/test-bdiff.py       Wed Jan 11 12:57:08 2017 -0800
>> >> @@ -1,10 +1,9 @@
>> >>  from __future__ import absolute_import, print_function
>> >>  import collections
>> >> +import silenttestrunner
>> >>  import struct
>> >>  import unittest
>> >>
>> >> -import silenttestrunner
>> >> -
>> >>  from mercurial import (
>> >>      bdiff,
>> >>      mpatch,
>> >
>> >
>> > Hmmm. This is seemingly a bug in the import checker because
>> silenttestrunner
>> > is not part of the Python standard library.
>>
>> So test-verify-repo-operations.py, test-manifest.py and test-lock.py
>> are also incorrect in that case?
>>
>
> They look incorrect to me w.r.t. silenttestrunner being in the stdlib
> block. test-verify-repo-operations.py is also not even close to using our
> import convention, which is why it is excluded by
> test-check-module-imports.t.
>
>
>>
>> >
>> > But, uh, I can't reproduce this failure. If I had to take a guess, it
>> would
>> > be that you have a silenttestrunner module installed in your Python
>> install
>> > and that is confusing the import checker into believing it is part of
>> the
>> > stdlib. Does `import silenttestrunner` work from a Python REPL on your
>> > machine?
>>
>> Nope, it fails.
>>
>
> I still can't reproduce your failure.
>
> If I run the import checker manually, I do get a different failure:
>
> $ echo tests/test-bdiff.py | python contrib/import-checker.py -
> tests/test-bdiff.py:8: direct symbol import bdiff, mpatch from mercurial
>
> And if I run a command inspired from test-check-module-imports.t:
>
> $ hg locate 'set:**.py or grep(r"^#!.*?python")'  'tests/**.t' | python
> contrib/import-checker.py -
>
> I don't see a failure for test-bdiff.py. Wat.
>
> I have a hunch import-checker.py is doing something wonky when processing
> multiple files.
>

I think this may be true, but I think isn't the full explanation.  The
reason it's not complaining about the direct symbol import when you run the
full set through is probably because of the multiple files.  If I instead
do `echo tests/test-bdiff.py | python "$import_checker" -` in the test, so
just the one file instead of the full set, I get both issues. I haven't
tracked down what file(s) it locates that hide the bdiff/mpatch issue, but
that's somewhat irrelevant I guess.

I think I'm now able to explain why Martin and I are seeing this and you
are not - we're both working on our Linux workstations, which at Google has
the home directories under /usr/local/google/home/<username>.  Meanwhile,
sys.prefix and sys.exec_prefix are '/usr' on my machine.  So, when
import_checker uses /usr as the stdlib_prefixes [1], we get anything in our
home directories included in those checks.

Since this is, afaict, the only imported thing from tests/ by other things
in tests [2], I wonder if it wouldn't make sense to just skip
silenttestrunner in import-checker by name, or make import-checker
recognize everything under $(dirname $TESTDIR) as not-stdlib.

[1]
https://www.mercurial-scm.org/repo/hg/file/tip/contrib/import-checker.py#l206
[2]
$ for f in $(ls *.py -1 | grep -v 'test-.*.py' | sed 's/.py$//'); do grep
'^import '$f *.py; done
test-bdiff.py:import silenttestrunner
test-ctxmanager.py:import silenttestrunner
test-lock.py:import silenttestrunner
test-manifest.py:import silenttestrunner
test-verify-repo-operations.py:import silenttestrunner


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
Pulkit Goyal - Feb. 13, 2017, 8:34 p.m.
Initially I fixed a lot of such error messages while adding absolute_import
to files. I just send a patch which I guess must prevent the test failure
at Google.
https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-February/092978.html

On Mon, Feb 13, 2017 at 3:03 PM, Kyle Lippincott <spectral@pewpew.net>
wrote:

>
>
> On Thu, Jan 12, 2017 at 8:59 PM, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
>
>> On Wed, Jan 11, 2017 at 5:36 PM, Martin von Zweigbergk <
>> martinvonz@google.com> wrote:
>>
>>> On Wed, Jan 11, 2017 at 4:48 PM, Gregory Szorc <gregory.szorc@gmail.com>
>>> wrote:
>>> > On Wed, Jan 11, 2017 at 12:58 PM, Martin von Zweigbergk via
>>> Mercurial-devel
>>> > <mercurial-devel@mercurial-scm.org> wrote:
>>> >>
>>> >> # HG changeset patch
>>> >> # User Martin von Zweigbergk <martinvonz@google.com>
>>> >> # Date 1484168228 28800
>>> >> #      Wed Jan 11 12:57:08 2017 -0800
>>> >> # Node ID 193e2d6cba6f294dca88b939a2a1afa5874a9794
>>> >> # Parent  9823e2f50a935f6170e01235b65b5282680ebdab
>>> >> tests: fix import order in test-bdiff
>>> >>
>>> >> Without this, I see the following failure in
>>> >> test-check-module-imports.t.
>>> >>
>>> >> @@ -180,3 +180,5 @@
>>> >>    > -X tests/test-hgweb-no-request-uri.t \
>>> >>    > -X tests/test-hgweb-non-interactive.t \
>>> >>    > | sed 's-\\-/-g' | python "$import_checker" -
>>> >> +  tests/test-bdiff.py:6: imports not lexically sorted:
>>> silenttestrunner <
>>> >> unittest
>>> >> +  [1]
>>> >>
>>> >> diff -r 9823e2f50a93 -r 193e2d6cba6f tests/test-bdiff.py
>>> >> --- a/tests/test-bdiff.py       Sun Jan 08 00:52:54 2017 +0800
>>> >> +++ b/tests/test-bdiff.py       Wed Jan 11 12:57:08 2017 -0800
>>> >> @@ -1,10 +1,9 @@
>>> >>  from __future__ import absolute_import, print_function
>>> >>  import collections
>>> >> +import silenttestrunner
>>> >>  import struct
>>> >>  import unittest
>>> >>
>>> >> -import silenttestrunner
>>> >> -
>>> >>  from mercurial import (
>>> >>      bdiff,
>>> >>      mpatch,
>>> >
>>> >
>>> > Hmmm. This is seemingly a bug in the import checker because
>>> silenttestrunner
>>> > is not part of the Python standard library.
>>>
>>> So test-verify-repo-operations.py, test-manifest.py and test-lock.py
>>> are also incorrect in that case?
>>>
>>
>> They look incorrect to me w.r.t. silenttestrunner being in the stdlib
>> block. test-verify-repo-operations.py is also not even close to using our
>> import convention, which is why it is excluded by
>> test-check-module-imports.t.
>>
>>
>>>
>>> >
>>> > But, uh, I can't reproduce this failure. If I had to take a guess, it
>>> would
>>> > be that you have a silenttestrunner module installed in your Python
>>> install
>>> > and that is confusing the import checker into believing it is part of
>>> the
>>> > stdlib. Does `import silenttestrunner` work from a Python REPL on your
>>> > machine?
>>>
>>> Nope, it fails.
>>>
>>
>> I still can't reproduce your failure.
>>
>> If I run the import checker manually, I do get a different failure:
>>
>> $ echo tests/test-bdiff.py | python contrib/import-checker.py -
>> tests/test-bdiff.py:8: direct symbol import bdiff, mpatch from mercurial
>>
>> And if I run a command inspired from test-check-module-imports.t:
>>
>> $ hg locate 'set:**.py or grep(r"^#!.*?python")'  'tests/**.t' | python
>> contrib/import-checker.py -
>>
>> I don't see a failure for test-bdiff.py. Wat.
>>
>> I have a hunch import-checker.py is doing something wonky when processing
>> multiple files.
>>
>
> I think this may be true, but I think isn't the full explanation.  The
> reason it's not complaining about the direct symbol import when you run the
> full set through is probably because of the multiple files.  If I instead
> do `echo tests/test-bdiff.py | python "$import_checker" -` in the test, so
> just the one file instead of the full set, I get both issues. I haven't
> tracked down what file(s) it locates that hide the bdiff/mpatch issue, but
> that's somewhat irrelevant I guess.
>
> I think I'm now able to explain why Martin and I are seeing this and you
> are not - we're both working on our Linux workstations, which at Google has
> the home directories under /usr/local/google/home/<username>.  Meanwhile,
> sys.prefix and sys.exec_prefix are '/usr' on my machine.  So, when
> import_checker uses /usr as the stdlib_prefixes [1], we get anything in our
> home directories included in those checks.
>
> Since this is, afaict, the only imported thing from tests/ by other things
> in tests [2], I wonder if it wouldn't make sense to just skip
> silenttestrunner in import-checker by name, or make import-checker
> recognize everything under $(dirname $TESTDIR) as not-stdlib.
>
> [1] https://www.mercurial-scm.org/repo/hg/file/tip/contrib/
> import-checker.py#l206
> [2]
> $ for f in $(ls *.py -1 | grep -v 'test-.*.py' | sed 's/.py$//'); do grep
> '^import '$f *.py; done
> test-bdiff.py:import silenttestrunner
> test-ctxmanager.py:import silenttestrunner
> test-lock.py:import silenttestrunner
> test-manifest.py:import silenttestrunner
> test-verify-repo-operations.py:import silenttestrunner
>
>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>

Patch

diff -r 9823e2f50a93 -r 193e2d6cba6f tests/test-bdiff.py
--- a/tests/test-bdiff.py	Sun Jan 08 00:52:54 2017 +0800
+++ b/tests/test-bdiff.py	Wed Jan 11 12:57:08 2017 -0800
@@ -1,10 +1,9 @@ 
 from __future__ import absolute_import, print_function
 import collections
+import silenttestrunner
 import struct
 import unittest
 
-import silenttestrunner
-
 from mercurial import (
     bdiff,
     mpatch,