- Notifications
You must be signed in to change notification settings - Fork24
Add package scripts and cli library, enable integration testing#536
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 ourterms of service andprivacy 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
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
- Add test mode detection to bypass Remote SSH extension requirement- Skip remoteAuthority access in test mode to avoid API proposal errors- Update test expectations to match actual extension behavior- Configure vscode-test to enable proposed API for tests- Add proper command registration verification with timing delayThe extension now gracefully handles test environments where the RemoteSSH extension is not available, allowing integration tests to pass.🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>
…nto jaggederest/integration_tests
- Add vitest.config.ts with proper include/exclude patterns- Exclude src/test/** directory from unit tests (VS Code integration tests)- Exclude compiled out/** directory from test discovery- Update tsconfig.json to exclude vitest.config.ts from compilation- Add .eslintignore to skip linting vitest config- Update test script to use default Vitest behavior- Fix .vscodeignore formatting (add missing newline)🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>
Uh oh!
There was an error while loading.Please reload this page.
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.
Leaving off an approve until we sortprettyBytes but this is great!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…d ES6- Updated tsconfig.json to use CommonJS module system with proper ES module interop- Converted dynamic imports of pretty-bytes to standard ES6 imports in remote.ts and storage.ts- Added integration test command to CLAUDE.md documentation🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>
…ension and warning user
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.
Pull Request Overview
This PR adds package scripts and a CLI library while enabling integration testing for the extension.
- Introduces vitest configuration and integration tests to validate extension activation and command registration.
- Updates error handling in the extension activation flow and adjusts asynchronous handling in the remote module.
- Enhances package.json scripts and CI/CD configurations for Node 22 support.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adds testing configuration for the vitest environment. |
| src/test/extension.test.ts | Implements integration tests for verifying extension functionality. |
| src/remote.ts | Modifies async handling in the updateStatus function. |
| src/extension.ts | Adjusts activation flow and Remote SSH error handling behavior. |
| package.json | Adds new CLI/test scripts and updates dependency versions. |
| CLAUDE.md | Documents the new integration test command. |
| .vscode-test.mjs | Introduces VS Code test configuration via the test-cli package. |
| .github/workflows/*.yaml | Updates Node version from 18 to 22 in CI and release workflows. |
| .eslintignore | Excludes vitest.config.ts from linting. |
Comments suppressed due to low confidence (2)
src/extension.ts:34
- The removal of a thrown error for a missing Remote SSH extension changes the activation behavior. Ensure this fallback is intentional and well-documented so that downstream logic is not unexpectedly executed without the necessary extension.
vscode.window.showErrorMessage(.vscode-test.mjs:1
- [nitpick] Consider adding a comment to explain the purpose of this VS Code test configuration to improve maintainability.
import { defineConfig } from "@vscode/test-cli";| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| vscodeProposed = (module as any)._load( | ||
| "vscode", | ||
| { | ||
| filename: remoteSSHExtension.extensionPath, | ||
| }, | ||
| false, | ||
| ); |
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.
Using 'module as any' bypasses type safety; consider a more type-safe approach or adding documentation to justify this workaround.
Copilot uses AI. Check for mistakes.
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.
Yeah I get it buddy I don't like it either but we're polymorphic on the SSH extensions.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -1,14 +1,17 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "module":"commonjs", | |||
| "target":"es6", | |||
| "target":"ES2022", | |||
| "moduleResolution":"node", | |||
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 was reading thatnode was renamed tonode10 andnode is a deprecated alias now.
This is a change from the default, right? Supposedlynode10 is not meant to be used anymore but idk exactly what the consequences are.
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.
we should probably set this to"node16"
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.
moduleResolution has dependencies onmodule, in that it cannot be node16 without module also being node16 which would cause the priorpretty-bytes import issue. I think leaving it for now and fixing it in a future PR is probably fine, given that's the case. We could also kick thepretty-bytes lib to the curb as well if it's not playing nice.
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.
(and the default value when it's not set whenmodule is commonjs isnode10 which is the synonym)
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.
Ohhhhh I see, so this was the default already, not a change to the setting, just making it explicit. 👍 Something we can take a look at again in the future, perhaps.
Uh oh!
There was an error while loading.Please reload this page.
- Add parserOptions.project to enable type-aware linting- Disable @typescript-eslint/require-await rule for markdown files- Remove unnecessary async keywords from functions without await🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>
package.json Outdated
| "test:ci":"CI=true yarn test", | ||
| "test:integration":"vscode-test", | ||
| "pretest":"yarn run compile-tests && yarn run build && yarn run lint", | ||
| "compile-tests":"tsc -p . --outDir out" |
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.
even if we only need it for compiling tests, this doesn't just compile tests. bit of a weird script name.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -1,14 +1,17 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "module":"commonjs", | |||
| "target":"es6", | |||
| "target":"ES2022", | |||
| "moduleResolution":"node", | |||
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.
we should probably set this to"node16"
| "parser":"markdown-eslint-parser" | ||
| "parser":"markdown-eslint-parser", | ||
| "rules": { | ||
| "@typescript-eslint/require-await":"off" |
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.
why are we disabling this rule for markdown files? we shouldn't be linting those with eslint
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.
"lint": "eslint . --ext ts,md", is the current version, I'm happy to disable it, I was just trying not to change behavior :)
code-asherJun 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Ah yeah prettier is ran through eslint, which runs on markdown. It does seem weird to me as well, maybe they should be split.
Does this mean if we have any other rules we have to disable them here? I wonder if there is some configuration we are missing that is supposed to make this work more smoothly.
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.
Or, would it work better if we enabled the rule only in.ts files maybe?
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.
Yeah that makes sense, I'll swap that out since now I'm skipping it on md and json both, which is 🤦 , so I'll explicitly enable it on.ts files only instead. the joys of whitelist/blacklist
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! Good call on the async rule. If there is an easy way to configure it more cleanly that would be awesome, otherwise maybe we could opt to split linting and formatting at some point.
Uh oh!
There was an error while loading.Please reload this page.
A working vscode-test integration test that validates that the module will load and add commands, at least.