PR: Fix error when setting a path with a single quote #6543

csabella · 2018-02-27T22:18:44Z

ccordoba12 · 2018-02-27T23:54:03Z

@csabella, please add a test for this.

csabella · 2018-02-28T00:06:08Z

@ccordoba12 I wasn't sure how to add a test. I put some code on the issue showing that it was working with a manual test. Is there an existing test that I can look at for creating a new directory or for working with these ipython command calls?

ccordoba12 · 2018-02-28T00:21:44Z

Take a look at test_conf_env_vars:

https://github.com/spyder-ide/spyder/blob/3.x/spyder/plugins/tests/test_ipythonconsole.py#L196

That does pretty much what you need to do here.

pep8speaks · 2018-02-28T13:20:13Z

Hello @csabella! Thanks for updating the PR.

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

Comment last updated on March 07, 2018 at 10:50 Hours UTC

csabella · 2018-02-28T13:39:48Z

@ccordoba12 Thanks for the suggestion! In that test file, test_get_env called get_env which had the same type of goal as get_cwd and set_cwd. Very helpful.

I'm getting PEP8 warnings, but I copied that code from the other tests. Is the longer line allowed here?

ccordoba12 · 2018-03-01T13:50:38Z

spyder/plugins/tests/test_ipythonconsole.py

@@ -323,6 +323,60 @@ def test_console_coloring(ipyconsole, qtbot):
    assert console_font_color.strip() == editor_font_color.strip()


+@pytest.mark.slow
+@flaky(max_runs=3)
+@pytest.mark.skipif(os.name == 'nt', reason="It times out sometimes on Windows")


Let's remove this skipif condition to test this on Windows.

ccordoba12 · 2018-03-01T13:52:24Z

spyder/plugins/tests/test_ipythonconsole.py

+
+    # Issue 6451.
+    savetemp = shell._cwd
+    tempdir = tempfile.mkdtemp(suffix="queen's")


Please use the tmpdir fixture instead of tempfile. There's an example of it here:

https://github.com/spyder-ide/spyder/blob/3.x/spyder/app/tests/test_mainwindow.py#L213

Thanks, I've made the change.

ccordoba12 · 2018-03-01T13:52:41Z

spyder/plugins/tests/test_ipythonconsole.py

+
+@pytest.mark.slow
+@flaky(max_runs=3)
+@pytest.mark.skipif(os.name == 'nt', reason="It times out sometimes on Windows")


Remove skipif

ccordoba12 · 2018-03-01T13:52:56Z

spyder/plugins/tests/test_ipythonconsole.py

+
+    # Issue 6451.
+    savetemp = shell._cwd
+    tempdir = tempfile.mkdtemp(suffix="queen's")


Please use the tmpdir fixture.

ccordoba12 · 2018-03-01T13:54:31Z

@csabella, this looks pretty good. I just left a couple of comments in your tests to improve them a bit.

And please don't worry about pep8 issues in test files. Unfortunately, we can't make pep8speaks to ignore them.

ccordoba12 · 2018-03-03T19:15:42Z

spyder/plugins/tests/test_ipythonconsole.py

+
+    # Assert we get the assigned value correctly
+    assert shell.get_value('cwd') == tempdir
+    os.rmdir(tempdir)


Please remove this so that this test can pass on Windows.

csabella · 2018-03-04T12:53:36Z

It looks like windows isn't applying the os.chdir before the assert. :-( Trying to figure it out.

ccordoba12 · 2018-03-04T17:05:46Z

spyder/plugins/tests/test_ipythonconsole.py

+    # Ask for directory.
+    with qtbot.waitSignal(shell.sig_change_cwd):
+        shell.get_cwd()
+    assert shell._cwd == tempdir


What if you introduce a qtbot.wait(1000) before this line to see if the test pass?

csabella · 2018-03-06T15:43:23Z

Ignore this. I didn't see the changes you did on the tests before I wrote my comment.

I've added a qtbot.wait(). However, all the existing tests near this one just skip windows (@pytest.mark.skipif(os.name == 'nt' or PYQT4, reason="It doesn't work on Windows and segfaults in PyQt4")). I can enter the commands run by those other tests manually into the IPython console on a windows machine (such as "import os; os.environ['FOO'] = 'bar'"), but yet the automated testing is skipped. I believe the chdir in this new test is running into the same issues as those other tests.

ccordoba12 · 2018-03-06T16:19:14Z

@csabella, I don't want to put an additional burden on you, so if you want to skip this test on Windows, please do it.

However, all the existing tests near this one just skip windows

Well, I just removed a lot of those skipif's because I noticed the corresponding tests were hanging on Windows due to us opening a QDialog and trying to close it with a QTimer. I removed that and used a lower level API instead.

csabella · 2018-03-06T17:51:20Z

@ccordoba12 It's not a burden. I just don't know what to change to get it to work. My dev box is Linux and it works there. I can run the Spyder installation on Windows, but I can't download the git repo and test it from there. So, I'm kind of in the dark trying to fix this in the tests. :-(

ccordoba12 · 2018-03-06T18:14:44Z

@csabella, no worries then. If you tested your PR on a Windows box and it works, then please skip the test. You can come back later to remove the skip if you find how to do it (I've done that a lot of times :-).

csabella · 2018-03-06T18:31:11Z

@ccordoba12 No, I haven't tested it on Windows. I can't install the git repo on the Windows box that I can access, so I can't test my changes there.

ccordoba12 · 2018-03-06T18:36:06Z

@dalthviz, please test @csabella work on Windows and let us know if it works for you or not.

jnsebgosselin · 2018-03-06T21:22:53Z

The patch is working, but the test_get_cwd is failing locally on my setup (Windows 7 64 bits). I'll take a look a it now in case I can find out why.

This is from the window that spawn during the test :

jnsebgosselin · 2018-03-06T21:35:55Z

The test is failing because of the \ in the path when running shell.execute("import os; os.chdir('''{}''')".format(tempdir))

Running this instead with a raw string works:
shell.execute("import os; os.chdir(r'''{}''')".format(tempdir))

So, following what is done in shell.py, something like this maybe? It's not pretty, but it works lol

    if os.name == 'nt':
        tempdir = tempdir.replace(u"\\", u"\\\\")

    # Change directory in the console.
    with qtbot.waitSignal(shell.executed):
        shell.execute(u"import os; os.chdir(u'''{}''')".format(tempdir))

    if os.name == 'nt':
        tempdir = tempdir.replace(u"\\\\", u"\\")

csabella · 2018-03-06T22:40:58Z

Thank you, @jnsebgosselin! I really appreciate you looking into this. Without being able to test on Windows, I would have never figured that out.

jnsebgosselin · 2018-03-06T23:24:10Z

I'm glad I could help! Is it me or the tests are failing for a reason that is unrelated to this PR? The test test_get_help is failing everywhere.

ccordoba12 · 2018-03-07T00:13:15Z

Cheryl, the failures are unrelated to your tests. I'm fixing them in #6626.

csabella · 2018-03-07T00:37:22Z

@ccordoba12 Thank you for letting me know. I was just going to start looking into them.

ccordoba12 · 2018-03-07T01:46:39Z

@csabella, please rebase to get my fix. Then this should be ready.

csabella · 2018-03-07T11:34:23Z

Thanks again @ccordoba12 and @jnsebgosselin for helping me get the test to pass. :-)

Fixes #6451

ccordoba12 added this to the v3.2.8 milestone Feb 27, 2018

CAM-Gerlach assigned csabella Feb 28, 2018

CAM-Gerlach added type:Bug component:IPython Console status:Pending Changes/Test labels Feb 28, 2018

ccordoba12 reviewed Mar 1, 2018

View reviewed changes

ccordoba12 reviewed Mar 3, 2018

View reviewed changes

ccordoba12 reviewed Mar 4, 2018

View reviewed changes

csabella force-pushed the issue6451 branch from 737096f to c95a1ff Compare March 6, 2018 15:53

ccordoba12 requested a review from dalthviz March 6, 2018 18:36

csabella added 7 commits March 7, 2018 05:50

issue6451: Error when setting a path with a single quote

15bf240

Add tests

142877d

Use tmpdir in tests

506870e

Remove os.rmdir() for windows testing

a6e9cc6

Add delay to try to fix failure on windows test.

4d84a5f

rebase

c29a470

Escape backslash for windows test

f0de4db

csabella force-pushed the issue6451 branch from 0399bf4 to f0de4db Compare March 7, 2018 10:50

ccordoba12 approved these changes Mar 7, 2018

View reviewed changes

ccordoba12 changed the title ~~PR: Error when setting a path with a single quote~~ PR: Fix error when setting a path with a single quote Mar 7, 2018

ccordoba12 merged commit 53ae83e into spyder-ide:3.x Mar 7, 2018

ccordoba12 added a commit that referenced this pull request Mar 7, 2018

Merge from 3.x: PR #6543

b6a9c2d

Fixes #6451

csabella deleted the issue6451 branch March 7, 2018 20:23

ccordoba12 mentioned this pull request Mar 16, 2018

PR: Initialize start time to a time and not None #6769

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Fix error when setting a path with a single quote #6543

PR: Fix error when setting a path with a single quote #6543

csabella commented Feb 27, 2018 •

edited by jnsebgosselin

Loading

ccordoba12 commented Feb 27, 2018

csabella commented Feb 28, 2018

ccordoba12 commented Feb 28, 2018 •

edited

Loading

pep8speaks commented Feb 28, 2018 •

edited

Loading

csabella commented Feb 28, 2018

ccordoba12 Mar 1, 2018

csabella Mar 1, 2018

ccordoba12 Mar 1, 2018

csabella Mar 1, 2018

ccordoba12 Mar 1, 2018

csabella Mar 1, 2018

ccordoba12 Mar 1, 2018

ccordoba12 commented Mar 1, 2018

ccordoba12 Mar 3, 2018

csabella commented Mar 4, 2018

ccordoba12 Mar 4, 2018

csabella commented Mar 6, 2018 •

edited

Loading

ccordoba12 commented Mar 6, 2018

csabella commented Mar 6, 2018

ccordoba12 commented Mar 6, 2018

csabella commented Mar 6, 2018

ccordoba12 commented Mar 6, 2018

jnsebgosselin commented Mar 6, 2018 •

edited

Loading

jnsebgosselin commented Mar 6, 2018 •

edited

Loading

csabella commented Mar 6, 2018

jnsebgosselin commented Mar 6, 2018

ccordoba12 commented Mar 7, 2018

csabella commented Mar 7, 2018

ccordoba12 commented Mar 7, 2018

csabella commented Mar 7, 2018

PR: Fix error when setting a path with a single quote #6543

PR: Fix error when setting a path with a single quote #6543

Conversation

csabella commented Feb 27, 2018 • edited by jnsebgosselin Loading

ccordoba12 commented Feb 27, 2018

csabella commented Feb 28, 2018

ccordoba12 commented Feb 28, 2018 • edited Loading

pep8speaks commented Feb 28, 2018 • edited Loading

Comment last updated on March 07, 2018 at 10:50 Hours UTC

csabella commented Feb 28, 2018

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

ccordoba12 commented Mar 1, 2018

Choose a reason for hiding this comment

csabella commented Mar 4, 2018

Choose a reason for hiding this comment

csabella commented Mar 6, 2018 • edited Loading

ccordoba12 commented Mar 6, 2018

csabella commented Mar 6, 2018

ccordoba12 commented Mar 6, 2018

csabella commented Mar 6, 2018

ccordoba12 commented Mar 6, 2018

jnsebgosselin commented Mar 6, 2018 • edited Loading

jnsebgosselin commented Mar 6, 2018 • edited Loading

csabella commented Mar 6, 2018

jnsebgosselin commented Mar 6, 2018

ccordoba12 commented Mar 7, 2018

csabella commented Mar 7, 2018

ccordoba12 commented Mar 7, 2018

csabella commented Mar 7, 2018

csabella commented Feb 27, 2018 •

edited by jnsebgosselin

Loading

ccordoba12 commented Feb 28, 2018 •

edited

Loading

pep8speaks commented Feb 28, 2018 •

edited

Loading

csabella commented Mar 6, 2018 •

edited

Loading

jnsebgosselin commented Mar 6, 2018 •

edited

Loading

jnsebgosselin commented Mar 6, 2018 •

edited

Loading