New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC #49301
Comments
class EnvironTests(mapping_tests.BasicTestMappingProtocol):
"""check that os.environ object conform to mapping protocol"""
type2test = None
def _reference(self):
return {"KEY1":"VALUE1", "KEY2":"VALUE2", "KEY3":"VALUE3"}
def _empty_mapping(self):
vvvvvv
os.environ.clear() <<<<<---------- *******
^^^^^^
return os.environ
def setUp(self):
self.__save = dict(os.environ)
os.environ.clear()
def tearDown(self):
os.environ.clear()
os.environ.update(self.__save)
# Bug 1110478
def test_update2(self):
if os.path.exists("/bin/sh"):
os.environ.update(HELLO="World")
vvvvv
value = os.popen("/bin/sh -c 'echo $HELLO'").read().strip()
^^^^^ finds os.environ['COMSPEC'] to be empty!
self.assertEquals(value, "World") this test will obviously fail, see _PyPopenCreateProcess in if (i = GetEnvironmentVariable("COMSPEC",NULL,0)) {
char *comshell;
}
/* Could be an else here to try cmd.exe / command.com in the path
Now we'll just error out.. */
else {
PyErr_SetString(PyExc_RuntimeError,
"Cannot locate a COMSPEC environment variable to "
"use as the shell");
return FALSE;
} the environment variables having been destroyed, there _is_ no |
The test actually doesn't rely on COMSPEC. On systems that have /bin/sh, |
I *did* have /bin/sh in a Windows box some time ago. |
Check if sys.platform == 'win32', if so, skip test. |
that's not the correct solution, ned. what that will do is when someone runs a combination of python and MSYS under wine, the test will be skipped incorrectly. thanks to the work that i did back in 2009, wine has now been improved significantly and has been capable of running python correctly for over 3 years now. i use it to do testing of pyjamas-desktop. bottom line: skipping regression tests by making assumptions based solely and exclusively on the platform type is not good news. |
Fair enough Luke. What is your recommended fix? |
hi ned, well, the situation surrounding the bug-reporting that i was doing at the time was a general campaign of "this person is obviously wasting our time because they're developing yet another port/platform, that is obviously a waste of our time, therefore we will shut him down and ban him from the bugtracker and thus generally make our lives easier". so against that background i really wasn't inclined to contribute to the development of python. the argument given for this specific bug [off-list because i had been ordered not to use the bugtracker] was "it's not a supported combination therefore obviously it's not a bug". leaving that aside, and assuming that things have moved on from there and that improvements to python and its expansion into areas beyond where it is currently entrenched are welcomed, the arguments given 3 years ago are actually valid... but from a different perspective: the combination of win32 and bash is particularly weird [i.e. borderline] and thus the test is incomplete. so it would be best i feel to consider what the test is trying to achieve: set environment variables, and then execute a command that results in that environment variable being passed and read back with python's popen command. in the POSIX case, the easiest way to do that would involve using /bin/sh - a pretty standard baseline application that can be expected to exist on every POSIX platform under the sun. but the most "generic" case would actually involve compiling a small c application which read - in c - a standard environment variable - and printf'd it then exited. a quick google search "c getenv" shows a perfect example: so the *ideal* situation i feel would be to use a shortened version of that, with the env variable "HELLO" printf'd out. if python popen returns "world", the test can be deemed to be successful. the less-than-ideal situation would be, rather than to skip the test, to use command.com or cmd.exe under win32 rather than executing /bin/sh... ... but the irony is that even if you did that, you would run into the same bug because the os.environ.clear() destroys the COMSPEC environment variable! soooOoo.. to fix that, you'd need to record the COMSPEC environment variable just before the os.environ.clear() and re-establish it prior to the popen. so it's a tricky one!
tricky one! |
Well, we are currently skipping the test if /bin/sh doesn't exist. We do have an existing runnable binary used only for testing that is compiled as part of the python build, so that isn't a completely crazy idea. I don't think that it gets installed anywhere, though, since the test that uses it is run only when tests are run in a checkout, not from an installed Python. |
Hi Luke, I've prepared two versions of this test. The first, bpo-5051-take2.diff, retains the environ.clear(), but saves and sets COMSPEC and PATH in the same update call as the "HELLO" variable. The second, and in my opinion more reasonable test, makes sure that "HELLO" isn't already set, sets it using update, then makes sure it is set properly. In both cases, the currently running python executable, fetched via sys.executable and run using os.popen, is used to print the value, instead of the shell's echo. This moves things closer towards cross-platform niceness, and removes the dependency on /bin/sh. Unfortunately, I don't have a Windows machine readily available to test this on. Could apply your preferred patch, run it for me, and let me know if you have any problems? Regards, Ned |
Hi Ned,
$ uname -a
MINGW32_NT-5.1 QEMU 1.0.18(0.48/3/2) 2012-11-21 22:34 i686 Msys Python 3.4.0a0 (default, Mar 20 2013, 00:32:43) $ cat ...test_os.py
....
# Bug 1110478
def test_update2(self):
minimal_environ_keys = ('COMSPEC', 'PATH',)
minimal_environ = {k:os.environ[k] for k in minimal_environ_keys
if k in os.environ}
os.environ.clear()
os.environ.update(HELLO="World")
minimal_environ['HELLO'] = "World"
os.environ.update(minimal_environ)
python_cmd = "{0} -c \"import os;print(os.environ['HELLO'])\""
with os.popen(python_cmd.format(sys.executable)) as popen:
value = popen.read().strip()
self.assertEqual(value, "World") # Bug 1110478
def test_update3(self):
self.assertTrue('HELLO' not in os.environ)
os.environ.update(HELLO="World")
python_cmd = "{0} -c \"import os;print(os.environ['HELLO'])\""
with os.popen(python_cmd.format(sys.executable)) as popen:
value = popen.read().strip()
self.assertEqual(value, "World")
.... result: So with both (take2&take3) updates tests pass. Should work with MSVC builds. May be test_os_popen_iter could be updated .
|
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: