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

Add basic lua tests #7

Merged
merged 16 commits into from
Jul 25, 2024
Merged

Conversation

fidgetingbits
Copy link
Collaborator

This adds basic lua testing capabilities to fix cursorless-dev#2374, by using busted. Mostly followed along https://hiphish.github.io/blog/2024/01/29/testing-neovim-plugins-with-busted/

One thing I'm not sure about is that this will require busted and/or luarocks to be installed, so I don't know exactly where you want those to be listed as prerequisites and/or added to some developer setup script. I plan to use the flake.nix dev shell to get them for myself.

I plan to rewrite buffer_get_selection() entirely, but that doesn't need to be part of this PR. Having some existing tests for it lets me compare the new function behaves the same.

Something else I haven't done yet is adding a way to run it in CI, but that's partially because I'm not sure about the install part of luarocks, etc. I can do the CI part as part of this PR, or in a follow up.

Lastly I'll likely clean up the tests a little bit, as I can re-use some code in a few places, but figured I'd post this up anyway.

@fidgetingbits
Copy link
Collaborator Author

fidgetingbits commented Jun 19, 2024

Was just trying to run the lua tests I added via github workflow, and realized that the existing tests don't get far enough to run the ones I added:

 packages/cursorless-neovim build: > @cursorless/[email protected] populate-dist /home/runner/work/cursorless/cursorless/packages/cursorless-neovim
packages/cursorless-neovim build: > bash ./scripts/populate-dist.sh
packages/cursorless-neovim build: Populating dist directory...
packages/cursorless-neovim build: CURSORLESS_REPO_ROOT: /home/runner/work/cursorless/cursorless
packages/cursorless-neovim build: cp: cannot stat '/home/runner/work/cursorless/cursorless/cursorless.nvim/*': No such file or directory
packages/cursorless-neovim build:  ELIFECYCLE  Command failed with exit code 1.
packages/cursorless-neovim build: Failed

Will look into it, but if you have any ideas let me know.

@pokey
Copy link
Collaborator

pokey commented Jun 19, 2024

Yeah I broke the neovim CI a few days ago and haven't gotten around to fixing it yet sorry. I can have a look

.gitignore Outdated Show resolved Hide resolved
docs/contributing/cursorless-in-neovim.md Show resolved Hide resolved
cursorless.nvim/test/unit/cursorless_spec.lua Outdated Show resolved Hide resolved
cursorless.nvim/test/unit/cursorless_spec.lua Show resolved Hide resolved
@saidelike
Copy link
Owner

saidelike commented Jul 16, 2024

That's amazing. Added a few comments in the code.

One drawback of having the unit tests inside cursorless.nvim/ is that all these files will currently be copied when packaging the extension for production. So we could either move them elsewhere (if possible), or we will have to go back to copying only required files for production. cc @pokey

One thing I'm not sure about is that this will require busted and/or luarocks to be installed, so I don't know exactly where you want those to be listed as prerequisites and/or added to some developer setup script. I plan to use the flake.nix dev shell to get them for myself.

What about adding requirements in docs/contributing/cursorless-in-neovim.md inside the ### Running lua tests section?

Something else I haven't done yet is adding a way to run it in CI, but that's partially because I'm not sure about the install part of luarocks, etc. I can do the CI part as part of this PR, or in a follow up.

I have fixed the neovim tests to work on CI in the nvim-talon branch. Can you rebase to the latest version so we confirm the lua unit tests work on CI?

Lastly I'll likely clean up the tests a little bit, as I can re-use some code in a few places, but figured I'd post this up anyway.

What do you want to clean up?

@saidelike
Copy link
Owner

@fidgetingbits It might be worth rebasing it to nvim-talon too to confirm the tests pass

@saidelike
Copy link
Owner

I did the rebase as a warning.

@fidgetingbits I would be curious if you know how to fix the failing tests now?

@fidgetingbits
Copy link
Collaborator Author

Addressed most of the feedback, but didn't look into why the CI test is failing yet.

@fidgetingbits
Copy link
Collaborator Author

I fixed the CI. The issue was that the busted action I chose to use turned out to be auto-running, so would fail before it got to the commands I had to manually run. I spent quite awhile trying to make it work, using -C to force it into cursorless.nvim folder and whatnot, but still could never get it to work, so just decided to go the manual lua/luarocks/busted install route. The downside is that it makes the test.yml a bit busier now.

There are at least a few options for cleaning it up a bit, so I figured I'd ask you before doing it:

  1. Create a separate .yml file to break out these actions and then call it from test.yml
  2. Create a new shell script to run the installations (which would mean manually calling apt get, etc) and call it from test.yml
  3. Leave it as is

@saidelike
Copy link
Owner

I fixed the CI. The issue was that the busted action I chose to use turned out to be auto-running, so would fail before it got to the commands I had to manually run. I spent quite awhile trying to make it work, using -C to force it into cursorless.nvim folder and whatnot, but still could never get it to work, so just decided to go the manual lua/luarocks/busted install route. The downside is that it makes the test.yml a bit busier now.

Nice one :)

There are at least a few options for cleaning it up a bit, so I figured I'd ask you before doing it:

1. Create a separate .yml file to break out these actions and then call it from test.yml

2. Create a new shell script to run the installations (which would mean manually calling apt get, etc) and call it from test.yml

3. Leave it as is

I think leaving it as-is is good.

  1. test.yml is not that big atm imho
  2. that would make it more annoying as we would not rely on power of test.yml which simplify the amount of code we need to write

I am happy with the PR and it is close to be merged imho, only a few minor feedbacks and we are ready to go!

@saidelike
Copy link
Owner

@fidgetingbits Just to let you know I have fixed Cursorless tests in neovim again on CI so merging nvim-talon into this PR will fix the tests here too.

.vscode/tasks.json Outdated Show resolved Hide resolved
@saidelike
Copy link
Owner

@fidgetingbits Do you mind documenting the architecture of the lua tests in https://github.com/saidelike/cursorless/blob/nvim-talon/docs/contributing/architecture/tests.md too?

@fidgetingbits
Copy link
Collaborator Author

@fidgetingbits Do you mind documenting the architecture of the lua tests in https://github.com/saidelike/cursorless/blob/nvim-talon/docs/contributing/architecture/tests.md too?

I took a stab at it.

@fidgetingbits
Copy link
Collaborator Author

Tests are passing and is easier to read now (imo), so hopefully mostly good to go

@saidelike
Copy link
Owner

saidelike commented Jul 23, 2024

Great stuff. I've changed a bit the tests documentation based on my understanding, hopefully you agree with my changes 3bef660 feel free to review/tweak otherwise. cc @fidgetingbits

@fidgetingbits
Copy link
Collaborator Author

Updated to address some of the feedback. Still no launch.json, but tests output are improved:

❯ busted --run unit
ok 1 - cursorless.nvim tests window_get_visible_lines() Can read one visible line
ok 2 - cursorless.nvim tests window_get_visible_lines() Can read all lines visible on the window
ok 3 - cursorless.nvim tests select_range() Selects the specified range
ok 4 - cursorless.nvim tests buffer_get_selection() Can get the forward selection in a format expected by cursorless
ok 5 - cursorless.nvim tests buffer_get_selection() Can get the backward selection in a format expected by cursorless
1..5

I also added .busted as a lua file in settings.json as it was defaulting to javascript for whatever reason.

I didn't review the documentation changes yet.

Also fwiw regarding being unable to test, I suspect you could run busted from wsl on windows no problem, assuming you actually want to try it. I suppose we should file an issue so that we don't rely on the XDG environment to run busted under in the future, and that way those tests can be run on windows, mac, etc.

@fidgetingbits
Copy link
Collaborator Author

❯ busted --run unit
ok 1 - cursorless.nvim tests window_get_visible_lines() Can read one visible line
ok 2 - cursorless.nvim tests window_get_visible_lines() Can read all lines visible on the window
ok 3 - cursorless.nvim tests select_range() Selects the specified range
ok 4 - cursorless.nvim tests buffer_get_selection() Can get the forward selection in a format expected by cursorless
ok 5 - cursorless.nvim tests buffer_get_selection() Can get the backward selection in a format expected by cursorless
1..5

Now that I know how this output looks, I'll tweak it to be even more concise/clean, but didn't have time today.

@saidelike
Copy link
Owner

saidelike commented Jul 24, 2024

I didn't review the documentation changes yet.

You might want to merge main into nvim-talon before doing so as Pokey has modified its contents a bit. See cursorless-dev@ee52d25

@saidelike
Copy link
Owner

❯ busted --run unit
ok 1 - cursorless.nvim tests window_get_visible_lines() Can read one visible line
ok 2 - cursorless.nvim tests window_get_visible_lines() Can read all lines visible on the window
ok 3 - cursorless.nvim tests select_range() Selects the specified range
ok 4 - cursorless.nvim tests buffer_get_selection() Can get the forward selection in a format expected by cursorless
ok 5 - cursorless.nvim tests buffer_get_selection() Can get the backward selection in a format expected by cursorless
1..5

Now that I know how this output looks, I'll tweak it to be even more concise/clean, but didn't have time today.

Note sure how you can improve it but I like already way more how it is now, nice one :)

Is it possible to simulate a failure to see how it renders?

@fidgetingbits
Copy link
Collaborator Author

Is it possible to simulate a failure to see how it renders?

I had put it in a response here #7 (comment) already

@saidelike saidelike merged commit 4c0ea88 into saidelike:nvim-talon Jul 25, 2024
7 checks passed
@fidgetingbits fidgetingbits deleted the lua-tests branch July 26, 2024 01:18
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.

3 participants