Fix test_pytestrunner_start

[StefRe]

by patching add_pathlist_to_PYTHONPATH. This function first checks whether PYTHONPATH is set in the actual (!) OS environment and then changes the passed-in env list by either setting or prepending the pathlist to PYTHONPATH. The test failed if the OS environment in that the test is run hasn't set PYTHONPATH, as in the test, we call add_pathlist_to_PYTHONPATH with an env list where PYTHONPATH is already set.

I'm not sure if this isn't a logic flaw in add_pathlist_to_PYTHONPATH: I think if ipyconsole is False it should look for PYTHONPATH not in the actual OS environment but in the passed-in env list. This doesn't play any role in the production code in the unittest plugin as we call add_pathlist_to_PYTHONPATH almost immediately after creating the new process, so its inherited environment will almost certainly correspond to the actual environment at the moment when add_pathlist_to_PYTHONPATH is being called .

[StefRe]

@jitseniesen I suggest reviewing/merging this PR only after #151 in order to avoid the need for rebasing as both this and #151 are based on the same commit.

[jitseniesen]

Thanks for looking into this. I asked what add_pathlist_to_PYTHONPATH is supposed to do. However, this is old code so I am not sure anybody remembers.

In case we don't get clarification on what the function is supposed to do, perhaps we should not rely on it in our plugin and instead write code ourselves to add the PYTHONPATH to the environment; what do you think?

[StefRe]

perhaps we should not rely on it in our plugin and instead write code ourselves to add the PYTHONPATH to the environment

Yes, this was my thought too. I'd get rid of this function and replace it by a more elegant solution. I'll prepare a PR shortly.

(I just took care of it because of the TODO comment).

[StefRe]

Refactor _prepare_process

• simplify the way pythonpath is inserted into the process environment
• get rid of external dependency add_pathlist_to_PYTHONPATH
• follow advice at https://doc.qt.io/qt-5/qprocessenvironment.html#toStringList to not use the list obtained from toStringList with setEnvironment:

The QStringList contents returned by this function are suitable for presentation. Use with the QProcess::setEnvironment function is not recommended due to potential encoding problems under Unix, and worse performance.

• it must be if pythonpath instead of if pythonpath is not None as
    pythonpath is a list and if it's empty it's still not None so we
    would insert a variable PYTHONPATH without a value into the environment

Fix test in unittestgui.py

pythonpath must be a list of strings, not a string. For instance, if it's a string 'abc', then 'a:b:c' would be prepended to the value of PYTHONPATH. The program works fine, however, in this case too and even when we don't set pythonpath of the widget.

Split test_pytestrunner_start into separate unit tests

test_pytestrunner_start was rather complex and tested not only start itself but also create_argument_list as well as _prepare_process and start of the base class. This was split into four separate unit tests for the involved individual pytestrunner and runnerbase functions.
(as usual - at least for me - writing the tests took far more time than fixing the code itself)

[jitseniesen]

as usual - at least for me - writing the tests took far more time than fixing the code itself

Me too! That is probably why I ended up with the monster test; sorry about that.

So the code is actually simpler when not using add_pathlist_to_PYTHONPATH, nice!

One question, since you got rid of to_text_string(). This function handles the difference between str and unicode types in Python 2. Did you test whether everything did still work under Python 2? This is not covered by automatic testing.

If you don't want to be bothered about Python 2, that is fine with me; I'm also tired of it. But I don't want to break Python 2 in between versions 0.4.0 and 0.4.1 of the plugin, so I will then postpone this PR to version 0.5 of the plugin. Since there should not be any user-visible changes, that is not a big deal for me, but it does mean that it will take longer before your changes make their way to the users.

[StefRe]

One question, since you got rid of to_text_string(). This function handles the difference between str and unicode types in Python 2. Did you test whether everything did still work under Python 2? This is not covered by automatic testing.

Hmm, how can I test this? Just by running the tests from python 2 or do I need to actually run spyder with the plugin from python 2. If the latter then how can I do that? I guess I need to use test cases where pythonpath contains non-ASCII characters, right?

As for the code:

            env = QProcessEnvironment.systemEnvironment()
            old_python_path = env.value('PYTHONPATH', None)
            python_path_str = os.pathsep.join(pythonpath)
            if old_python_path:
                python_path_str += os.pathsep + old_python_path
            env.insert('PYTHONPATH', python_path_str)

My understanding is that with API 2 (sip.setapi('QString', 2)) QString will be unicode in Py2 and str in Py3. So in Py2, env.value returns a unicode string which will remain unicode when appended to os.pathsep. So everything should work fine provided that the passed in pythonpath is a list of unicode strings. This wasn't checked or enforced in add_pathlist_to_PYTHONPATH, so nothing changed in this respect. We don't need to care about env as this is not a list of strings anymore but a QProcessEnvironment object. So Qt should handle it correctly as long a we supply a unicode string as the second parameter of env.insert.

[StefRe]

In order to test on python 2.7 I tried to install spyder (via pip) and it failed with the following:

ERROR: Could not find a version that satisfies the requirement ipykernel>=5.1.3; python_version > "2" (from spyder-kernels<1.10.0,>=1.9.0->spyder) (from versions: 4.0.1, 4.0.2, 4.0.3, 4.1.0, 4.1.1, 4.2.0, 4.2.1, 4.2.2, 4.3.0, 4.3.1, 4.4.0, 4.4.1, 4.5.0, 4.5.1, 4.5.2, 4.6.0, 4.6.1, 4.7.0, 4.8.0, 4.8.1, 4.8.2, 4.9.0, 4.10.0, 4.10.1)
ERROR: No matching distribution found for ipykernel>=5.1.3; python_version > "2" (from spyder-kernels<1.10.0,>=1.9.0->spyder)

If I understand it correctly it has a requirement of ipykernel for python 3 which is of course not available on python 2. To be honest I don't feel much like spending more time on getting spyder to work on python 2. If you have a working installation maybe you could run the tests on python 2. If not I could just as a precaution wrap python_path_str in to_text_string() even if it might not be necessary:

python_path_str = to_text_string(os.pathsep.join(pythonpath))

What do you think?

[jitseniesen]

I installed a Python 2 environment and found out that the plugin does not work in Python 2 anymore because PR #146 uses nonlocal and I did not appreciate when reviewing that PR that that is a Python 3 keyword.

Since I can't be bothered to backport that PR and I don't want to ask you to do so, I decided that the next version of the plugin will be Python 3 only which means you don't need to make any changes. Sorry for making you jump through the hoops.

[StefRe]

Thanks! There seem to be several ways to implement nonlocal variables in python 2, but dropping support for python 2 makes things certainly easier.