-
Notifications
You must be signed in to change notification settings - Fork 100
test: LSP server verso tests #699
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
base: main
Are you sure you want to change the base?
Conversation
|
Summary of PR review discussion with David:
|
I think this one isn't really necessary for a merge here. It's important, but so is avoiding bitrot and getting value from the test that's already here. One or two more should suffice to validate the approach. |
Upstream is happy with this so I'll proceed with that plan. |
9e3f79d to
70158ef
Compare
This avoids spurious infoview display of the internal elaborated constant type. This bug is only observable after applying the fix in leanprover#700 ; will wait for leanprover#700 and leanprover#699 to merge this so we can properly provide a test. Co-authored-by: David Thrane Christiansen <[email protected]>
0cb4ad2 to
6bdf8a5
Compare
Points addressed, except for "test all Verso LSP extensions". I added a custom pass on Verso test runner to run the tests, would be nice to have a fancier test runner at some point so we can run the tests in parallel for example. |
This avoids spurious infoview display of the internal elaborated constant type. This bug is only observable after applying the fix in leanprover#700 ; will wait for leanprover#700 and leanprover#699 to merge this so we can properly provide a test. Co-authored-by: David Thrane Christiansen <[email protected]>
src/tests/TestMain.lean
Outdated
| IO.println "Running interactive (LSP) tests..." | ||
| IO.println s!"current dir: {(← IO.Process.getCurrentDir)}" | ||
| -- We use the lower-level Process.spawn not to buffer the test output | ||
| -- Is this OK on windows ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we set up a Windows runner so you can be sure? No core Verso developer runs Windows, and all sorts of things are different over there; it'd be nice to know that we can build and test everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to setup a windows runner, I think it is a key platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #724 for a (low-prio) attempt to add Windows CI; I propose to keep things as they are until the Windows env to run CI is settled down.
|
Thanks for the reviews @david-christiansen , I've incorporated your feedback. Open points:
|
We add LSP tests for Verso documents; we reuse the core LSP testing infrastructure from lean4 upstream. See `doc/dev/testing.md` there for more details. Co-authored-by: David Thrane Christiansen <[email protected]>
|
My opinions:
No, we should save that for user-visible things.
Keep 'em, and fix them in that PR :)
Yes |
|
Thanks, we should then be ready to go. |
We add LSP tests for Verso documents; we reuse the core LSP testing infrastructure from lean4 upstream. See
doc/dev/testing.mdthere for more details.This is a draft PR, some comments: