[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Mark Lawrence]

Changes by Mark Lawrence :


--
type:  -> behavior
versions: +Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Roumen Petrov]

Roumen Petrov added the comment:

Hi Ned,

> Ned Jackson Lovely added the comment:
[SNIP]
> 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?

$ 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)
[GCC 4.7.2] on win32
---
$ 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")

 @unittest.skipUnless(os.path.exists('/bin/sh'), 'requires /bin/sh')
 def test_os_popen_iter(self):
 with os.popen(
 "/bin/sh -c 'echo \"line1\nline2\nline3\"'") as popen:
 it = iter(popen)
 self.assertEqual(next(it), "line1\n")
 self.assertEqual(next(it), "line2\n")
 self.assertEqual(next(it), "line3\n")
 self.assertRaises(StopIteration, next, it)


result:

test_update (test.test_os.EnvironTests) ... ok
test_update2 (test.test_os.EnvironTests) ... ok
test_update3 (test.test_os.EnvironTests) ... ok
test_values (test.test_os.EnvironTests) ... ok


So with both (take2&take3) updates tests pass. Should work with MSVC builds.

May be test_os_popen_iter could be updated .

> Regards,
>
> Ned

--
nosy: +rpetrov

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

Re: [issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Roumen Petrov]

Hi Ned,


Ned Jackson Lovely added the comment:

[SNIP]

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?



$ 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)
[GCC 4.7.2] on win32
---
$ 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")

@unittest.skipUnless(os.path.exists('/bin/sh'), 'requires /bin/sh')
def test_os_popen_iter(self):
with os.popen(
"/bin/sh -c 'echo \"line1\nline2\nline3\"'") as popen:
it = iter(popen)
self.assertEqual(next(it), "line1\n")
self.assertEqual(next(it), "line2\n")
self.assertEqual(next(it), "line3\n")
self.assertRaises(StopIteration, next, it)


result:

test_update (test.test_os.EnvironTests) ... ok
test_update2 (test.test_os.EnvironTests) ... ok
test_update3 (test.test_os.EnvironTests) ... ok
test_values (test.test_os.EnvironTests) ... ok


So with both (take2&take3) updates tests pass. Should work with MSVC 
builds.



May be test_os_popen_iter could be updated .


Regards,

Ned



___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Ned Jackson Lovely]

Ned Jackson Lovely added the comment:

Hi Luke,

I've prepared two versions of this test. The first, issue5051-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

--
Added file: http://bugs.python.org/file29481/issue5051-take3.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Ned Jackson Lovely]

Changes by Ned Jackson Lovely :


Added file: http://bugs.python.org/file29480/issue5051-take2.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[R. David Murray]

R. David Murray added the comment:

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.

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Luke Kenneth Casson Leighton]

Luke Kenneth Casson Leighton added the comment:

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:
http://msdn.microsoft.com/en-gb/library/aa246422%28v=vs.60%29.aspx

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!

* the ideal test is to actually compile up some c code.  that would however 
mean shipping a binary with the python distribution as part of the test suite 
[eek! there must be other solutions...]

* the less-than-ideal test is to use a pre-existing application, but then you 
have to work out, on a per-supported-platform basis, an OS-specific method of 
echoing an environment variable

* the least perfect method is to skip the test entirely on platforms that don't 
have /bin/sh, but then you have the situation where the regression tests aren't 
actually doing their job, thus defeating the goal of python being a 
platform-independent environment because you can't rely on code working across 
all platforms.

tricky one!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Ned Jackson Lovely]

Ned Jackson Lovely added the comment:

Fair enough Luke. What is your recommended fix?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Luke Kenneth Casson Leighton]

Luke Kenneth Casson Leighton added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Ned Jackson Lovely]

Ned Jackson Lovely added the comment:

Check if sys.platform == 'win32', if so, skip test.

--
keywords: +patch
nosy: +n
versions: +Python 3.4 -Python 2.7
Added file: http://bugs.python.org/file29449/issue5051.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Gabriel Genellina]

Gabriel Genellina  added the comment:

I *did* have /bin/sh in a Windows box some time ago.
Probably the test should check sys.platform in addition to /bin/sh 
existence.

--
nosy: +gagenellina

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Martin v. Löwis]

Martin v. Löwis  added the comment:

The test actually doesn't rely on COMSPEC. On systems that have /bin/sh,
COMSPEC should have no relevance.

--
nosy: +loewis

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue5051] test_update2 in test_os.py invalid due to os.environ.clear() followed by reliance on environ COMSPEC

[Luke Kenneth Casson Leighton]

New submission from Luke Kenneth Casson Leighton :

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):
   vv
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")
   v
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
posixmodule.c.

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
COMSPEC to obtain a working cmd.exe or command.com in order to
execute the /bin/sh (or /bin/sh.exe in the case of having installed
msys and created a symlink from c:/msys/bin to c:/bin).

--
components: Tests
messages: 80502
nosy: lkcl
severity: normal
status: open
title: test_update2 in test_os.py invalid due to os.environ.clear() followed by 
reliance on environ COMSPEC
versions: Python 2.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com