Skip to content

Add tests and development environment - #7

Merged
katauber merged 11 commits into
mainfrom
4-addEnvironments
Jun 18, 2026
Merged

Add tests and development environment#7
katauber merged 11 commits into
mainfrom
4-addEnvironments

Conversation

@katauber

Copy link
Copy Markdown
Member

Add tests and a development environment with hot reload.
Resolves #3

@katauber
katauber requested a review from fsteeg June 17, 2026 11:44

@fsteeg fsteeg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great, hot reloading and testing works.

Some details / remarks:

  • I've tweaked the README to make the testing steps more explicit in 79bd7e9.
  • I've inlined some variables and extracted a getCompletionItems method in the tests in 8c11178.
  • To me it feels like the tests have a lot of redundancy in their description strings. For instance, I think I'd skip the DisplayName annotations, IMO seeing the method names is fine, and avoids having to keep them in sync. I feel similar about the traditional-style assertions, e.g. assertNotNull & "should return a non-null". I think assertj-style assertions avoid a lot of that redundancy and read better, see e.g. in lobid-gnd-ui.
  • Should we perhaps link to the playground CONTRIBUTING.md instead of duplicating it here?

@katauber

Copy link
Copy Markdown
Member Author

Great, hot reloading and testing works.

Some details / remarks:

  • I've tweaked the README to make the testing steps more explicit in 79bd7e9.
  • I've inlined some variables and extracted a getCompletionItems method in the tests in 8c11178.

Thank you

  • To me it feels like the tests have a lot of redundancy in their description strings. For instance, I think I'd skip the DisplayName annotations, IMO seeing the method names is fine, and avoids having to keep them in sync. I feel similar about the traditional-style assertions, e.g. assertNotNull & "should return a non-null". I think assertj-style assertions avoid a lot of that redundancy and read better, see e.g. in lobid-gnd-ui.

Looking at the code this may have redundancies but what I like about the display names is how it looks when running gradlew test:
image
instead of
image

The strings in the assert parts are not really necessary. I will remove that.

  • Should we perhaps link to the playground CONTRIBUTING.md instead of duplicating it here?

I'm not sure about that. On one hand there sure will be changes in the CONtributing.md that apply to both repositories. On the other hand I like that e.g. links for creating a new issue fit the corresponding repository (https://github.com/metafacture/metafacture-lsp/issues/new). I also made some changes like
Describe the exact steps which reproduce the problem in as many details as possible. For example, start by explaining which client you used to connect to a language server. When listing steps, don't just say what you did, but explain how you did it. For example, if you send a request, explain what your call looks like and how you send it to the server. because the example of the playground didn' fit here.

@fsteeg

fsteeg commented Jun 18, 2026

Copy link
Copy Markdown
Member

+1

@fsteeg fsteeg assigned katauber and unassigned fsteeg Jun 18, 2026
@katauber
katauber merged commit 8e8575f into main Jun 18, 2026
1 check passed
@katauber
katauber deleted the 4-addEnvironments branch June 18, 2026 14:28
@katauber

Copy link
Copy Markdown
Member Author

Deployed, call websocket via wss://test.metafacture.org/ls

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.

Add test and developement environment

2 participants