Add support for NeomakeJobCancelled user event

[Furkanzmc]

Closes issue #2492.

[blueyed]

Thanks already.
What do you think about calling NeomakeJobFinished instead always?

[Furkanzmc]

Makes sense.

I'll omit the exit_code in job info then. Are you OK with that?

[blueyed]

Thanks for the update.
Haven't looked at CI failures, but the current approach seems fine to me.
Please update, or let me know if I should do it.

[Furkanzmc]

I made the changes you requested.

Haven't looked at CI failures, but the current approach seems fine to me.

I wasn't able to see anything related to my changes in the logs. It was hard for me to see what's going on as I'm not used to the output there. So, I really don't know what is causing the failure.

[blueyed]

Thanks for the update!
CI failures are likely fixed after rebasing on master maybe. You can merge it only, will squash-merge it anyway.
Also let me know if I should do it / take it from here.

[blueyed]

Now there are relevant test failures, e.g.

(433/709) [EXECUTE] Handle finished job that got canceled (#1158)
      > [D +0.02]: [248.-:3:1] Running makers: success-maker.
      > [debug  ]: Calling User autocmd NeomakeCountsChanged with context: {'bufnr': 3, 'file_mode': 0, 'reset': 1}.
      > [verbose]: [248.233:3:1] Starting [string]: /bin/bash -c 'echo success'.
      > [debug  ]: [248.233:3:1] cwd: /home/neomake/repo.
      > [D +0.01]: [248.233:3:1] output on stdout: ['success', ''].
      > [debug  ]: [248.233:3:1] exit: success-maker: 0.
      > [debug  ]: [248.233:3:1] Not processing output for mode "V".
      > [debug  ]: [248.233:3:1] Queuing action process_pending_output for BufEnter, WinEnter, InsertLeave, CursorHold, CursorHoldI.
      > [debug  ]: [248.233:3:1] Output left to be processed, not cleaning job yet.
      > [debug  ]: [248.-:3:1] Skipping cleaning of make info: 1 active jobs: ['Job 233: success-maker'].
      > [debug  ]: [249.-:3:1] Running makers: success-maker.
      > [verbose]: [249.-:-:1] Canceling already running job (248.233) for the same maker.
      > [debug  ]: [248.233:3:1] Canceling job.
      > [debug  ]: [248.233:3:1] Removed 1 action queue entries.
      > [debug  ]: [248.233:3:1] Job exited already.
      > [debug  ]: [248.233:3:1] Cleaning jobinfo.
      > [debug  ]: [248.233:3:1] Calling User autocmd NeomakeJobFinished with context: {'jobinfo': 'Job 233: success-maker [canceled, finished]'}.
      > [debug  ]: [248.-:3:1] Cleaning make info.
      > [verbose]: [249.234:3:1] Starting [string]: /bin/bash -c 'echo success'.
      > [debug  ]: [249.234:3:1] cwd: /home/neomake/repo.
      > [D +0.01]: [249.234:3:1] output on stdout: ['success', ''].
      > [debug  ]: [249.234:3:1] exit: success-maker: 0.
      > [debug  ]: [249.234:3:1] Not processing output for mode "V".
      > [debug  ]: [249.234:3:1] Queuing action process_pending_output for BufEnter, WinEnter, InsertLeave, CursorHold, CursorHoldI.
      > [debug  ]: [249.234:3:1] Output left to be processed, not cleaning job yet.
      > [debug  ]: [249.-:3:1] Skipping cleaning of make info: 1 active jobs: ['Job 234: success-maker'].
    (433/709) [EXECUTE] (X) 0 should be equal to 1
      > /home/neomake/repo/tests/processing.vader:532: AssertEqual len(g:neomake_test_jobfinished), 0

(where the canceled job is handled as "finished" now, i.e. the test should get adjusted)

As for running the tests locally, try make test, and make tests/processing.vader to run only that file.

[Furkanzmc]

I squashed the commits and I'll try to address the tests as well. Thanks for pointing out the related failures.

[blueyed]

I think it makes more sense to adjust g:neomake_test_jobfinished in the tests, and add g:neomake_test_jobcanceled:

 autoload/neomake.vim   | 1 -
 tests/include/init.vim | 7 ++++++-
 tests/processing.vader | 5 ++++-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git i/autoload/neomake.vim w/autoload/neomake.vim
index 2046dff5f..5be477761 100644
--- i/autoload/neomake.vim
+++ w/autoload/neomake.vim
@@ -1375,7 +1375,6 @@ function! s:CleanJobinfo(jobinfo, ...) abort
         if has_key(a:jobinfo, 'exit_code')
             call remove(a:jobinfo, 'exit_code')
         endif
-
         call neomake#utils#hook('NeomakeJobFinished', {'jobinfo': a:jobinfo})
     endif
 
diff --git i/tests/include/init.vim w/tests/include/init.vim
index 04119b0b3..ddedee212 100644
--- i/tests/include/init.vim
+++ w/tests/include/init.vim
@@ -288,8 +288,13 @@ function! g:NeomakeSetupAutocmdWrappers()
   endfunction
 
   let g:neomake_test_jobfinished = []
+  let g:neomake_test_jobcanceled = []
   function! s:OnNeomakeJobFinished(context)
-    let g:neomake_test_jobfinished += [a:context]
+    if get(a:context.jobinfo, 'canceled', 0)
+      let g:neomake_test_jobcanceled += [a:context]
+    else
+      let g:neomake_test_jobfinished += [a:context]
+    endif
   endfunction
 
   let g:neomake_test_countschanged = []
diff --git i/tests/processing.vader w/tests/processing.vader
index a9059bc2b..2cd7fce2c 100644
--- i/tests/processing.vader
+++ w/tests/processing.vader
@@ -529,11 +529,14 @@ Execute (Handle finished job that got canceled (#1158)):
   AssertNeomakeMessage 'Not processing output for mode "V".', 3
   let jobs = neomake#Make(0, [g:success_maker])
   AssertNeomakeMessage '\mCanceling already running job (\d\+.\d\+) for the same maker.'
-  AssertEqual len(g:neomake_test_jobfinished), 1
+  AssertEqual len(g:neomake_test_jobfinished), 0
+  AssertEqual len(g:neomake_test_jobcanceled), 1
   call neomake#CancelJob(jobs[0])
   if neomake#has_async_support()
     NeomakeTestsWaitForFinishedJobs
   endif
+  AssertEqual len(g:neomake_test_jobfinished), 0
+  AssertEqual len(g:neomake_test_jobcanceled), 2
 
 Execute (Job does not get restarted when canceled):
   if NeomakeAsyncTestsSetup()

Sorry for not reviewing it more properly before.

[blueyed]

Also I think that exit_code should be kept actually: it might contain useful information, i.e. when another exit code is used for when a process gets terminated etc.
Instead I think you should check for the canceled attribute to figure out if a job was canceled.
What do you think?

(I can push the above changes, and also add what I suggested above if you agree)

[Furkanzmc]

when another exit code is used for when a process gets terminated etc.

Can you elaborate on this? I don't understand what you mean.

---

I haven't thought about it before, but since we switched to the same event name for when a job is cancelled, removing the exit code will break existing configurations that rely on the exit_code. I should have thought about that before.

You are right, it makes sense. You can push your changes. Please let me know If there's additional things I need to do. Do the changes you made take care of the tests? I can make additional changes if not.

[blueyed]

when another exit code is used for when a process gets terminated etc.

Can you elaborate on this? I don't understand what you mean.

A job might handle e.g. SIGTERM and exit in a special way, and/or the signal status gets reflected there etc.
I.e. it is useful to have it also when canceled/terminated.

You are right, it makes sense. You can push your changes. Please let me know If there's additional things I need to do. Do the changes you made take care of the tests? I can make additional changes if not.

Cool, will do so later, probably tomorrow/during the week. I will take of the tests also.