Re: [Python-checkins] python/dist/src/Lib/email/test test_email_codecs.py,1.1,1.2
[Diverting to python-dev... -BAW] timmie> Update of /cvsroot/python/python/dist/src/Lib/email/test timmie> In directory timmie> usw-pr-cvs1:/tmp/cvs-serv3289/python/lib/email/test | Modified Files: | test_email_codecs.py | Log Message: | Changed import from | from test.test_support import TestSkipped, run_unittest | to | from test_support import TestSkipped, run_unittest timmie> Otherwise, if the Japanese codecs aren't installed, timmie> regrtest doesn't believe the TestSkipped exception raised timmie> by this test matches the timmie> except (ImportError, test_support.TestSkipped), msg: timmie> it's looking for, and reports the skip as a crash failure timmie> instead of as a skipped test. timmie> I suppose this will make it harder to run this test timmie> outside of regrtest, but under the assumption only Barry timmie> does that, better to make it skip cleanly for everyone timmie> else. A better fix, IMO, is to recognize that the `test' package has become a full fledged standard lib package (a Good Thing, IMO), heed our own admonitions not to do relative imports, and change the various places in the test suite that "import test_support" (or equiv) to "import test.test_support" (or equiv). I've twiddled the test suite to do things this way, and all the (expected Linux) tests pass, so I'd like to commit these changes. Unit test writers need to remember to use test.test_support instead of just test_support. We could do something wacky like remove '' from sys.path if we really cared about enforcing this. It would also be good for folks on other systems to make sure I haven't missed a module. -Barry
[Barry]
A better fix, IMO, is to recognize that the `test' package has become a full fledged standard lib package (a Good Thing, IMO), heed our own admonitions not to do relative imports, and change the various places in the test suite that "import test_support" (or equiv) to "import test.test_support" (or equiv).
I've twiddled the test suite to do things this way, and all the (expected Linux) tests pass, so I'd like to commit these changes. Unit test writers need to remember to use test.test_support instead of just test_support. We could do something wacky like remove '' from sys.path if we really cared about enforcing this. It would also be good for folks on other systems to make sure I haven't missed a module.
Note test/README, which says in part: """ NOTE: Always import something from test_support like so: from test_support import verbose or like so: import test_support ... use test_support.verbose in the code ... Never import anything from test_support like this: from test.test_support import verbose "test" is a package already, so can refer to modules it contains without "test." qualification. If you do an explicit "test.xxx" qualification, that can fool Python into believing test.xxx is a module distinct from the xxx in the current package, and you can end up importing two distinct copies of xxx. This is especially bad if xxx=test_support, as regrtest.py can (and routinely does) overwrite its "verbose" and "use_large_resources" attributes: if you get a second copy of test_support loaded, it may not have the same values for those as regrtest intended. """ I don't have a deep understanding of these miserable issues, so settled for a one-line patch that worked. The admonition to never import from test.test_support was a BDFL Pronouncement at the time. Note that Jack runs tests in ways nobody else does, via importing something or other from an interactive Python session (Mac Classic doesn't have a cmdline shell -- something like that). It's always an adventure trying to guess how things will break for him, although I'm not sure your suggestion is (or isn't) relevant to Jack. I imagine things will work provided that all imports "are the same". I'm not sure fiddling all the code is worth it just to save a line of typing in the email package's test suite.
"TP" == Tim Peters <tim.one@comcast.net> writes:
TP> Note test/README, which says in part: TP> """ TP> NOTE: Always import something from test_support like so: TP> from test_support import verbose TP> or like so: | import test_support | ... use test_support.verbose in the code ... TP> Never import anything from test_support like this: TP> from test.test_support import verbose TP> "test" is a package already, so can refer to modules it TP> contains without "test." qualification. If you do an explicit TP> "test.xxx" qualification, that can fool Python into believing TP> test.xxx is a module distinct from the xxx in the current TP> package, and you can end up importing two distinct copies of TP> xxx. This is especially bad if xxx=test_support, as TP> regrtest.py can (and routinely does) overwrite its "verbose" TP> and "use_large_resources" attributes: if you get a second copy TP> of test_support loaded, it may not have the same values for TP> those as regrtest intended. """ Yep, but I think those recommendations are out-of-date. You added them to the file almost 2 years ago. ;) Note that the warnings in that README go away when regrtest also imports test_support from the test package. TP> I don't have a deep understanding of these miserable issues, TP> so settled for a one-line patch that worked. The admonition TP> to never import from test.test_support was a BDFL TP> Pronouncement at the time. Hmm, I don't know if he considers that admonition to still be in effect, but I'd like to hope not. We're discouraging relative imports these days, and I don't see any deep reason why the regression tests need to break this rule to function (and indeed, on Unix at least it doesn't seem to). TP> Note that Jack runs tests in ways nobody else does, via TP> importing something or other from an interactive Python TP> session (Mac Classic doesn't have a cmdline shell -- something TP> like that). It's always an adventure trying to guess how TP> things will break for him, although I'm not sure your TP> suggestion is (or isn't) relevant to Jack. I wouldn't presume to know! So I'll generate a patch, upload it to SF, and assign it to Jack for review. TP> I imagine things will work provided that all imports "are the TP> same". Yes. TP> I'm not sure fiddling all the code is worth it just to TP> save a line of typing in the email package's test suite. It's a bit uglier than that because since Lib/test gets magically added to sys.path during regrtest by virtue of running "python Lib/test/regrtest.py". So to find the "same" test_support module, you'd probably have to do something more along the lines of
import os import test.regrtest testdir = os.path.dirname(test.regrtest.__file__) sys.path.insert(0, testdir) import test_support
blechi-ly y'rs, -Barry
It's a bit uglier than that because since Lib/test gets magically added to sys.path during regrtest by virtue of running "python Lib/test/regrtest.py".
Perhaps regrtest.py can specifically remove its own directory from sys.path? (Please don't just remove sys.path[0] or ''; look in sys.argv[0] and deduce from there.) --Guido van Rossum (home page: http://www.python.org/~guido/)
"GvR" == Guido van Rossum <guido@python.org> writes:
>> It's a bit uglier than that because since Lib/test gets >> magically added to sys.path during regrtest by virtue of >> running "python Lib/test/regrtest.py". GvR> Perhaps regrtest.py can specifically remove its own directory GvR> from sys.path? (Please don't just remove sys.path[0] or ''; GvR> look in sys.argv[0] and deduce from there.) Good idea: -------------------- snip snip -------------------- mydir = os.path.dirname(sys.argv[0]) sys.path.remove(mydir) -------------------- snip snip -------------------- I also followed up to Guido privately, re: the motivation for this change. Also, Neal's right, I missed some of the relative imports of test_support and I'm ready to commit those fixes once Guido gives the go ahead. -Barry
A better fix, IMO, is to recognize that the `test' package has become a full fledged standard lib package (a Good Thing, IMO), heed our own admonitions not to do relative imports, and change the various places in the test suite that "import test_support" (or equiv) to "import test.test_support" (or equiv).
Good idea.
I've twiddled the test suite to do things this way, and all the (expected Linux) tests pass, so I'd like to commit these changes.
You've done this by now, right? Fine.
Unit test writers need to remember to use test.test_support instead of just test_support. We could do something wacky like remove '' from sys.path if we really cared about enforcing this. It would also be good for folks on other systems to make sure I haven't missed a module.
Perhaps it would be a good idea for test_support (and perhaps some other crucial testing support modules) to add something at the top like this? if __name__ != "test.test_support": raise ImportError, "test_support must be imported from the test package" --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido van Rossum wrote:
A better fix, IMO, is to recognize that the `test' package has become a full fledged standard lib package (a Good Thing, IMO), heed our own admonitions not to do relative imports, and change the various places in the test suite that "import test_support" (or equiv) to "import test.test_support" (or equiv).
Good idea.
Shouldn't this also be done for from XXX import YYY? grep test_support `find Lib -name '*.py'` | \ egrep -v '(from test |test\.test_support)' | grep import Neal
[Barry]
A better fix, IMO, is to recognize that the `test' package has become a full fledged standard lib package (a Good Thing, IMO), heed our own admonitions not to do relative imports, and change the various places in the test suite that "import test_support" (or equiv) to "import test.test_support" (or equiv).
[Guido]
Good idea.
[Neal]
Shouldn't this also be done for from XXX import YYY?
grep test_support `find Lib -name '*.py'` | \ egrep -v '(from test |test\.test_support)' | grep import
Good catch! Looks like Barry hardly scratched the surface of this. I *thought* his checkin which claimed to fix this throughout Lib/test was a tad small. :-( --Guido van Rossum (home page: http://www.python.org/~guido/)
[Neal]
Shouldn't this also be done for from XXX import YYY?
grep test_support `find Lib -name '*.py'` | \ egrep -v '(from test |test\.test_support)' | grep import
[me]
Good catch! Looks like Barry hardly scratched the surface of this. I *thought* his checkin which claimed to fix this throughout Lib/test was a tad small. :-(
Neal, Barry: on second thought, DON'T FIX THIS YET! I'd like to have a discussion with Barry about the motivation for this. I need to at least understand why Barry thinks he needs this, and reconcile this with my earlier position that relative imports were compulsory in this case. --Guido van Rossum (home page: http://www.python.org/~guido/)
participants (4)
-
barry＠zope.com -
Guido van Rossum -
Neal Norwitz -
Tim Peters