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

janlazo · 2018-03-09T02:05:39Z

Ref #7412 (comment), #8034
Need #8107 for jobstop to return exit code 0 if the program successfully exits and returns exit code 0 if run from a shell (ex. cmd.exe, bash).

Testing to see if switching to cmd.exe improves testing with job API and cat.exe on Windows.

I'm giving up on NULs and newlines for jobsend (list argument). cat.exe still has the invalid channel id error, write_file truncates the string with \0 and find.exe truncates the input string after \0.

start (cmd.exe builtin) reaps its child processes with /b (I can't find them with tasklist) so I'm using /min instead for a minimized window. Redirecting to nul shouldn't be necessary. If that's intended with the jobstop() test, so be it.

Not sure what's the expected behaviour for start (cmd.exe builtin) because nvim is not reaping the child processes on exit after :qa!, even on nested nvim termnal. Seems to have different semantics from fork (or what bash does with &). Perhaps powershell is better suited here ~~but I don't know how (yes, there's Start-Process but I can't pass arguments to the external program).~~ ~~I can use Start-Job so if the session terminates, the jobs die too. Is that close enough?~~ Leave it for later. I disabled the pty teardown test.

more.com (like less) is the default pager in Windows so it can be used to display files. That leaves the NUL tests for cat if more can't do it.

fnamemodify doesn't adjust slashes and it's the same on Vim too. Have to use expand() again.

justinmk · 2018-03-09T14:26:35Z

Somehow the exit code is wrong for these two tests on the 32bit build:

[  FAILED  ] C:/projects/neovim/test/functional\core\job_spec.lua @ 140: jobs allows interactive commands
C:/projects/neovim/test/functional\core\job_spec.lua:151: Expected objects to be the same.
Passed in:
(table) {
  [1] = 'notification'
  [2] = 'exit'
 *[3] = {
    [1] = 0
   *[2] = 1 } }
Expected:
(table) {
  [1] = 'notification'
  [2] = 'exit'
 *[3] = {
    [1] = 0
   *[2] = 0 } }
stack traceback:
	C:/projects/neovim/test/functional\core\job_spec.lua:151: in function <C:/projects/neovim/test/functional\core\job_spec.lua:140>
[  FAILED  ] C:/projects/neovim/test/functional\core\job_spec.lua @ 195: jobs preserves NULs
C:/projects/neovim/test/functional\core\job_spec.lua:202: Expected objects to be the same.
Passed in:
(table) {
  [1] = 'notification'
  [2] = 'exit'
 *[3] = {
    [1] = 0
   *[2] = 1 } }
Expected:
(table) {
  [1] = 'notification'
  [2] = 'exit'
 *[3] = {
    [1] = 0
   *[2] = 0 } }
stack traceback:

should we leave those "pending" for now?

janlazo · 2018-03-09T18:55:34Z

@justinmk Sure, but I'll do it later. I want to see if it happens in 32-bit build only.

Old tests aren't running.

[00:14:33] C:\projects\neovim\build>cmake --build "C:\projects\neovim\ci\\..\src\nvim\testdir" -- VERBOSE=1 
[00:14:33] Error: could not load cache
[00:14:33] 
[00:14:33] C:\projects\neovim\build>endlocal

janlazo · 2018-03-09T20:34:21Z

Exit codes are still inconsistent even with cmd.exe but this is more consistent on stdout than powershell.
Powershell sometimes doesn't execute a command if the last argument and \r\n aren't separated by whitespace so, when it happens to me, I press Enter again for it to run.

I'll create cat.cmd alternative using cmd.exe built-ins to get more consistent exit codes. cat.exe is covered by other tests anyway.

justinmk · 2018-03-09T20:37:17Z

Weird. I wonder if we should just write our own cat.exe in C, instead of this old gnu binary I found ...

justinmk · 2018-03-09T23:22:13Z

test/functional/core/job_spec.lua

@@ -138,7 +139,7 @@ describe('jobs', function()
  end)

  it('allows interactive commands', function()
-    nvim('command', "let j = jobstart(['cat', '-'], g:job_opts)")
+    nvim('command', "let j = jobstart(['"..nvim_cat.."', '-'], g:job_opts)")


Doesn't foo.cmd require a shell? So I would not expect this to work, the jobstart({list}) form skips the shell.

That's one reason why including cat.exe was important.

And changing these invocations to use the shell form of jobstart({string}) changes the nature of the tests, so we don't want to do that.

You're right. I'll keep the list form and run the batchfile with cmd.exe directly. It should be fine if the path of the tempfile doesn't have to be escaped.

janlazo · 2018-03-10T03:01:58Z

find.exe /v "" (see comments in https://superuser.com/a/853718) is better for multiple newlines. It doesn't preserve NULs though.

~~We can use cat.exe for testing the newline -> NUL conversion without checking the error code.~~ Nevermind. invalid channel id error still happens.

find.exe requires that on each jobsend, it's either \r\n or \n\n. On uneven # of consecutive \n, prepend the last \n with \r.

janlazo · 2018-03-11T19:31:00Z

@justinmk How does jobstop work in Windows? IIRC, Windows doesn't support signals so killed processes can't clean up.

justinmk · 2018-03-11T19:44:18Z

@janlazo Do you mean grandchildren? We will need to iterate through the process tree.

See terminate_all in Vim, which was basically copied from https://stackoverflow.com/q/1173342 . After a long search, I concluded that's the only way to do it, except another approach used by libuv (see uv__init_global_job_handle): create a win32 job object (but that has a quirk in Windows 7 and older: https://stackoverflow.com/a/6138203 ).

The above will be implemented in #8107 .

If you meant something else, please give a specific example.

justinmk · 2018-03-12T15:55:24Z

test/functional/core/job_spec.lua

+    nvim('command', 'call jobsend(j, "'..(iswin()
+      and [[a\n\nc\n\n]]
+      or  [[a\n\nc\n\n\n\nb\n\n]]
+    )..'")')


Would rather keep the test pending than have a lot of special-case code that is difficult to follow, just to get the test working. Maybe something can be improved in the future so that the test can pass without requiring lots of weird workarounds.

janlazo · 2018-03-13T02:04:37Z

That should be the last of it (unless cat.exe fails again or find.exe errors on jobstop).
I can port my changes for powershell from #8034 to test jobwait in the meantime.

justinmk · 2018-03-17T23:37:07Z

test/functional/core/job_spec.lua

+  return os.execute((iswin()
+    and 'tasklist /nh /fi "PID eq '..pid..'" | findstr '..pid..' > nul'
+    or  'ps -p '..pid..' > /dev/null'))
+end


#8107 provides nvim_get_proc({pid}) which can be used in the tests via helpers.meths.get_proc(). It returns helpers.NIL if the process doesn't exist.

I need to return an exit code though.
Need something like nvim_get_proc(pid) == helpers.NIL and 1 or 0.
Or I can use it directly on the jobpid test.

Why do you need an exit code? If nvim_get_proc returns NIL then the process wasn't found. Non-NIL means it was found.

justinmk · 2018-03-17T23:39:29Z

test/functional/core/job_spec.lua

+  return os.execute((iswin()
+    and 'taskkill /f /t /pid '..pid..' > nul'
+    or  'ps -p '..pid..' > /dev/null'))
+end


Would be easy to expose nvim_kill_proc() if this turns out to be unreliable.

Can revert this after neovim#8120.

This reverts commit ae409b5. This PR (neovim#8120) defaults to cmd.exe for job_spec.lua

justinmk · 2018-03-19T07:06:26Z

test/functional/core/job_spec.lua

    nvim('command', 'call jobsend(j, "abc\\nxyz")')
-    eq({'notification', 'stdout', {0, {'abc', 'xyz'}}}, next_msg())
+    eq({'notification', 'stdout', {0, iswin() and {'abc', ''} or {'abc', 'xyz'}}}, next_msg())


Probably should used expect_msg_seq

For this line only? This is expected behaviour for find.exe because the string has to end with newline (or some character for find.exe to receive the input). As for the stdout after jobstop, I don't know why.

I can experiment with expect_msg_seq for both stdout.

On 2nd thought, I'll revert it. Leave it for cat when the situation improves. There's enough tests for find.exe anyway.

justinmk · 2018-03-19T07:11:05Z

test/functional/core/job_spec.lua

+      -- find.exe can't handle 3+ consecutive newlines consistently
+      nvim('command', "let j = jobstart(['find', '/v', ''], g:job_opts)")
+      nvim('command', [[call jobsend(j, "a\n\nc\n\n")]])
+      expect_msg_seq(


If it's not consistent then just mark it pending. The entire purpose of this test is to check that newlines are preserved (consistently).

I'll revert it back to pending. Revist when testing with cat.exe gets better.

justinmk · 2018-03-19T07:13:47Z

test/functional/core/job_spec.lua

    nvim('command', "call jobstop(j)")
+    if iswin() then
+      eq({'notification', 'stdout', {0, {'final nl', ''}}}, next_msg())


This output has the newline, doesn't it? That defeats the purpose of the test. Should just be pending.

It's leftover from stdout when find.exe exits for some reason. Don't recall this happening on a regular cmd.exe window. Strange that this doesn't happen on jobclose(g:job_id, "stdin")

justinmk · 2018-03-19T07:32:56Z

test/functional/core/job_spec.lua

    -- clean up after ourselves
-    os.execute('kill -9 '..pid..' > /dev/null')
+    os_kill(pid)
+    neq(0, os_ps(pid))


It should be NOT found since we killed it.

I'm checking if the os_kill did work. Is there a better way to do it or do you want to check the exit code of os_kill?

@janlazo Ah, I didn't realize os_ps was returned the exit code, I thought it returned a pid. Anyways nvim_get_proc should be used instead.

justinmk · 2018-03-19T07:34:56Z

test/functional/core/job_spec.lua

+      \  jobstart('Start-Sleep -Milliseconds 70; exit 4'),
+      \  jobstart('Start-Sleep -Milliseconds 50; exit 5'),
+      \  jobstart('Start-Sleep -Milliseconds 30; exit 6'),
+      \  jobstart('Start-Sleep -Milliseconds 10; exit 7')


This is going to be unreliable because powershell startup takes 600ms at least.

Bump the sleep timeout to (1)700, (1)500, (1)300, (1)100 or use cmd.exe and ping instead?
The timeout test for jobwait lasts up to 2-3s so it's long wait either way.

I think cmd.exe would be a better choice here.

The timeout test for jobwait lasts up to 2-3s so it's long wait either way.

Yes but I'm thinking of the variance in event ordering, e.g. if powershell starts 20ms faster in one case.

Likely because of Powershell's lazy-loading and Appveyor-specific modules. Can't prevent them with -NoProfile.

justinmk · 2018-03-19T07:37:49Z

test/functional/core/job_spec.lua

@@ -775,22 +840,36 @@ describe("pty process teardown", function()
  end)

  it("does not prevent/delay exit. #4798 #4900", function()


Could use nvim_get_proc_children instead of a screen test.

For the PIDs? It won't work with the child process launched with start (cmd.exe). Have to rethink this test for Windows. Anything launched with start should be killed separately.

Not sure what you mean, but if !start is detached from nvim somehow, such that it wouldn't prevent nvim from exiting (if there were a nvim bug in its jobs teardown routine), then that defeats the purpose of the test.

nvim will exit completely and !start terminates after running the program (ie. ping) but the program that start does run (ie ping) will not necessarily terminate if nvim terminates.
That said, closing :terminal seems to terminate programs run with start /b.

but the program that start does run (ie ping) will not necessarily terminate if nvim terminates.

That defeats the purpose of the test :)

This reverts commit ae409b5. This PR (neovim#8120) defaults to cmd.exe for job_spec.lua

cmd.exe (shell) is faster and more reliable than powershell (.NET frontend). It's best for short and basic tests that don't require non-trivial scripting. cmd.exe doesn't support sleep so use powershell's Start-Sleep as substitute.

This reverts commit ae409b5. This PR (neovim#8120) defaults to cmd.exe for job_spec.lua

Use ping to test job detach Use find.exe as an alternative to cat.exe Use nvim_get_proc to check pid

justinmk approved these changes Mar 9, 2018

View reviewed changes

justinmk reviewed Mar 9, 2018

View reviewed changes

janlazo force-pushed the win_job_cmd branch 2 times, most recently from 548ac58 to c68f10e Compare March 10, 2018 02:46

janlazo force-pushed the win_job_cmd branch from f95b10f to 44240d8 Compare March 11, 2018 14:56

janlazo force-pushed the win_job_cmd branch from 335b985 to bd109f2 Compare March 11, 2018 19:33

janlazo force-pushed the win_job_cmd branch 4 times, most recently from 79ac88b to 2cc77ff Compare March 12, 2018 14:58

justinmk reviewed Mar 12, 2018

View reviewed changes

janlazo changed the title ~~test: win: use cmd.exe by default in job_spec.lua~~ [RFC] test: win: use cmd.exe by default in job_spec.lua Mar 13, 2018

marvim added the RFC label Mar 13, 2018

janlazo force-pushed the win_job_cmd branch 3 times, most recently from d4ec323 to db27b09 Compare March 13, 2018 21:18

janlazo mentioned this pull request Mar 13, 2018

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

Closed

janlazo force-pushed the win_job_cmd branch 2 times, most recently from 075e5c3 to 63d3b76 Compare March 17, 2018 15:52

justinmk reviewed Mar 17, 2018

View reviewed changes

justinmk added a commit to justinmk/neovim that referenced this pull request Mar 18, 2018

test/win: use cmd.exe for test

ae409b5

Can revert this after neovim#8120.

janlazo force-pushed the win_job_cmd branch from e109641 to 1fd2643 Compare March 18, 2018 23:33

janlazo added a commit to janlazo/neovim that referenced this pull request Mar 18, 2018

Revert "test/win: use cmd.exe for test"

9d23641

This reverts commit ae409b5. This PR (neovim#8120) defaults to cmd.exe for job_spec.lua

janlazo force-pushed the win_job_cmd branch 2 times, most recently from 66a5437 to bf83b3e Compare March 19, 2018 06:08

justinmk reviewed Mar 19, 2018

View reviewed changes

janlazo force-pushed the win_job_cmd branch from 39960e9 to 6fbeee4 Compare March 20, 2018 16:34

janlazo added a commit to janlazo/neovim that referenced this pull request Mar 20, 2018

Revert "test/win: use cmd.exe for test"

e0a0cc2

This reverts commit ae409b5. This PR (neovim#8120) defaults to cmd.exe for job_spec.lua

janlazo force-pushed the win_job_cmd branch from 5965ac0 to 27714a6 Compare March 26, 2018 05:38

janlazo added a commit to janlazo/neovim that referenced this pull request Mar 26, 2018

Revert "test/win: use cmd.exe for test"

7ea0f7f

This reverts commit ae409b5. This PR (neovim#8120) defaults to cmd.exe for job_spec.lua

janlazo added 6 commits March 26, 2018 01:45

test: win: use powershell for Start-Sleep only

cf4fbb6

cmd.exe (shell) is faster and more reliable than powershell (.NET frontend). It's best for short and basic tests that don't require non-trivial scripting. cmd.exe doesn't support sleep so use powershell's Start-Sleep as substitute.

test: win: try stderr callback and slow output

3bd555f

test: win: use find.exe /v "" as alternative cat -

03e69a5

Revert "test/win: use cmd.exe for test"

e6ee06e

This reverts commit ae409b5. This PR (neovim#8120) defaults to cmd.exe for job_spec.lua

fixup: ping.exe works with cmd.exe pipe

ad6d577

test: win: enable jobpid() tests

51f5cfc

Use ping to test job detach Use find.exe as an alternative to cat.exe Use nvim_get_proc to check pid

janlazo force-pushed the win_job_cmd branch 2 times, most recently from fda26ed to 73d48b7 Compare March 27, 2018 02:34

janlazo changed the title ~~[RFC] test: win: use cmd.exe by default in job_spec.lua~~ [RDY] test: win: use cmd.exe by default in job_spec.lua Mar 27, 2018

marvim added RDY and removed RFC labels Mar 27, 2018

janlazo closed this Apr 14, 2018

jszakmeister removed the RDY label Apr 14, 2018

justinmk added platform:windows test labels Apr 15, 2018

janlazo reopened this Apr 15, 2018

janlazo force-pushed the win_job_cmd branch from 73d48b7 to 51f5cfc Compare April 15, 2018 01:02

marvim added the RDY label Apr 15, 2018

justinmk merged commit 7598e6c into neovim:master Apr 15, 2018

justinmk removed the RDY label Apr 15, 2018

janlazo deleted the win_job_cmd branch May 28, 2018 15:09

		@@ -775,22 +840,36 @@ describe("pty process teardown", function()
		end)

		it("does not prevent/delay exit. #4798 #4900", function()

Uh oh!

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

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

Uh oh!

Conversation

janlazo commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Mar 9, 2018

Uh oh!

janlazo commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janlazo commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janlazo Mar 10, 2018 • 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!

janlazo commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janlazo commented Mar 11, 2018

Uh oh!

justinmk commented Mar 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janlazo commented Mar 13, 2018

Uh oh!

justinmk Mar 17, 2018 • 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!

janlazo Mar 19, 2018 • 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!

justinmk Mar 20, 2018 • 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!

janlazo Mar 19, 2018 • 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!

janlazo Mar 19, 2018 • 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!

janlazo Mar 19, 2018 • 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!

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

janlazo commented Mar 9, 2018 •

edited

Loading

janlazo commented Mar 9, 2018 •

edited

Loading

janlazo commented Mar 9, 2018 •

edited

Loading

janlazo Mar 10, 2018 •

edited

Loading

janlazo commented Mar 10, 2018 •

edited

Loading

justinmk Mar 17, 2018 •

edited

Loading

janlazo Mar 19, 2018 •

edited

Loading

justinmk Mar 20, 2018 •

edited

Loading

janlazo Mar 19, 2018 •

edited

Loading

janlazo Mar 19, 2018 •

edited

Loading

janlazo Mar 19, 2018 •

edited

Loading

justinmk Mar 20, 2018 •

edited

Loading

janlazo Mar 19, 2018 •

edited

Loading

janlazo Mar 20, 2018 •

edited

Loading