- 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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
Enables integration testing and adds dynamic CLI/utility loading support.
- Adds a placeholder extension test to validate the test pipeline.
- Implements dynamic ESM imports for
pretty-bytesinstorage.tsandremote.ts. - Updates
package.jsonand introduces.vscode-test.mjsto configure and run integration tests.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/extension.test.ts | Introduces a dummy test suite to verify the test harness |
| src/storage.ts | Adds dynamic import ofpretty-bytes infetchBinary |
| src/remote.ts | Applies dynamic import ofpretty-bytes and makesupdateStatus async |
| package.json | Addstest:integration, pretest hooks, and new test CLI deps |
| .vscode-test.mjs | Configures VSCode Test CLI for integration test discovery |
Comments suppressed due to low confidence (3)
src/test/extension.test.ts:3
- [nitpick] The suite name 'first test' is generic; consider renaming it to reflect its purpose (e.g., 'Extension activation suite').
suite("first test", () => {src/storage.ts:127
- The dynamic import logic in
fetchBinaryisn't covered by existing tests; consider adding unit tests to verify thatpretty-bytesis loaded correctly and used as expected.
// Load ESM module if not already loadedsrc/remote.ts:846
- The new async
updateStatusfunction with dynamic import isn't covered by tests; consider adding integration tests to validate its behavior under different network inputs.
const updateStatus = async (network: {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.
- 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!
| ); | ||
| throw new Error("Remote SSH extension not found"); | ||
| } | ||
| // In test mode, use regular vscode API |
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.
Nice! I wonder if we can avoid the special testing behavior. Maybe we should always try to activate, but if we are unable to get this special API we skip the remote registration (and possibly log a warning)? Similar to what you already have except we always do it, not just in testing.
That would make it nicer for use cases where you have not installed the remote extension yet, for whatever reason, while reducing drift between production and testing.
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 suspect this is the kind of behavior we want, so 👍 to logging a warning and handling it gracefully
| import prettyBytes from "pretty-bytes"; | ||
| // Dynamic import for ESM module | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| let prettyBytes: any; |
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 know you said this was temporary, just leaving a note here to acknowledge we need to update this and other references before merge. 😄
| import * as assert from "assert"; | ||
| import * as vscode from "vscode"; | ||
| suite("Extension Test Suite", () => { |
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.
Exciting to have this working!!
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.
NB the
pretty_bytesnonsense is a temporary workaround, I won't merge this until I've fixed the actual usage, but I wanted to get a demonstration test going ASAP since right now I can happily make totally breaking changes 🤦