-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
runtime(termdebug): Fix #12718 #14792
Conversation
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 |
Sure thing! Yes, there are some other places where this could happen: pretty much everywhere where |
that sounds perfect. Regarding the tests, just check the existing termdebug test. It's not so hard, you basically need to create a |
.. working on it. Please ignore all the pushes until I notify. |
Check it now! |
It seems some tests failed due to the following:
Most likely it is due to lines 319 and 332 of |
The error occurs here:
|
Ready for review. :) |
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 |
Changes implemented. |
Hum... still does not pass some CI tests. I am wondering if the following shall be changed somewhere as well...
|
Hmm, it passed in my repository... |
Hum... could you double check my changes to check if I did everything right? |
Co-authored-by: K.Takata <kentkt@csc.jp>
Usch! Hopefully one day someone will make a Vim/Vim9 language server to catch these typos :D |
src/testdir/test_termdebug.vim
Outdated
call writefile(['This', 'is', 'a', 'test'], filename, 'D') | ||
" Throw away the file once the test has done. | ||
Termdebug | ||
sleep 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using hard-coded sleeps in tests will increase the time it takes to run the tests. Can you change these
to WaitForAssert() calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
src/testdir/test_termdebug.vim
Outdated
" Check only the head of the error message | ||
let error_message = "You have a file/folder named '" .. filename .. "'" | ||
Termdebug | ||
sleep 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you replace this hard-coded sleep with WaitForAssert()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWESOME! I am going to give it a shot straight away. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go up and directory and make vim first (cd .. && make)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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>
Hi!
This is my proposal for fixing #12718
I could have done something even more robust, like
Shall the same
if-then-else
condition be placed in other lines of the formfile <hard-coded name>
?OBS! I am not in the vim-dev mailing list!