-
Notifications
You must be signed in to change notification settings - Fork 4
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
Dependency checks and environment setup assistance #55
Conversation
These changes are untested.
It was necessary to switch from Jest to Mocha + Chai because Jest does not expose a stable API for programmatic invocation. Programmatic invocation is necessary in order for the tests to have access to the 'vscode' module, a process that involves actually running the tests within a VS Code instance. There is a backdoor way to invoke Jest programmatically, but I failed to find a way to do that while preserving access to the 'vscode' module.
080e397
to
f65d74e
Compare
I think that either of these alternatives are better than requiring the user to press Enter. I would go for the option that is easiest to implement. The key is that a user must be 1) given sufficient information to do a manual install and 2) given the opportunity to back out of an automatic install.
I'm not sure what to do about this one. Perhaps we shouldn't worry about it considering that a "retry" can always be performed to get to messages that disappeared? |
The intent of this refactoring is to consolidate references to the various dependencies. Setting up a new dependency requires too much wiring.
ed8607c
to
97f312e
Compare
This adds detail to the button shown to the user, which should say something like "Install using Snap" instead of just "Install." This also unburdens the user from having to press Enter to run the suggested command.
97f312e
to
c1d1cfb
Compare
5faa62f
to
d807980
Compare
I still need to check in the GUI that this behaves as intended, but the tests pass, and I don't anticipate major changes in this PR before it gets merged. |
The script was originally removed because it was difficult to update dependencies without waiting for the LDS to build. Perhaps a better workaround is to use the --install-dependencies flag (although that does have known risks, since it also disables any install scripts that our dependencies may have).
This repository does not need the action, but lf-lang/release-tools does.
Another caveat: This PR introduces nondeterministic test failures like this:
These only occur on the MacOS runners, but they occur pretty often -- something like half the time. I think that these failures are bad, but acceptable. |
Would caching be a good strategy to mitigate the connection issues? |
These tests uncovered 3 (!!!) bugs (all fixed in this commit). Yikes.
39ad6ee
to
e204452
Compare
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.
I have no tested this locally yet, but this looks great! I left some minor comments.
After the latest bugfix, I just got this branch to pass a basic smoke test (by installing Rust from within the VS Code editor). I think that what we have now should be better than nothing. |
This branch adds checks for various dependencies, with corresponding messages and suggested install commands in the UI. The new behavior is described mostly declaratively in
check_dependencies.ts
.This feature has been exceedingly difficult to test. Tests exist, but coverage is incomplete. This is not good, but I think it is acceptable
because when the extension sends text to the VS Code terminal, it does not append a terminating newline. This means the user can review the suggested install/update command before pressing Enterif CI is supplemented with manual tests.