[WIP] Enable more tests in Windows #7412

janlazo · 2017-10-19T04:25:29Z

Unlike #7343, this PR focuses on tests only.

The following types of tests must be updated to work in Windows environment:

any tests with hard dependency on Unix environment
- Makefile
  - /bin/sh must resolve to an absolute path without spaces otherwise you get shell returned 1 all over the place
- oldtests that fail in Windows
  - anything that relies on set shell=sh because, unlike nvim, vim respects $SHELL on Windows and doesn't use shellescape in most tests by hardcoding the single quotes (WHY???)
    - in other words, all tests that rely on the shell and must be escaped will most likely fail on cmd.exe
  - test17.in
    - gf doesn't work for paths with spaces such as C:\Program Files (x86)\nvim\share\nvim\runtime\defaults.vim
    - maybe set shellslash breaks it since its shell defaults should be for cmd.exe
  - test32.in
    - infinite loop
    - broken TUI?
- os.remove()
  - Os module relies on POSIX environment. Windows is not POSIX compatible. WSL doesn't count because the tests run in Appveyor.
  - does not work with directories and read-only files in Windows
    - try nvim's delete
  - os.execute in helpers.do_rmdir works in Windows. What about Unix? rm -rf hangs while cmd.exe's rd returns early.
- LuaFileSystem (and other 3rd-party Lua modules)
  - must shim them to run os.execute internally or use VimL equivalents.
  - for anyone who want to use all of its API, just disable the test on Windows.
- Unix programs like yes
  - replace programs with cat - and other shipped tools included in Windows zip
- /dev/null
  - use nul in cmd.exe
  - use $null in powershell
- xterm codes (what about winpty?)
jobstart + jobsend/jobpid/jobclose (generally applies to channels)
- ~~unreliable for has('win64')?~~ Unreliable for both but seems to be more flaky in win64.
- ~~desync with cat.exe so testing is limited to jobstart only~~
- jobpid(jobstart(['cat', '-')) is unreliable
nested terminals (or nvim)
- scrollback (?) issue
  - feed(G) and different expected screen output as workaround
- Data file mismatch - some data files may have been concurrently updated without locking support
  - VimL issue with eval.c or terminal buffer updates?
- ~~fails with "-u NONE --cmd '" .. nvim_set .. "'" (how?)~~ It's fine. I mixed up the shells in core/job_spec.lua
  - powershell supports single-quote escaping but it is incompatible with Unix shells
  - cmd.exe supports double-quote escaping and ^ for single character escaping. These two can be combined but it is tedious to escape the entire command this way. There is sxe and sxq but they don't work well with shellescape because 1 mishandled double quote can ruin the entire command.
screen highlight
- Any background/foreground color beyond the default 16 colors is hit-or-miss.
autocmd + (terminal or jobstart)
- possible race condition
errors for opening a directory
- Vim outputs the expected message from the shortmess test
- Neovim attempts to edit it (?) and gets read errors.
VimL delete()
- do not use with working directories in Windows
FileWriteCmd
- E13 in Windows
- reproduced in Vim 8.0.1123 (bug-vim?)
Viml mkdir()
- cannot handle directory junctions on Windows so use directory symlinks with mklink /d
  - directory symlinks require elevated access, directory junctions do not
build exits early when testing providers (ex. ruby, node) in Windows
- see [WIP] Enable more tests in Windows #7412 (comment)

I don't know why the Appveyor builds hang when I use helpers.get_pathsep() so I hardcode the value of pathsep local variable in tests that check filepaths such as test/functional/ex_cmds/cd_spec.lua

~~To minimize diffs, I use set fileformat=unix or the ++opt variant.~~ \r\n as eol in Windows is enough for shada on BufWriteCmd, FileAppendCmd, and FileWriteCmd. For some reason, :w! is required in FileWriteCmd after os.remove in before_each() and teardown().

For some reason, nvim does not kill a parallel process created via start /b cmd /s/c "<command>", where <command> is the user's command entered in a cmd.exe session. /b is for not creating a new window. If start /min for a minimized new window doesn't work out, then I'll try powershell's Start-Job or Start-Process since core/job_spec.lua defaults to powershell anyway. It works for MINGW_32.

janlazo · 2017-10-20T18:50:29Z

@justinmk @jamessan Any idea how to resolve E13 for shada on FileWriteCmd? Aside from :w!, I'm out of ideas.

janlazo · 2017-10-21T07:06:00Z

The remaining tests are flaky so this is enough for 0.2.1

janlazo · 2017-10-22T19:26:13Z

@justinmk Any blockers on getting this merged for 0.2.1? I do not plan on enabling more tests.

justinmk · 2017-10-22T20:04:08Z

jobpid(jobstart(['cat', '-')) is unreliable

Just to emphasize, we should fix the root problem rather than working around that in the tests.

Appveyor builds hang when I use helpers.get_pathsep() so I hardcode the value of pathsep local variable in tests that check filepaths such as test/functional/ex_cmds/cd_spec.lua

I don't see any usages where get_pathsep() needs to query the current nvim session, so it should be changed to just use iswin(), which uses package.config as a hack to guess the system without needing a nvim session.

local function get_pathsep()
  return iswin() and '\\' or '/'
end

nested terminals (or nvim)
fails with "-u NONE --cmd '" .. nvim_set .. "'" (how?)

Is it sending single-quotes to cmd.exe?

errors for opening a directory
Vim outputs the expected message from the shortmess test
Neovim attempts to edit it (?) and gets read errors.

Gah, I wonder if os_nodetype() needs to be adjusted again. Else it's a bug in fileio.c:readfile().

os.remove() does not work with directories and read-only files in Windows

helpers.rmdir() will correctly handle directories on Windows. For files, the best thing to do is to make sure the file is closed in the nvim session before trying to remove it. We could write a util function to force-close the nvim session, but that could be confusing for test authors. In helpers.do_rmdir() I catch the error and show a hint message: try :%bwipeout! before rmdir().

justinmk · 2017-10-22T20:08:17Z

test/functional/core/job_spec.lua

+    if not iswin() then
+      nonexecutable = nonexecutable .. '/non_executable.txt'
+    end
+    local nonexecutable_jobid = eval("jobstart(['"..nonexecutable.."'])")


ternary is less noisy, here.

local nonexecutable_jobid = (iswin() and eval("jobstart(['./test/functional/fixtures'])") or eval("jobstart(['./test/functional/fixtures/non_executable.txt'])"))

Will that run 2 jobs?

No, if iswin() is false then the and ... is skipped. And eval() cannot return nil, unless something else is broken.

What if jobstart returns 0?

justinmk · 2017-10-22T20:14:27Z

test/functional/core/job_spec.lua

-    nvim('command', "call jobstart(['touch', '.Xtestjob'])")
-    nvim('command', "sleep 100m")
+    local touch_cmd = iswin() and "'Out-File -encoding ASCII .Xtestjob'"
+                              or "['touch', '.Xtestjob']"


I'm surprised Lua continues the line. Even if it does, should put parens around it to be explicit.

local touch_cmd = (iswin() and ... or ...)

justinmk · 2017-10-22T20:18:15Z

test/functional/core/job_spec.lua

+    local touch_cmd = iswin() and "'Out-File -encoding ASCII .Xtestjob'"
+                              or "['touch', '.Xtestjob']"
+    nvim('command', "call jobstart(" .. touch_cmd .. ")")
+    nvim('command', "sleep " .. (iswin() and "5" or "100m"))


5s is expensive for such a trivial test. Can we just check that jobstart() returned >0 instead? And instead of running "touch", run nvim -u NONE ....

call jobstart(['nvim', '-u', 'NONE', '-c', 'q', '.Xtestjob'])?
Are sleep and delete required if we're checking the job id only?

I think not. IIUC the test only verifies that jobstart() works without the options dict. So the touch+sleep is pointless, and anything covered by it is well-trodden by other tests.

justinmk · 2017-10-22T20:21:30Z

test/functional/eval/system_spec.lua

-      if helpers.pending_win32(pending) then return end
-      eq({'part1\npart2\npart3'}, eval('systemlist("cat '..fname..'")'))
+      if iswin() then
+        eq({'part1\npart2\npart3\r'}, eval('systemlist("type '..fname..'")'))


can't we use cat on windows?

Yes, but the trailing \r remains. See #7343 (comment).

Sure, but the more we can do to avoid platform-specific variations, the better.

justinmk · 2017-10-22T20:25:33Z

test/functional/ex_cmds/bang_filter_spec.lua

+      map = [[nnoremap <silent>\l :!dir /b bang_filter_spec<cr>]]
+      result = [[
+  :!dir /b bang_filter_spec                            |
+  ]]


Can't the whitespace be done once below, instead of twice here? Also a ternary is clearer:

command(iswin() and [[nnoremap <silent>\l :!dir /b bang_filter_spec<cr>]] or [[nnoremap <silent>\l :!ls bang_filter_spec<cr>]]) local result = (iswin() and [[:!dir /b bang_filter_spec |]] or [[:!ls bang_filter_spec |]])

justinmk · 2017-10-22T20:27:54Z

test/functional/ex_cmds/cd_spec.lua

      eq(0 , tlwd())
-      eq(globalDir .. '/' .. directories.window, wcwd())
+      eq(globalDir .. pathsep .. directories.window, wcwd())


Some sort of pathjoin() function would help, in the future ...

justinmk · 2017-10-22T20:33:20Z

test/functional/ex_cmds/write_spec.lua

+    end
+    eq(true, write_file(fname_bak, 'TTYX'))
+    -- FIXME: exc_exec('write!') outputs 0 in Windows
+    if not helpers.iswin() then


just return early, to avoid the extra nesting and VCS churn.

if helpers.iswin() then return end?

justinmk · 2017-10-22T20:35:25Z

test/functional/helpers.lua

@@ -319,6 +319,7 @@ end
 -- Dedent the given text and write it to the file name.
 local function write_file(name, text, dont_dedent)
  local file = io.open(name, 'w')
+  if file == nil then return false end


Was this silently failing before? If not, it will now, which means we would have to explicitly check eq(true, write_file()) everywhere.

Why don't we raise an error (exception) here instead?

It was failing for ex_cmds/write_spec.lua because os.remove did not remove the files. write_file tried to write on a directory so file:write raised an error.

- echo "" does not hang in powershell - cmd.exe's echo command does not hang. - job tests default to powershell (WHY?) - wait 5 seconds for powershell to create an empty file - powershell is slow - cannot reliably validate the id returned by jobstart via jobpid, jobstop - if using cmd.exe, waiting for a second should be enough - remaining job tests are unreliable in Windows because any build can pass/fail for same conditions without changes, especially if the error is in stderr

Try nvim's delete() for cross-platform file remove in Windows

justinmk · 2018-02-19T23:00:36Z

Regarding your note in one of the commit messages:

job tests default to powershell (WHY?)

IIRC that was my fault, I did so for these reasons:

at the time we didn't have cat.exe, etc., so I thought that powershell might be closer to unix for some utilities
powershell shell syntax and scripting (for/while-loops) are closer to unix shells and more sane
I have a vague idea that we may want to default shell=powershell for end-users, someday.
- That may take longer than expected because powershell is still surprisingly slow.
IIRC quoting was more sane on powershell. This was before your fixes for shell quoting.

If shell=cmd.exe is better for tests, feel free to change it!

justinmk · 2018-02-19T23:13:24Z

Regarding the preserves NULs test, I have this note in an old branch:

   it('preserves NULs', function()
-    if helpers.pending_win32(pending) then return end  -- TODO: Need `cat`. 
+    -- TODO: Windows: the data is received on_stderr instead of on_stdout. Why?
+    if helpers.pending_win32(pending) then return end

janlazo changed the title ~~Enable more tests in Windows~~ [WIP] Enable more tests in Windows Oct 19, 2017

marvim added the WIP label Oct 19, 2017

janlazo mentioned this pull request Oct 19, 2017

[RFC] win: defaults: 'shellcmdflag', 'shellxquote' #7343

Closed

janlazo force-pushed the win_test_enable branch 5 times, most recently from ca7f5b3 to d63e17c Compare October 20, 2017 18:10

janlazo force-pushed the win_test_enable branch 2 times, most recently from e7b8f7d to fafd95d Compare October 20, 2017 23:19

janlazo changed the title ~~[WIP] Enable more tests in Windows~~ [RFC] Enable more tests in Windows Oct 21, 2017

marvim added RFC and removed WIP labels Oct 21, 2017

janlazo force-pushed the win_test_enable branch from b83baa1 to c58f779 Compare October 21, 2017 18:32

janlazo changed the title ~~[RFC] Enable more tests in Windows~~ [RDY] Enable more tests in Windows Oct 21, 2017

marvim added RDY and removed RFC labels Oct 21, 2017

justinmk added the test label Oct 22, 2017

justinmk reviewed Oct 22, 2017

View reviewed changes

janlazo and others added 21 commits February 19, 2018 07:10

win: enable legacy test 025

0fd899a

win: enable legacy test 097

bde32ed

win: enable legacy/delete_spec.lua

4f65cd7

win: enable legacy test 011

f4d82c1

win: enable legacy/packadd_spec.lua

e55de56

win: enable legacy/wordcount_spec.lua

3c0cc9c

win: enable legacy/getcwd_spec.lua

c5a7f45

win: enable legacy test 30

18a53b6

win: enable legacy/arglist_spec.lua

10fbae0

win: enable legacy test 093

44dc8b4

win: enable legacy test 107

2943056

win: enable legacy test 059

f1e6828

win: enable legacy test 051

6beb7ee

test: enable ex_cmds/write_spec.lua in Windows

55ce6bf

test: enable legacy/fixeol_spec in Windows

df99ab4

Try nvim's delete() for cross-platform file remove in Windows

test: enable ex_cmds/cd_spec.lua on Windows

d80bf3c

test: win: enable termclose_spec.lua

8dcfd58

test: win: enable ui/wildmode_spec.lua

07dfe0f

test: win: emulate yes with for loop

795da34

Resolve issues mentioned in PR review

7fa69fb

janlazo force-pushed the win_test_enable branch from a022b7d to 7fa69fb Compare February 19, 2018 12:11

justinmk merged commit e215b6c into neovim:master Feb 19, 2018

justinmk removed the WIP label Feb 19, 2018

janlazo deleted the win_test_enable branch February 20, 2018 18:23

janlazo mentioned this pull request Feb 20, 2018

[RFC] test: win: enable more job tests #8034

Closed

janlazo mentioned this pull request Mar 9, 2018

[RDY] test: win: use cmd.exe by default in job_spec.lua #8120

Merged

janlazo mentioned this pull request May 12, 2018

[RFC] test: win: enable remaining functional tests #8378

Merged

Uh oh!

[WIP] Enable more tests in Windows #7412

[WIP] Enable more tests in Windows #7412

Uh oh!

Conversation

janlazo commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janlazo commented Oct 20, 2017

Uh oh!

janlazo commented Oct 21, 2017

Uh oh!

janlazo commented Oct 22, 2017

Uh oh!

justinmk commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Feb 19, 2018

Uh oh!

Uh oh!

janlazo commented Oct 19, 2017 •

edited

Loading

justinmk commented Oct 22, 2017 •

edited

Loading

justinmk Oct 22, 2017 •

edited

Loading

justinmk Oct 22, 2017 •

edited

Loading

justinmk Oct 22, 2017 •

edited

Loading

justinmk commented Feb 19, 2018 •

edited

Loading