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 <david@lean-fro.org>
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 <david@lean-fro.org>
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.
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.
Happy to setup a windows runner, I think it is a key platform.
There was a problem hiding this comment.
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 <david@lean-fro.org>
|
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. |
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 <david@lean-fro.org>
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 <david@lean-fro.org>
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 <david@lean-fro.org>
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 <david@lean-fro.org>
…#707) This avoids spurious infoview display of the internal elaborated constant type. This bug is only observable after applying the fix in #700 ; will wait for #700 and #699 to merge this so we can properly provide a test. Co-authored-by: David Thrane Christiansen <david@lean-fro.org> --------- Co-authored-by: David Thrane Christiansen <david@lean-fro.org>
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: