classification
Title: test_site fails with -S flag
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: zach.ware Nosy List: Arfrever, python-dev, r.david.murray, vajrasky, zach.ware
Priority: normal Keywords: patch

Created on 2013-11-29 10:02 by vajrasky, last changed 2013-12-11 23:03 by zach.ware. This issue is now closed.

Files
File name Uploaded Description Edit
fix_test_site_with_s_flag.patch vajrasky, 2013-11-29 10:02 review
fix_test_site_with_s_flag_v2.patch vajrasky, 2013-12-01 14:25 Updated patch according to Zachary's review. review
issue19828.diff zach.ware, 2013-12-11 21:30 review
Messages (11)
msg204711 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-11-29 10:02
$ ./python -S Lib/test/test_site.py 
Traceback (most recent call last):
  File "Lib/test/test_site.py", line 28, in <module>
    raise unittest.SkipTest("importation of site.py suppressed")
unittest.case.SkipTest: importation of site.py suppressed

This counts as fail test. So when you execute the whole unit test:

$ ./python -S Lib/test

You'll get 2 fail tests.

The first one is test_trace, already fixed in #19398, but not yet committed.

The second one is this one, test_site.

'raise unittest.SkipTest("importation of site.py suppressed")' will make the file counts as fail test.

Attached the patch to make it does not count as fail test.
msg204787 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-11-30 08:52
See also issue 1674555, which aims to make test_site run without -S and everything else run with -S.  I think this issue is invalid, if I understand what you wrote correctly, since test_site *should* be reported as a skipped test if -S is specified.
msg204922 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-12-01 14:25
Before patch:
$ ./python -S Lib/test/test_site.py 
Traceback (most recent call last):
  File "Lib/test/test_site.py", line 28, in <module>
    raise unittest.SkipTest("importation of site.py suppressed")
unittest.case.SkipTest: importation of site.py suppressed

After patch:
$ ./python -S Lib/test/test_site.py 
sssssssssssssssssssss
----------------------------------------------------------------------
Ran 21 tests in 0.003s

OK (skipped=21)

With this patch, ./python -S Lib/test will report test_site as *skipped* not *failed*.

My grand plan is to make ./python -S Lib/test successful.
msg204945 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-12-01 17:33
That's not relevant, though.  In fact, the existing error message for running the module directly is exactly correct: the entire test module fails to run, which is a more accurate reflection of the reality than showing all of the individual tests as skipped.  

If you run test_site under the test harness you also currently get the correct result:

rdmurray@hey:~/python/p34>./python -S Lib/test test_site
[1/1] test_site
test_site skipped -- importation of site.py suppressed
1 test skipped:
    test_site

In other words, you don't need to do anything to test_site to get it to work correctly for your desired goal, it already does.
msg204993 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-12-02 09:33
Yeah, but...

$ ./python -S Lib/test
== CPython 3.4.0b1 (default:373797990f57, Dec 2 2013, 16:46:35) [GCC 4.7.2]
==   Linux-3.5.0-37-generic-x86_64-with-debian-wheezy-sid little-endian
==   hash algorithm: siphash24 64bit
==   /home/ethan/Documents/code/python/cpython3.4/build/test_python_28533
Testing with flags: sys.flags(debug=0, inspect=0, interactive=0, optimize=0, dont_write_bytecode=0, no_user_site=0, no_site=1, ignore_environment=0, verbose=0, bytes_warning=0, quiet=0, hash_randomization=1, isolated=0)
[  1/387] test_grammar
[  2/387] test_opcodes
[  3/387] test_dict
...
[385/387/2] test_zipimport
[386/387/2] test_zipimport_support
[387/387/2] test_zlib
361 tests OK.
2 tests failed:
    test_site test_trace
2 tests altered the execution environment:
    test_idle test_importlib
22 tests skipped:
    test_codecmaps_cn test_codecmaps_hk test_codecmaps_jp
    test_codecmaps_kr test_codecmaps_tw test_curses test_devpoll
    test_gdb test_kqueue test_msilib test_ossaudiodev test_smtpnet
    test_socketserver test_startfile test_timeout test_tk
    test_ttk_guionly test_urllibnet test_winreg test_winsound
    test_xmlrpc_net test_zipfile64

Feel free to close this as wontfix. This issue is not a life and death situation anyway. :)
msg204994 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-12-02 09:39
Sorry to forget to add the relevant detail:

$ ./python -S Lib/test
....
[284/387] test_shutil
[285/387] test_signal
[286/387] test_site
test test_site failed -- multiple errors occurred; run in verbose mode for details
[287/387/1] test_slice
[288/387/1] test_smtpd
[289/387/1] test_smtplib
....
msg205811 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-12-10 13:36
The real issue here is that the test used to determine whether -S was passed or not is outdated: instead of checking sys.flags.no_site, it checks whether 'site' is in sys.modules.  This is no longer a valid test, since site's side effects are contained within site.main, which is only run if no_site is False.  In fact, distutils imports symbols from site unconditionally, as do a couple of test modules; hence why test_site currently fails running `python -S Lib/test`: test_distutils (and others) run before test_site and cause site to be present in sys.modules.

I think we should move away from a toplevel SkipTest, though; some of the tests may be applicable whether -S is passed or not, though of course the ImportSideEffectTests would not be.
msg205926 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-12-11 21:30
Looking more closely, there's not much that looks like it really should be tested if explicitly called with -S, so perhaps the toplevel skip is still best after all.  Any other thoughts?

Attached patch fixes `python -S Lib/test` and cleans up an unused import. I'm a bit iffy on the added comment.
msg205928 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-12-11 21:43
I would just change the comment to "it is not useful to run the tests if we were called with -S".  Otherwise it looks fine.

If someone adds tests that are meaningful with -S is specified, we can put them in a different class, and move the skip to the class level.  You could mention that in the comment as well.
msg205938 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-11 23:01
New changeset 40884256f8dd by Zachary Ware in branch '3.3':
Issue #19828: Fixed test_site when the whole suite is run with -S.
http://hg.python.org/cpython/rev/40884256f8dd

New changeset 90834ad91d56 by Zachary Ware in branch 'default':
Issue #19828: Merge with 3.3
http://hg.python.org/cpython/rev/90834ad91d56
msg205939 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-12-11 23:03
Fixed.  Thank you Vajrasky for raising the issue, and David for the much better wording.
History
Date User Action Args
2013-12-11 23:03:35zach.waresetstatus: open -> closed
versions: + Python 3.3
messages: + msg205939

assignee: zach.ware
resolution: fixed
stage: resolved
2013-12-11 23:01:34python-devsetnosy: + python-dev
messages: + msg205938
2013-12-11 21:43:47r.david.murraysetmessages: + msg205928
2013-12-11 21:30:46zach.waresetfiles: + issue19828.diff

messages: + msg205926
2013-12-10 13:36:55zach.waresetmessages: + msg205811
2013-12-02 09:39:08vajraskysetmessages: + msg204994
2013-12-02 09:33:36vajraskysetmessages: + msg204993
2013-12-01 17:33:04r.david.murraysetmessages: + msg204945
2013-12-01 14:25:58vajraskysetfiles: + fix_test_site_with_s_flag_v2.patch

messages: + msg204922
2013-11-30 08:52:47r.david.murraysetnosy: + r.david.murray
messages: + msg204787
2013-11-29 21:05:29Arfreversetnosy: + Arfrever
2013-11-29 14:57:10zach.waresetnosy: + zach.ware
2013-11-29 10:02:31vajraskycreate