[vim/vim] Some vim9 remote tests will fail if GVIM server exists (Issue #9723)

[Michael Soyka]

Steps to reproduce

1. start a GVIM server
2. build vim (features, etc unimportant, I think)
3. run test: make test_vim9_builtin.res
4. view file test.log

You will see that tests that use an empty string for the servername have failed.
For example, the call remote_expr("","") in Test_remote_expr() should fail with error E241- "Unable to send to ", but instead fails with error E449- "Invalid expression received". You will also see the same result for the remote_foreground("") call in Test_remote_foreground(). Finally, the remote_send("","") call in Test_remote_send() should fail with error E241 but does not fail.

These tests fail because a GVIM server is running while the tests are being executed.

Expected behaviour

These tests will pass if a GVIM server is not running during the test.

Version of Vim

8.2.4330

Environment

Linux (I don't think anything else matters)

Logs and stack traces

These tests fail because Vim internally replaces the empty string servername with "GVIM".  I couldn't find anything in the help pages for remote_expr(), et al that documents this policy.  

I suspect you haven't seen this in your test environment because your build procedure creates an executable named "vim" even if it includes GUI support.

—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/issues/9723@github.com>

[Bram Moolenaar]

Yeah, the tests sometimes don't work when something in the environment interferes. That is unavoidable. You can run the build and test process in a separate container if needed. The CI works that way.
I don't think we can fix this without losing test coverage.

—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/issues/9723/1037190064@github.com>

[Michael Soyka]

I agree that it is undesirable to reduce test coverage. However, if the purpose of the tests is to verify failure to send because a server does not exist, why not use some unlikely name for the server as is done in Test_remote_foreground():

assert_fails('remote_foreground("NonExistingServer")', 'E241:')
assert_fails('remote_foreground("")', 'E241:')

The first call makes the purpose of the test clear. As a side note, isn't the second call redundant or is its purpose to test the substitution of "GVIM for "" under the assumption it does not exist?

If the tests are to be changed, I suggest using something other than an empty string and adding the substitution to Vim documentation.

—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/issues/9723/1037257870@github.com>

[Yegappan Lakshmanan]

Hi,

On Sat, Feb 12, 2022 at 7:29 AM Michael Soyka <vim-dev...@256bit.org> wrote:

I agree that it is undesirable to reduce test coverage. However, if the purpose of the tests is to verify failure to send because a server does not exist, why not use some unlikely name for the server as is done in Test_remote_foreground():

assert_fails('remote_foreground("NonExistingServer")', 'E241:')
assert_fails('remote_foreground("")', 'E241:')

The first call makes the purpose of the test clear. As a side note, isn't the second call redundant or is its purpose to test the substitution of "GVIM for "" under the assumption it does not exist?

The purpose of the second test is to verify the behavior of remote_forground() with an empty

string for the server name (as you described). This test was added as part of checking the

behavior of built-in functions with an empty string as an argument (8.2.3459).

- Yegappan

[vim-dev ML]

Hi,

On Sat, Feb 12, 2022 at 7:29 AM Michael Soyka ***@***.***>

—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.Message ID: <vim/vim/issues/9723/1037261220@github.com>

[mss...@gmail.com]

On Saturday, February 12, 2022 at 10:39:30 AM UTC-5 yega...@gmail.com wrote:

Hi,

On Sat, Feb 12, 2022 at 7:29 AM Michael Soyka <vim-dev...@256bit.org> wrote:

I agree that it is undesirable to reduce test coverage. However, if the purpose of the tests is to verify failure to send because a server does not exist, why not use some unlikely name for the server as is done in Test_remote_foreground():

assert_fails('remote_foreground("NonExistingServer")', 'E241:')
assert_fails('remote_foreground("")', 'E241:')

The first call makes the purpose of the test clear. As a side note, isn't the second call redundant or is its purpose to test the substitution of "GVIM for "" under the assumption it does not exist?

The purpose of the second test is to verify the behavior of remote_forground() with an empty

string for the server name (as you described). This test was added as part of checking the

behavior of built-in functions with an empty string as an argument (8.2.3459).

Got it, thanks for the explanation.  However, it still remains, I think, that the behavior you're testing is undocumented., yes?

- Yegappan

If the tests are to be changed, I suggest using something other than an empty string and adding the substitution to Vim documentation.

-mike

[Yegappan Lakshmanan]

On Sat, Feb 12, 2022 at 7:57 AM mss...@gmail.com <mss...@gmail.com> wrote:
>
>
>
> On Saturday, February 12, 2022 at 10:39:30 AM UTC-5 yega...@gmail.com wrote:
>>
>> Hi,
>>
>> On Sat, Feb 12, 2022 at 7:29 AM Michael Soyka <vim-dev...@256bit.org> wrote:
>>>
>>> I agree that it is undesirable to reduce test coverage. However, if the purpose of the tests is to verify failure to send because a server does not exist, why not use some unlikely name for the server as is done in Test_remote_foreground():
>>>
>>> assert_fails('remote_foreground("NonExistingServer")', 'E241:')
>>> assert_fails('remote_foreground("")', 'E241:')
>>>
>>> The first call makes the purpose of the test clear. As a side note, isn't the second call redundant or is its purpose to test the substitution of "GVIM for "" under the assumption it does not exist?
>>
>>
>> The purpose of the second test is to verify the behavior of remote_forground() with an empty
>> string for the server name (as you described). This test was added as part of checking the
>> behavior of built-in functions with an empty string as an argument (8.2.3459).
>
>
> Got it, thanks for the explanation. However, it still remains, I think, that the behavior you're testing is undocumented., yes?
>

Yes. I can't find a description of this in the Vim help files (if an
empty string is passed to the remote_xxx() functions,
then the name of the remote server is "GVIM"). Can you create a patch?

Thanks,

[mss...@gmail.com]

On Saturday, February 12, 2022 at 11:01:37 AM UTC-5 yega...@gmail.com wrote:

On Sat, Feb 12, 2022 at 7:57 AM mss...@gmail.com <mss...@gmail.com> wrote:
>
>
>
> On Saturday, February 12, 2022 at 10:39:30 AM UTC-5 yega...@gmail.com wrote:
>>
>> Hi,
>>
>> On Sat, Feb 12, 2022 at 7:29 AM Michael Soyka <vim-dev...@256bit.org> wrote:
>>>
>>> I agree that it is undesirable to reduce test coverage. However, if the purpose of the tests is to verify failure to send because a server does not exist, why not use some unlikely name for the server as is done in Test_remote_foreground():
>>>
>>> assert_fails('remote_foreground("NonExistingServer")', 'E241:')
>>> assert_fails('remote_foreground("")', 'E241:')
>>>
>>> The first call makes the purpose of the test clear. As a side note, isn't the second call redundant or is its purpose to test the substitution of "GVIM for "" under the assumption it does not exist?
>>
>>
>> The purpose of the second test is to verify the behavior of remote_forground() with an empty
>> string for the server name (as you described). This test was added as part of checking the
>> behavior of built-in functions with an empty string as an argument (8.2.3459).
>
>
> Got it, thanks for the explanation. However, it still remains, I think, that the behavior you're testing is undocumented., yes?
>

Yes. I can't find a description of this in the Vim help files (if an
empty string is passed to the remote_xxx() functions,
then the name of t 

he remote server is "GVIM"). Can you create a patch?

 

Yes I can.

[Bram Moolenaar]

> Got it, thanks for the explanation. However, it still remains, I think,
> that the behavior you're testing is undocumented., yes?

I thought it was mentioned somewhere, but I can't find it now. I'll add
a note at each of the functions that has the {server} argument.

--
hundred-and-one symptoms of being an internet addict:
30. Even though you died last week, you've managed to retain OPS on your
favorite IRC channel.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Christian Brabandt]

workaround has been mentioned. Let's close it.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you commented.Message ID: <vim/vim/issues/9723/1140205110@github.com>

[Christian Brabandt]

Closed #9723 as completed.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you commented.Message ID: <vim/vim/issue/9723/issue_event/6696691164@github.com>