Skip to content
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

Fix failing tests on Windows. #201

Merged
merged 1 commit into from
Jan 14, 2017
Merged

Fix failing tests on Windows. #201

merged 1 commit into from
Jan 14, 2017

Conversation

brcolow
Copy link
Contributor

@brcolow brcolow commented May 5, 2016

Test "test_vim.test_command" was failing because of the call to
os.unlink(..), as the file was still open in vim. Adding a conditional
to execute the call only if we are not running under Windows was the
most straight-forward "fix".

Test "test_vim.test_chdir" was failing for two reasons. Firstly because
of missing Windows-specific handling of path names in
find_file_in_path_option in file_search.c in Neovim core. Secondly
because although one can indeed :cd / on Windows (or at least, should
be able to), a corresponding call to :pwd will echo the root drive
of the current working directory of Neovim.

test_chdir should work now that neovim/neovim#4711 was merged.

@@ -22,7 +22,8 @@ def test_command():
vim.command('w')
ok(os.path.isfile(fname))
eq(open(fname).read(), 'testing\npython\napi\n')
os.unlink(fname)
if os.name != 'nt':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the file was still open in vim

What about adding vim.command('q') here, then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried it before posting the issue actually, didn't fix it =(

Copy link
Contributor

@equalsraf equalsraf May 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the previous suggestion was for sending 'q', maybe that call to unlink is made before neovim actually shuts down. just for testing purposes try sleeping for a second before calling unlink just to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No amount of sleeping seems to have any affect..still results in error:

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\brcolow\\AppData\\Local\\Temp\\tmp9zm0bm83'

@bfredl
Copy link
Member

bfredl commented May 7, 2016

is this ready to merge?

@brcolow
Copy link
Contributor Author

brcolow commented May 7, 2016

I think so...the question from @justinmk still remains about why the unlink doesn't work on Windows, and the suggestion by @fwalch about using vim.command('q') (which doesn't work)...in other words: this makes the tests pass, but I don't know if removing the unlink line will have any unintended consequences. The other test though (test_vim.test_chdir) is 👍. So just need a confirmation/decision about test_vim.test_command about removing the unlink call.

@@ -22,7 +22,8 @@ def test_command():
vim.command('w')
ok(os.path.isfile(fname))
eq(open(fname).read(), 'testing\npython\napi\n')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brcolow Try changing this so that the open()'d file gets closed before we try os.unlink() below. http://askubuntu.com/a/701536 Perhaps this:

with open(fname) as f:
    eq(f.read(), 'testing\npython\napi\n')

Then we shouldn't need to check os.name != 'nt' below.

@@ -21,8 +21,8 @@ def test_command():
vim.command('normal itesting\npython\napi')
vim.command('w')
ok(os.path.isfile(fname))
eq(open(fname).read(), 'testing\npython\napi\n')
os.unlink(fname)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line shouldn't be removed, otherwise there's no reason to make the change

Test "test_vim.test_command" was failing because of the call to
os.unlink(..), as the file was still open in vim. Adding a conditional
to execute the call only if we are not running under Windows was the
most straight-forward "fix".

Test "test_vim.test_chdir" was failing for two reasons. Firstly because
of missing Windows-specific handling of path names in
find_file_in_path_option in file_search.c in Neovim core - this was
fixed in neovim/neovim#4711. Secondly because although one can indeed
`:cd /` on Windows (or at least, should be able to), a corresponding
call to `:pwd` will echo the root drive of the current working
directory of Neovim.
@justinmk justinmk merged commit a2e1169 into neovim:master Jan 14, 2017
bfredl added a commit to bfredl/python-client that referenced this pull request Nov 8, 2017
Brings the client up-to-date with Nvim 0.2.1

Changes since 0.1.13:
* a2e1169 Fix tests on windows (neovim#201)
* 9a0e729 fix an indexing bug when setting lines in a Range object (neovim#270)
* 4abd5d0 Documentation update (neovim#272)
* a703b47 make sure logging always uses UTF-8 regardless of locale (neovim#276)
* 68aa352 argument to allow nested notification handlers (neovim#262)
bfredl added a commit to bfredl/python-client that referenced this pull request Nov 8, 2017
Brings the client up-to-date with Nvim 0.2.1

Changes since 0.1.13:
* a2e1169 Fix tests on windows (neovim#201)
* 9a0e729 Fix an indexing bug when setting lines in a Range object (neovim#270)
* 4abd5d0 Documentation update (neovim#272)
* a703b47 Make sure logging always uses UTF-8 regardless of locale (neovim#276)
* 68aa352 Argument to allow nested notification handlers (neovim#262)
bfredl added a commit to bfredl/python-client that referenced this pull request Nov 8, 2017
Brings the client up-to-date with Nvim 0.2.1

Changes since 0.1.13:
* a2e1169 Fix tests on windows (neovim#201)
* 9a0e729 Fix an indexing bug when setting lines in a Range object (neovim#270)
* 4abd5d0 Documentation update (neovim#272)
* a703b47 Make sure logging always uses UTF-8 regardless of locale (neovim#276)
* 68aa352 Add argument to allow nested notification handlers (neovim#262)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants