runtime(termdebug): Fix #12718 #14792

ubaldot · 2024-05-17T11:55:22Z

Hi!

This is my proposal for fixing #12718
I could have done something even more robust, like

if empty(glob('gdb'))
     file gdb
  elseif empty(glob(' Termdebug-gdb-console'))
     file Termdebug-gdb-console
  else
    Echoerr("You have a file/folder named 'gdb' or 'Termdebug-gdb-console'. Termdebug cannot start. Please rename them. ")
endif

Shall the same if-then-else condition be placed in other lines of the form file <hard-coded name> ?

OBS! I am not in the vim-dev mailing list!

chrisbra · 2024-05-17T16:16:27Z

Thanks, are there other places where this could clash? I think the warning approach is good, although you have a leading white space inside your glob() function. Can you fix this up and add a test to https://github.com/vim/vim/blob/master/src/testdir/test_termdebug.vim ?

ubaldot · 2024-05-17T16:30:44Z

Sure thing! Yes, there are some other places where this could happen: pretty much everywhere where file command is used. You could /file in the source and see that you have it in other 2-3 places. I can use the same solution there as well. Regarding the tests: I never wrote a test for vimscripts, if there is some guideline it would be great to read! Finally: I will try to fix it during the weekend if I will find some time or during next week. Would that work?

chrisbra · 2024-05-17T16:37:42Z

that sounds perfect. Regarding the tests, just check the existing termdebug test. It's not so hard, you basically need to create a gdb file and then check that either the plugin is not loaded (so you are still in a clean state, e.g. only one single buffer opened), or check if you can see the error message in the messages list. Or something like this :) Thanks for looking into this!

runtime/pack/dist/opt/termdebug/plugin/termdebug.vim

src/testdir/test_termdebug.vim

ubaldot · 2024-05-20T08:58:39Z

.. working on it. Please ignore all the pushes until I notify.

src/testdir/test_termdebug.vim

ubaldot · 2024-05-20T11:44:09Z

Check it now!
(yes, the previous bureplacement_filename was a typo!)

ubaldot · 2024-05-20T14:57:56Z

It seems some tests failed due to the following:

Failures: 
	From test_termdebug.vim:
	Found errors in Test_termdebug_bufnames():
	Caught exception in Test_termdebug_bufnames(): Vim(bwipeout):E517: No buffers were wiped out: bwipe! 12 @ command line..script /home/runner/work/vim/vim/src/testdir/runtest.vim[607]..function

Most likely it is due to lines 319 and 332 of [src/testdir/test_termdebug.vim](https://github.com/vim/vim/pull/14792/files#diff-07567861d8244d8d0078cb5d13045a8fb5d438ec20af2d2341f648ca6d3b05f7) and it is not super clear to me how to solve it... any hints?

k-takata · 2024-05-20T20:36:14Z

RunTheTest[57]..Test_termdebug_bufnames[20]..9_EndPromptDebug[9]..9_EndDebugCommon, line 4

The error occurs here:

vim/runtime/pack/dist/opt/termdebug/plugin/termdebug.vim

Line 741 in 648aedf

exe 'bwipe! ' . s:ptybuf

src/testdir/test_termdebug.vim

ubaldot · 2024-05-21T08:08:15Z

Ready for review. :)

k-takata · 2024-05-21T10:48:55Z

It seems that the following patch fixes the CI failure:

--- a/src/testdir/test_termdebug.vim
+++ b/src/testdir/test_termdebug.vim
@@ -317,10 +317,9 @@ func Test_termdebug_bufnames()
   call WaitForAssert({-> assert_false(bufexists(filename))})
   call WaitForAssert({-> assert_true(bufexists(replacement_filename))})
   quit!
+  call WaitForAssert({-> assert_equal(1, winnr('$'))})
 
   " Check if error message is in :message
-  " Delay for securing that Termdebug is shutoff
-  sleep 1
   let g:termdebug_config['disasm_window'] = 1
   let filename = 'Termdebug-asm-listing'
   call writefile(['This', 'is', 'a', 'test'], filename, 'D')
@@ -328,10 +327,11 @@ func Test_termdebug_bufnames()
   let error_message = "You have a file/folder named '" .. filename .. "'"
   Termdebug
   sleep 2
-  call WaitForAssert({->assert_notequal(-1, stridx(execute('messages'), error_message))})
+  call WaitForAssert({-> assert_notequal(-1, stridx(execute('messages'), error_message))})
   quit!
-  wincmd t
+  wincmd b
   wincmd q
+  call WaitForAssert({-> assert_equal(1, winnr('$'))})
 
   unlet g:termdebug_config
 endfunc

ubaldot · 2024-05-21T11:15:04Z

Changes implemented.

ubaldot · 2024-05-21T11:49:59Z

Hum... still does not pass some CI tests.

I am wondering if the following shall be changed somewhere as well...

--- a/src/testdir/test_termdebug.vim
+++ b/src/testdir/test_termdebug.vim

k-takata · 2024-05-21T12:14:00Z

Hmm, it passed in my repository...
https://github.com/k-takata/vim/actions/runs/9173423210
Commit: k-takata@f9523a2

src/testdir/test_termdebug.vim

ubaldot · 2024-05-21T12:15:46Z

Hum... could you double check my changes to check if I did everything right?

Co-authored-by: K.Takata <kentkt@csc.jp>

ubaldot · 2024-05-21T12:20:58Z

Usch! Hopefully one day someone will make a Vim/Vim9 language server to catch these typos :D

yegappan · 2024-05-21T13:45:26Z

src/testdir/test_termdebug.vim

+  call writefile(['This', 'is', 'a', 'test'], filename, 'D')
+  " Throw away the file once the test has done.
+  Termdebug
+  sleep 2


Using hard-coded sleeps in tests will increase the time it takes to run the tests. Can you change these
to WaitForAssert() calls?

Ok, I replaced the sleep 2 with call WaitForAssert({-> assert_equal(3, winnr('$'))} , which, in my understanding, wait until the third window is available (in this case, given the termdebug setup, it means that termdebug is up and running).

yegappan · 2024-05-21T14:48:56Z

src/testdir/test_termdebug.vim

+  " Check only the head of the error message
+  let error_message = "You have a file/folder named '" .. filename .. "'"
+  Termdebug
+  sleep 2


Can you replace this hard-coded sleep with WaitForAssert()?

Done! Sorry for so many commits, it's my first time. I wish I could test the CI on my fork first... shall I cancel the PR if the CI still fails?

No worries. After all, that's what we have CI for ;) BTW: you can also run the tests locally: https://github.com/vim/vim/blob/master/src/testdir/README.txt

Just need to run make test_termdebug.res and then check the test.log and messages file.

AWESOME! I am going to give it a shot straight away. :)

From the README

RUNNING THE TESTS: To run a single test from the src directory: $ make test_<name> The below commands should be run from the src/testdir directory. To run a single test: $ make test_<name>.res

I am in ./vim/src/testdir but when I run make test_termdebug.res I get make: *** No rule to make target '../vim', needed by 'test_termdebug.res'. Stop. Perhaps there is some requirement that I am missing?

EDIT:
make VIMPROG=/usr/bin/vim test_termdebug.res seems working...

go up and directory and make vim first (cd .. && make)

Thanks!
I built the vim executable. The test run but it seems that my tests are not run:

$ make test_termdebug.res VIMRUNTIME=../../runtime ../vim -f -u unix.vim --gui-dialog-file guidialog -U NONE --noplugin --not-a-term -S runtest.vim test_termdebug.vim --cmd 'au SwapExists * let v:swapchoice = "e"' | LC_ALL=C LANG=C LANGUAGE=C awk '/Executing Test_/{match($0, "([0-9][0-9]:[0-9][0-9] *)?Executing Test_[^\\)]*\\)"); print substr($0, RSTART, RLENGTH) "\r"; fflush()}' 02:05 Executing Test_termdebug_basic() 02:09 Executing Test_termdebug_bufnames() 02:09 Executing Test_termdebug_mapping() 02:09 Executing Test_termdebug_tbreak()

It is missing Test_termdebug_bufnames(). Shall I add it to some other file?

chrisbra · 2024-05-21T21:33:45Z

thanks, looks good now.

runtime(termdebug): check for gdb file/dir before using as buffer name Add test so that this doesn't regress. fixes: vim/vim#12718 closes: vim/vim#14792 vim/vim@62ccaa6 Co-authored-by: Ubaldo Tiberi <ubaldo.tiberi@volvo.com>

…using as buffer name Add test so that this doesn't regress. fixes: vim/vim#12718 closes: vim/vim#14792 vim/vim@62ccaa6 Co-authored-by: Ubaldo Tiberi <ubaldo.tiberi@volvo.com>

…using as buffer name (#28908) Add test so that this doesn't regress. fixes: vim/vim#12718 closes: vim/vim#14792 vim/vim@62ccaa6 Co-authored-by: Ubaldo Tiberi <ubaldo.tiberi@volvo.com>

…using as buffer name (neovim#28908) Add test so that this doesn't regress. fixes: vim/vim#12718 closes: vim/vim#14792 vim/vim@62ccaa6 Co-authored-by: Ubaldo Tiberi <ubaldo.tiberi@volvo.com>

Fix vim#12718

c8b78ca

github-actions bot added runtime termdebug labels May 17, 2024

ubaldot added 3 commits May 20, 2024 10:33

Added simple test

d2755c5

fixing tests...

53e243f

.

3b3a42e

k-takata reviewed May 20, 2024

View reviewed changes

k-takata changed the title ~~Fix #12718~~ runtime(termdebug): Fix #12718 May 20, 2024

Tests should work now

52efd50

k-takata reviewed May 20, 2024

View reviewed changes

src/testdir/test_termdebug.vim Outdated Show resolved Hide resolved

Added check on error messages list

158668f

A typo was still there... now it should be fixed

648aedf

k-takata reviewed May 20, 2024

View reviewed changes

src/testdir/test_termdebug.vim Outdated Show resolved Hide resolved

src/testdir/test_termdebug.vim Outdated Show resolved Hide resolved

src/testdir/test_termdebug.vim Outdated Show resolved Hide resolved

src/testdir/test_termdebug.vim Outdated Show resolved Hide resolved

ubaldot added 2 commits May 21, 2024 10:04

tests should be fixed

60984af

Implemented suggestions

93e369f

execute 'Termdebug'-> Termdebug

61f703f

Changes for fixing CI failures implementes

f0ff599

k-takata reviewed May 21, 2024

View reviewed changes

src/testdir/test_termdebug.vim Outdated Show resolved Hide resolved

Update src/testdir/test_termdebug.vim

bf3e7b8

Co-authored-by: K.Takata <kentkt@csc.jp>

yegappan reviewed May 21, 2024

View reviewed changes

Replaced hardcoded sleep with WaitForAssert()

add255f

yegappan reviewed May 21, 2024

View reviewed changes

ubaldot added 3 commits May 21, 2024 17:45

Replaced the second sleep 2 statement and fixed typos

5777753

Fixed typos

9edce5a

Fixed typos

2765ce8

chrisbra closed this in 62ccaa6 May 21, 2024

zeertzjq mentioned this pull request May 22, 2024

vim-patch:62ccaa6: runtime(termdebug): check for gdb file/dir before using as buffer name neovim/neovim#28908

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime(termdebug): Fix #12718 #14792

runtime(termdebug): Fix #12718 #14792

ubaldot commented May 17, 2024 •

edited

chrisbra commented May 17, 2024

ubaldot commented May 17, 2024

chrisbra commented May 17, 2024

ubaldot commented May 20, 2024

ubaldot commented May 20, 2024

ubaldot commented May 20, 2024

k-takata commented May 20, 2024

ubaldot commented May 21, 2024

k-takata commented May 21, 2024

ubaldot commented May 21, 2024

ubaldot commented May 21, 2024

k-takata commented May 21, 2024

ubaldot commented May 21, 2024

ubaldot commented May 21, 2024

yegappan May 21, 2024

ubaldot May 21, 2024

yegappan May 21, 2024

ubaldot May 21, 2024

chrisbra May 21, 2024

ubaldot May 21, 2024

ubaldot May 21, 2024 •

edited

chrisbra May 21, 2024 •

edited

ubaldot May 21, 2024

chrisbra commented May 21, 2024

runtime(termdebug): Fix #12718 #14792

runtime(termdebug): Fix #12718 #14792

Conversation

ubaldot commented May 17, 2024 • edited

chrisbra commented May 17, 2024

ubaldot commented May 17, 2024

chrisbra commented May 17, 2024

ubaldot commented May 20, 2024

ubaldot commented May 20, 2024

ubaldot commented May 20, 2024

k-takata commented May 20, 2024

ubaldot commented May 21, 2024

k-takata commented May 21, 2024

ubaldot commented May 21, 2024

ubaldot commented May 21, 2024

k-takata commented May 21, 2024

ubaldot commented May 21, 2024

ubaldot commented May 21, 2024

yegappan May 21, 2024

Choose a reason for hiding this comment

ubaldot May 21, 2024

Choose a reason for hiding this comment

yegappan May 21, 2024

Choose a reason for hiding this comment

ubaldot May 21, 2024

Choose a reason for hiding this comment

chrisbra May 21, 2024

Choose a reason for hiding this comment

ubaldot May 21, 2024

Choose a reason for hiding this comment

ubaldot May 21, 2024 • edited

Choose a reason for hiding this comment

chrisbra May 21, 2024 • edited

Choose a reason for hiding this comment

ubaldot May 21, 2024

Choose a reason for hiding this comment

chrisbra commented May 21, 2024

ubaldot commented May 17, 2024 •

edited

ubaldot May 21, 2024 •

edited

chrisbra May 21, 2024 •

edited