PR: Add test with non-ascii directory for consoles stderr file #5013

dalthviz · 2017-08-20T04:01:57Z

dalthviz · 2017-08-26T13:46:51Z

@ccordoba12 should I do something else with this one?

ccordoba12 · 2017-08-26T18:06:07Z

spyder/plugins/ipythonconsole.py

-            if not osp.isdir(programs.TEMPDIR):
-                os.mkdir(programs.TEMPDIR)
+            if not osp.isdir(osp.join(programs.TEMPDIR, u'測試', u'اختبار')):
+                os.makedirs(osp.join(programs.TEMPDIR, u'測試', u'اختبار'))


@dalthviz, I don't like this test directory to be part of our sourcecode.

Please add a new parameter to __init__ called test_dir=TEMPDIR. With that this should look like:

if self.testing: if not osp.isdir(test_dir): os.makedirs(test_dir)

Then in our tests you can change that parameter by this directory containing Chinese and Arabic characters.

ccordoba12 · 2017-08-26T18:07:05Z

spyder/widgets/ipythonconsole/client.py

+            if not self.testing:
+                stderr_file = osp.join(TEMPDIR, stderr_file)
+            else:
+                stderr_file = osp.join(TEMPDIR, u'測試', u'اختبار', stderr_file)


Please also find a solution similar to the one I posted above to avoid putting this temp directory as part of our source code.

pep8speaks · 2017-08-26T20:16:25Z

Hello @dalthviz! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 27, 2017 at 19:07 Hours UTC

ccordoba12 · 2017-08-27T16:12:02Z

spyder/plugins/tests/test_ipythonconsole.py

@@ -74,9 +74,45 @@ def close_console():
    return console


+@pytest.fixture
+def ipyconsole_stderr(request):


@dalthviz, are you sure we need an extra fixture for this? Is it not possible to add an extra parameter to the previous fixture?

Yep, you are right 👍 no need of a extra fixture

ccordoba12 · 2017-08-27T16:13:22Z

spyder/plugins/ipythonconsole.py

@@ -590,7 +590,7 @@ class IPythonConsole(SpyderPluginWidget):
    focus_changed = Signal()
    edit_goto = Signal((str, int, str), (str, int, str, bool))

-    def __init__(self, parent, testing=False):
+    def __init__(self, parent, testing=False, test_dir=programs.TEMPDIR):


Please use here only TEMPDIR, so import it from programs with

from programs import TEMPDIR

We also need to add

self.test_dir = test_dir

ccordoba12 · 2017-08-27T16:17:07Z

spyder/plugins/ipythonconsole.py

@@ -932,7 +933,8 @@ def write_to_stdin(self, line):
    @Slot(bool)
    @Slot(str)
    @Slot(bool, str)
-    def create_new_client(self, give_focus=True, filename=''):
+    def create_new_client(self, give_focus=True, filename='',
+                          stderr_dir=programs.TEMPDIR):


Please don't add a new parameter here because we'd need to also add new Slot's

ccordoba12 · 2017-08-27T16:19:14Z

spyder/plugins/ipythonconsole.py

@@ -944,7 +946,8 @@ def create_new_client(self, give_focus=True, filename=''):
                              additional_options=self.additional_options(),
                              interpreter_versions=self.interpreter_versions(),
                              connection_file=cf,
-                              menu_actions=self.menu_actions)
+                              menu_actions=self.menu_actions,
+                              stderr_dir=stderr_dir)


Instead of doing this, please change it to

if self.testing: client.stderr_dir = self.test_dir

ccordoba12 · 2017-08-27T16:20:39Z

spyder/widgets/ipythonconsole/client.py

@@ -95,7 +95,8 @@ def __init__(self, plugin, id_,
                 additional_options, interpreter_versions,
                 connection_file=None, hostname=None,
                 menu_actions=None, slave=False,
-                 external_kernel=False, given_name=None):
+                 external_kernel=False, given_name=None,
+                 stderr_dir=TEMPDIR):


Please remove this

ccordoba12 · 2017-08-27T16:21:23Z

spyder/widgets/ipythonconsole/client.py

@@ -113,6 +114,7 @@ def __init__(self, plugin, id_,
        self.stop_icon = ima.icon('stop')
        self.history = []
        self.allow_rename = True
+        self.stderr_dir = stderr_dir


This needs to be

# Used only for testing self.stderr_dir = None

ccordoba12 · 2017-08-27T16:22:15Z

spyder/widgets/ipythonconsole/client.py

@@ -163,7 +165,7 @@ def stderr_file(self):
        """Filename to save kernel stderr output."""
        if self.connection_file is not None:
            stderr_file = self.kernel_id + '.stderr'
-            stderr_file = osp.join(TEMPDIR, stderr_file)
+            stderr_file = osp.join(self.stderr_dir, stderr_file)


Finally, this needs to be

if self.stderr_dir is not None: stderr_file = osp.join(self.stderr_dir, stderr_file) else: stderr_file = osp.join(TEMPDIR, stderr_file)

Fixes #4836

Adds test with non-ascii directory for stderr file.

7f7a7ea

dalthviz added this to the v3.2.2 milestone Aug 20, 2017

dalthviz self-assigned this Aug 20, 2017

dalthviz requested a review from ccordoba12 August 20, 2017 04:01

dalthviz changed the base branch from master to 3.x August 20, 2017 04:02

dalthviz added 3 commits August 19, 2017 23:42

Testing.

1a6d16b

Testing.

913dc66

Testing.

a300cd3

ccordoba12 reviewed Aug 26, 2017

View reviewed changes

Improvements.

9dc75d5

Testing.

885db09

ccordoba12 reviewed Aug 27, 2017

View reviewed changes

Improvements.

737fd4d

ccordoba12 approved these changes Aug 28, 2017

View reviewed changes

ccordoba12 changed the title ~~PR: Add test with non-ascii directory for stderr file~~ PR: Add test with non-ascii directory for consoles stderr file Aug 28, 2017

ccordoba12 merged commit 8efe647 into spyder-ide:3.x Aug 28, 2017

ccordoba12 added a commit that referenced this pull request Aug 28, 2017

Merge from 3.x: PR #5013

53d1479

Fixes #4836

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Add test with non-ascii directory for consoles stderr file #5013

PR: Add test with non-ascii directory for consoles stderr file #5013

dalthviz commented Aug 20, 2017

dalthviz commented Aug 26, 2017

ccordoba12 Aug 26, 2017

ccordoba12 Aug 26, 2017

pep8speaks commented Aug 26, 2017 •

edited

Loading

ccordoba12 Aug 27, 2017

dalthviz Aug 27, 2017

ccordoba12 Aug 27, 2017

ccordoba12 Aug 27, 2017

ccordoba12 Aug 27, 2017

ccordoba12 Aug 27, 2017

ccordoba12 Aug 27, 2017

ccordoba12 Aug 27, 2017

ccordoba12 Aug 27, 2017

PR: Add test with non-ascii directory for consoles stderr file #5013

PR: Add test with non-ascii directory for consoles stderr file #5013

Conversation

dalthviz commented Aug 20, 2017

dalthviz commented Aug 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Aug 26, 2017 • edited Loading

Comment last updated on August 27, 2017 at 19:07 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Aug 26, 2017 •

edited

Loading