unsure how to make the tests pass again after headless update

shortcuts commented 1 week ago

Contributing guidelines

[X] I have read CONTRIBUTING.md
[X] I have read CODE_OF_CONDUCT.md
[X] I have updated 'mini.nvim' to latest version

Module(s)

mini.test

Description

Hey, first of all, thanks for all the plugins

I've been (extensively?) using mini.test since the start of my plugin and recently took a 2 week break, I just came back, started to fix some issues and somehow all tests are red now -- after some investigation it seems like this breaking change landed in stable and totally impacted the way tests are executed, but I've not been able to determine how to make them pass again.

In the meantime of a potential solution, I've just pinned the mini.test deps to the commit before this release, so it's not blocking me

Neovim version

0.10.0, 0.9.5, 0.8.3, 0.7.2

Steps to reproduce

git clone git@github.com:shortcuts/no-neck-pain.nvim.git debug-no-neck-pain
cd debug-no-neck-pain
make deps-latest (sorry I'm doing "e2e" testing with other plugins so there's a few deps to fetch in order to assert there's no plugin clash)
make test-API
some tests fail

Expected behavior

A more detailed explanation of what to change to make the test runner work like before, or things to consider updating with this new addition

Actual behavior

The test runner fails without clear instruction of potential changes

echasnovski commented 1 week ago

Thanks for the issue!

Sorry to hear that the 'mini.test' change broke your test setup. As you can see from the 'mini.test' commit, I only had to change a single mock for tests to keep passing.

I'll take a look at your setup.

shortcuts commented 1 week ago

Thanks for the issue!

Sorry to hear that the 'mini.test' change broke your test setup. As you can see from the 'mini.test' commit, I only had to change a single mock for tests to keep passing.

I'll take a look at your setup.

No problem that's normal that your plugin evolves :)

I just had an idea -- since my plugin rely on screen width, the change of default columns might alter my assertions, I'll try to change that asap to see if it has an effect

shortcuts commented 1 week ago

calling child.set_size(500, 500) at the beginning of a test case or adding --cmd "set lines=500 columns=500" to my test command doesn't change the outcome, still failing

echasnovski commented 1 week ago

The issue seems to be that plugin code uses vim.api.nvim_list_uis() to get information about instance dimensions (here, here, and here). As tests now run in headless mode, it means that there are no UIs attached to them initially (that's basically what :h --headless says).

My personal recommendation would be to not use nvim_list_uis() if the only information you want is instance dimensions. Using vim.o.columns and vim.o.lines for width and height usually seems to be enough. Those work even in headless instance. I tried replacing those in code and it resulted in a several regular test failures. Those require some code base knowledge, so I stopped here.

Other approaches are to updates tests to either mock vim.api.nvim_list_uis() or use vim.api.nvim_ui_attach().

Closing as an intended behavior of 'mini.test'.

Notes:

The way I debugged this was to first notice that there is a visible delay in failing tests which usually indicate some kind of non-direct error shown in child process. Then I edited one of failing tests to include Helpers.expect.equality(child.cmd_capture('messages')) to see what messages were shown. It returned a attempt to index a nil value at 'no-neck-pain/state.lua:591' with additional traceback. This pointed directly at nvim_list_uis() not behaving as you'd expect.
Requiring plugin's module from running instance means that the module must be visible which is not a very clean setup, as ideally the only place plugin should be loaded is child process.

shortcuts commented 6 days ago

wow thanks for the thorough explanation @echasnovski, I'll try to rely on columns and lines instead!

Then I edited one of failing tests to include Helpers.expect.equality(child.cmd_capture('messages')) to see what messages were shown.

Not sure if possible but that could be a great feature to be able to expose this (with -v for example) in the failing case!

echasnovski commented 5 days ago

Not sure if possible but that could be a great feature to be able to expose this (with -v for example) in the failing case!

That would be a problem because test failure is indicated purely by the error during test case function execution. The fact that the error cause is usually some aspect of child process state not being as expected is a convention. This independence is intentional and makes this quality of life improvement a challenge.

echasnovski / mini.nvim