Skip to content
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

Tests: fix init.lua tests hanging #13744

Closed
wants to merge 1 commit into from
Closed

Tests: fix init.lua tests hanging #13744

wants to merge 1 commit into from

Conversation

mjlbach
Copy link
Contributor

@mjlbach mjlbach commented Jan 13, 2021

Without this workaround, the test suite hangs for me on macOS, linux, and NixOS.

@mjlbach mjlbach changed the title Tests: fix init.lua tests hanging [RDY] Tests: fix init.lua tests hanging Jan 13, 2021
@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 13, 2021

@dm1try do you know why this is necessary? These tests seem awfully slow.

@dm1try
Copy link
Contributor

dm1try commented Jan 13, 2021

@mjlbach I was not able to debug this properly. I remember the test was hanging and waiting for "any" input, so I left the comment.

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 13, 2021

Is there any harm in adding the workaround for these tests too?

@dm1try
Copy link
Contributor

dm1try commented Jan 13, 2021

Is there any harm in adding the workaround for these tests too?

nope (maybe a "mental" harm only for a code reader :))
though, let me re-check why this is needed in the first place.

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 13, 2021

What's super weird is this only started giving me issues (very) recently), and doesn't seem to affect CI.

@dm1try
Copy link
Contributor

dm1try commented Jan 13, 2021

What's super weird is this only started giving me issues (very) recently), and doesn't seem to affect CI.

yeah, I was not able to reproduce this on my side(osx) on current master

Screen Shot 2021-01-13 at 03 37 26

and I realized why the "workaround" was needed in the first place: the screen on test nvim istance is small enough so the message "Conflicting configs ..." takes multiple lines and shows "Press Enter or type any command.." at the end.

so this not a workaround anymore but an expected step :)

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 13, 2021

Now it all makes sense. Thanks :)

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 13, 2021

Maybe I should remove the "why is this needed" in the tests haha. I think it's noted somewhere you need CR for small multiline messages

@dm1try dm1try mentioned this pull request Jan 13, 2021
@dm1try
Copy link
Contributor

dm1try commented Jan 13, 2021

Maybe I should remove the "why is this needed" in the tests haha. I think it's noted somewhere you need CR for small multiline messages

oops, I've already opened a PR :)

@mjlbach
Copy link
Contributor Author

mjlbach commented Jan 13, 2021

I'm going to do it in this one anyways since we need to add the additional not to fail local tests

@@ -458,7 +458,7 @@ describe('user config init', function()

it('loads init.lua from XDG config home by default', function()
clear{ args_rm={'-u' }, env={ XDG_CONFIG_HOME=xconfig }}

feed('<cr>') -- confirm "Conflicting config ..." message
Copy link
Contributor

@dm1try dm1try Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, in this test, such message should not exist on clear setup(init.vim is not created in this context).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you run the tests? does test Xhome directory get removed between test runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran with make test. Let me see if adding this back causes the test to hang. When I tested before, I had to disable this test in order for the test suite to not hang.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remove that one, it hangs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unable to reproduce it, try to debug on your side.
for example, put some "test" expectation after feed

feed('<cr>') 
matches('check it!', meths.exec('messages', true))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if helpful, but this test was hanging for me as well. I tried the debugging suggestion and got the following output:

[ RUN      ] user config init loads init.lua from XDG config home by default: 1020.91 ms ERR
test/helpers.lua:100: Pattern does not match.
Pattern:
check it!
Actual:
E484: Can't open file $HOME/bin/share/nvim/syntax/syntax.vim

stack traceback:
        test/helpers.lua:100: in function 'matches'
        test/functional/core/startup_spec.lua:463: in function <test/functional/core/startup_spec.lua:459>

I was surprised to see it was referencing $HOME/bin. I had run make CMAKE_INSTALL_PREFIX=$HOME/bin/ to build but hadn't run make install yet. Running make install fixed the hanging test. Unsure if this is working as intended.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, there is certainly a problem with setup of testing environment(maybe XDG_CONFIG_HOME somehow leaked)
I'm going to take a look at this one more time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dm1try any news ? I am hit by this too. Error is triggered when neovim sees both an init.lua and init.vim but I haven't looked into why it finds both (leftover from previous state ? )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any news ?

@teto I checked the assumption above(about XDG_CONFIG_HOME) but didn't find anything suspicious.

Error is triggered when neovim sees both an init.lua and init.vim but I haven't looked into why it finds both (leftover from previous state ? )

if this is still reproducible , could you re-check if XDG_CONFIG_HOME points to "Xhome/Xconfig"
and is this directory removed on each test run(maybe there is some os-specific behavior and this setup does not work as expected)

also, maybe the tests are run in parallel somehow?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having some trouble reproducing the hangups

@mjlbach mjlbach changed the title [RDY] Tests: fix init.lua tests hanging Tests: fix init.lua tests hanging Jan 13, 2021
@mjlbach mjlbach marked this pull request as draft January 13, 2021 16:45
@teto teto added this to the 0.5 milestone Feb 18, 2021
@mjlbach
Copy link
Contributor Author

mjlbach commented Feb 18, 2021

@dm1try I did some additional digging. The issue seems to be the runtime path getting polluted with entries from $HOME, and missing syntax.vim (as also noted by @fkm3)

See with my standard neovim configuration
And where I deleted ~/.local/share/nvim

Note the runtime/packpath. Note, just deleting local packpath is not sufficient to prevent the test from hanging, I believe the hang is due to this error about missing syntax.vim in runtime path.

The following modification fixes the test without feeding <cr>, but these aren't the correct directories.

  it('loads init.lua from XDG config home by default', function()
    clear{args_rm={'-u' }, env={ XDG_CONFIG_HOME=xconfig ; XDG_DATA_HOME=xconfig; VIMRUNTIME=xconfig}}

    eq(1, eval('g:lua_rc'))
    eq(init_lua_path, eval('$MYVIMRC'))
  end)

@mjlbach
Copy link
Contributor Author

mjlbach commented Feb 18, 2021

Sidenote, I'm also mildly concerned about the complaints about the "Failed to start server: address in use" warning.

@@ -441,6 +441,7 @@ describe('user config init', function()

before_each(function()
rmdir(xhome)
rmdir(xconfig)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing xconfig isn't necessary to fix the tests, but this seems like something we should do for consistency? @teto

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? it's a nested directory.

@@ -472,7 +473,7 @@ describe('user config init', function()
end)

it('loads custom lua config and does not set $MYVIMRC', function()
clear{ args={'-u', custom_lua_path }, env={ XDG_CONFIG_HOME=xconfig }}
clear{ args={'-u', custom_lua_path }, env={ XDG_CONFIG_HOME=xconfig ; XDG_DATA_HOME=xconfig; VIMRUNTIME=xconfig}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's weird is XDG_DATA_HOME should be set on line 12 in cmake set(ENV{XDG_DATA_HOME} ${BUILD_DIR}/Xtest_xdg/share)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this points at INFO 2021-02-18T11:07:10.488 1012134 emsg_multiline:681: E484: Can't open file Xhome/Xconfig/syntax/syntax.vim

@mjlbach
Copy link
Contributor Author

mjlbach commented Feb 18, 2021

This actually seems to be an issue with not running installPhase. Using the nix flake

cmakeConfigurePhase
buildPhase
TEST_FILE="test/functional/core/startup_spec.lua" make functionaltest # hangs
installPhase
TEST_FILE="test/functional/core/startup_spec.lua" make functionaltest # works

@dm1try
Copy link
Contributor

dm1try commented Feb 19, 2021

@dm1try I did some additional digging. The issue seems to be the runtime path getting polluted with entries from $HOME, and missing syntax.vim (as also noted by @fkm3)

good investigation
not sure about the nix issue described above, but it looks like runtimepath should be properly stubbed in test environment for excluding side-effects provided by sharing those directories with other environments/instances.

@mjlbach
Copy link
Contributor Author

mjlbach commented Feb 19, 2021

Closing as we know what the issue is (seems like a nix thing)

@mjlbach mjlbach closed this Feb 19, 2021
@teto
Copy link
Member

teto commented Feb 19, 2021

I dont think it's a nix issue, the tests are looking into the install folder (I beleive CMAKE_INSTALL_PREFIX) but one should be able to run the tests without installation. We may need to better control the runtimepath in the tests.

@mjlbach
Copy link
Contributor Author

mjlbach commented Feb 19, 2021

Sorry, that was a bit dismissive on my part. I should have said, that the issue is exposed by nix, and we should probably address what you and I had discussed on IRC, but that isn't really about fixing the test, but fixing CMAKE_INSTALL_PREFIX so we can run tests without installing the runtimepath files into build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants