-
Notifications
You must be signed in to change notification settings - Fork 42
Add tests and test support commands for visual editor #770
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
Conversation
f83426e
to
324935b
Compare
I'm pretty out of my depth here, but it looks like |
3cd85b2
to
324935b
Compare
I added a log in |
I agree entirely! I can dig into this a bit more later, but I am wondering if the extension is not getting activated with its LSP and similar. We might want to look at how the air extension runs tests to dig into this a bit. Here is there GH action (they bundle a binary in their extension FYI): https://github.com/posit-dev/air/blob/main/.github/workflows/test-code.yml And here is where they have some code they use for the extension to be activated and the LSP to be running: https://github.com/posit-dev/air/blob/main/editors/code/src/test/extension.ts |
I was able to verify that the extension's commands are not present at all. Here's how: I removed the
So there seems to be no Quarto commands available right now! I'm gonna check out what @juliasilge sent! That second link looks really promising for ensuring the extension is activated. |
8673c6c
to
dad1cac
Compare
Some progress. I tried to explicitly activate the extension in the tests and got this error in the logs:
|
b66135b
to
a881820
Compare
a881820
to
1a3b90f
Compare
Ok dang... I added a test workflow step to
edit: and if I remove calls to the new test commands, and add some long waits, I still get this error:
so |
3da3165
to
1a3b90f
Compare
More progress! I added a bunch of console.logs into
So... @cscheid I have narrowed down the issue with tests in the github workflow: |
Ah, of course! We should definitely install Quarto in this workflow! I definitely should have done that already 🙈 I think a good place to start is just the regular install that you can do via: |
b49ee49
to
cc87b25
Compare
Nice! That helps. More progress! I used the quarto action you linked to install quarto-cli in the test workflow and now we have a new error!
which may have to do with some other errors I have been seeing in the workflow logs? These logs don't show up locally for me, such as:
edit: I tried removing the explicit extension activation. Back to the original error about not recognizing quarto commands. edit 2: I tried adding additional logging next to the edit 3: I added some logging to various spots in |
8d45add
to
3ff85a5
Compare
3ff85a5
to
f8f92e7
Compare
@lionel- I am getting a bit stuck on debugging the quarto extension test github workflow. Do you have any advice or guidance? Do you know why our LSP server might not run in the workflow? |
@lionel- I notice that in air, you do this to wait for the LSP to be fully started up in tests: Could you say more about that in a testing context? And how we might apply that to the Quarto extension? |
6a3bcf1
to
030687a
Compare
030687a
to
3c79df0
Compare
Skipping the added in tests in the workflow/CI for now. This PR is ready for review and merge. We can come back to getting the extension, more specifically the LSP, working in CI later. Any advice on that is still appreciated! |
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.
Looks good to me!
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.
Looks great!
Quarto basics
✔ Can open a Quarto document (199ms)
- Can edit in visual mode
- Roundtrip doesn't change hello.qmd
1 passing (261ms)
2 pending
Let's see if we can continue to iterate on these tests so we can run them in CI.
This starts the LSP and only resolves when fully started (the heavy lifting is done by vscode-languageclient's However from the error messages above it looks like the LSP is getting started but fails to do so. |
980833f
to
ec41257
Compare
I tried one last thing this morning (and reverted it): doing |
This PR builds on-top of #757 to add tests, supporting functionality in test-utils, and registers visual editor commands specifically for tests. Two tests were added, they test:
Unfortunately these tests rely on
wait
ing an approximate amount of time for the visual editor to open and for other related commands to complete. They are slow and potentially flaky. I think this could be improved, but I'm not yet sure how. We may be able to return things from commands that help us understand if the visual editor has finished opening (maybe we will have to do some polling?). I wonder how other teams do this...The tests pass on my machine and its kind of fun to watch :)
edit: I just noticed the tests fail on this PR. It seems to be because the extension is not built before the tests are ran, resulting in the commands that were added in this PR not being present at the time of running tests. I attempted a fix (in now-reverted commits) that:
test-extension
job after thepackage-extension
job but that did not help;package-extension
job, but that did not help;