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.

Sign up for GitHub

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

Jump to bottom

fix!: use name option to support multiple lsp servers in one test #37

Merged
merged 3 commits into from
Aug 6, 2023
Merged

fix!: use name option to support multiple lsp servers in one test #37

merged 3 commits into from
Aug 6, 2023

Conversation

sineed
Copy link
Contributor

@sineed sineed commented Jul 14, 2023

closes #29

It should now support having multiple LSP servers started in one test and also to be isolated in async test files.

mhanberg
mhanberg requested changes Jul 14, 2023
View reviewed changes
Copy link
Collaborator

@mhanberg mhanberg left a comment

Choose a reason for hiding this comment

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

You should remove this line: https://github.com/elixir-tools/gen_lsp/blob/main/lib/gen_lsp.ex#L123

lib/gen_lsp/buffer.ex
@@ -9,10 +9,15 @@ defmodule GenLSP.Buffer do
@options_schema NimbleOptions.new!(
communication: [
type: :mod_arg,
default: {GenLSP.Communication.Stdio, []},
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted here and removed in GenLSP

lib/gen_lsp/buffer.ex Outdated
Comment on lines 31 to 32
init_opts = NimbleOptions.validate!(opts, @options_schema)
GenServer.start_link(__MODULE__, init_opts, name: Keyword.get(opts, :name, __MODULE__))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
init_opts = NimbleOptions.validate!(opts, @options_schema)
GenServer.start_link(__MODULE__, init_opts, name: Keyword.get(opts, :name, __MODULE__))
opts = NimbleOptions.validate!(opts, @options_schema)
GenServer.start_link(__MODULE__, opts, Keyword.take(opts, [:name]))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied

lib/gen_lsp/test.ex Outdated
Comment on lines 33 to 54
buffer =
start_supervised!({GenLSP.Buffer, communication: {GenLSP.Communication.TCP, [port: 0]}})
buffer_id = String.to_atom("#{opts[:name]}buffer")
lsp_id = String.to_atom("#{opts[:name]}lsp")

buffer = start_supervised!(
Supervisor.child_spec(
{
GenLSP.Buffer,
communication: {GenLSP.Communication.TCP, [port: 0]},
name: buffer_id
},
id: buffer_id
)
)

{:ok, port} = :inet.port(GenLSP.Buffer.comm_state(buffer).lsocket)

lsp = start_supervised!({mod, Keyword.merge([buffer: buffer], opts)})
lsp = start_supervised!(
Supervisor.child_spec(
{mod, Keyword.merge([buffer: buffer], opts)},
id: lsp_id
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think any of this needs to change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually not as without specifying id I get the following failure:

  1) test can start multiple servers in one test/2 (GenLSPTest)
     test/gen_lsp_test.exs:150
     ** (RuntimeError) failed to start child with the spec {GenLSP.Buffer, [communication: {GenLSP.Communication.TCP, [port: 0]}, name: :example_server_2buffer]}.
     Reason: bad child specification, more than one child specification has the id: GenLSP.Buffer.
     If using maps as child specifications, make sure the :id keys are unique.
     If using a module or {module, arg} as child, use Supervisor.child_spec/2 to change the :id, for example:

         children = [
           Supervisor.child_spec({MyWorker, arg}, id: :my_worker_1),
           Supervisor.child_spec({MyWorker, arg}, id: :my_worker_2)
         ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or for the lsp without id:

  1) test can start multiple servers in one test/2 (GenLSPTest)
     test/gen_lsp_test.exs:150
     ** (RuntimeError) failed to start child with the spec {GenLSPTest.ExampleServer, [buffer: #PID<0.288.0>, name: :example_server_2]}.
     Reason: bad child specification, more than one child specification has the id: GenLSPTest.ExampleServer.
     If using maps as child specifications, make sure the :id keys are unique.
     If using a module or {module, arg} as child, use Supervisor.child_spec/2 to change the :id, for example:

         children = [
           Supervisor.child_spec({MyWorker, arg}, id: :my_worker_1),
           Supervisor.child_spec({MyWorker, arg}, id: :my_worker_2)
         ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can see in the NextLS PR that I had to specify this id in most of processes to make it work

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is only happening because you are starting them under the same test supervisor, i don't think you'll ever be starting two LSPs in one test.

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 actually did it in the new test for NextLS 🙉

test/gen_lsp_test.exs Outdated
Comment on lines 149 to 155

test "can start multiple servers in one test/2" do
assert %{} = server(GenLSPTest.ExampleServer, name: :example_server_2)

assert_raise RuntimeError, ~r/more than one child specification has the id: :example_server_2buffer/, fn ->
server(GenLSPTest.ExampleServer, name: :example_server_2)
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test is testing that you can't open 2 with the same name, not that it can start multiple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it already started a server in the setup

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, missed that, i don't think you need the assert raise assertion

i would probably assert on if the server is alive then with the alive? function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

@sineed sineed mentioned this pull request Jul 14, 2023
Closed
@sineed sineed force-pushed the unique-lsp-test-server branch from 71a8d47 to 66bde89 Compare July 14, 2023 15:09
@mhanberg mhanberg force-pushed the unique-lsp-test-server branch from 66bde89 to b9cff29 Compare August 6, 2023 01:51
@mhanberg mhanberg merged commit 8f586dc into elixir-tools:main Aug 6, 2023
@github-actions github-actions bot mentioned this pull request Aug 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhanberg mhanberg mhanberg requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GenLSP.Test.server/2 cannot be isolated properly
2 participants
@sineed @mhanberg