-
Notifications
You must be signed in to change notification settings - Fork 41
Add speedtest command #864
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,13 +21,17 @@ import { toError } from "./error/errorUtils"; | |
| import { featureSetForVersion } from "./featureSet"; | ||
| import { type Logger } from "./logging/logger"; | ||
| import { type LoginCoordinator } from "./login/loginCoordinator"; | ||
| import { withProgress } from "./progress"; | ||
| import { withCancellableProgress, withProgress } from "./progress"; | ||
| import { maybeAskAgent, maybeAskUrl } from "./promptUtils"; | ||
| import { | ||
| RECOMMENDED_SSH_SETTINGS, | ||
| applySettingOverrides, | ||
| } from "./remote/sshOverrides"; | ||
| import { getGlobalFlags, resolveCliAuth } from "./settings/cli"; | ||
| import { | ||
| getGlobalFlags, | ||
| getGlobalShellFlags, | ||
| resolveCliAuth, | ||
| } from "./settings/cli"; | ||
| import { escapeCommandArg, toRemoteAuthority, toSafeHost } from "./util"; | ||
| import { vscodeProposed } from "./vscodeProposed"; | ||
| import { | ||
|
|
@@ -162,6 +166,73 @@ export class Commands { | |
| this.logger.debug("Login complete to deployment:", url); | ||
| } | ||
|
|
||
| /** | ||
| * Run a speed test against the currently connected workspace and display the | ||
| * results in a new editor document. | ||
| */ | ||
| public async speedTest(): Promise<void> { | ||
| const workspace = this.workspace; | ||
| if (!workspace) { | ||
| vscode.window.showInformationMessage("No workspace connected."); | ||
| return; | ||
| } | ||
|
|
||
| const duration = await vscode.window.showInputBox({ | ||
| title: "Speed Test Duration", | ||
| prompt: "Duration for the speed test (e.g., 5s, 10s, 1m)", | ||
| value: "5s", | ||
| validateInput: (v) => { | ||
| return /^\d+[smh]$/.test(v.trim()) | ||
| ? null | ||
| : "Enter a duration like 5s, 10s, or 1m"; | ||
|
Comment on lines
+185
to
+187
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue here is that this will always be incomplete, this basically accepts ANY Go duration string like |
||
| }, | ||
| }); | ||
| if (duration === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| const result = await withCancellableProgress( | ||
| async ({ signal }) => { | ||
| const baseUrl = this.requireExtensionBaseUrl(); | ||
| const safeHost = toSafeHost(baseUrl); | ||
| const binary = await this.cliManager.fetchBinary(this.extensionClient); | ||
|
Comment on lines
+196
to
+198
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should use the |
||
| const version = semver.parse(await cliUtils.version(binary)); | ||
| const featureSet = featureSetForVersion(version); | ||
| const configDir = this.pathResolver.getGlobalConfigDir(safeHost); | ||
| const configs = vscode.workspace.getConfiguration(); | ||
| const auth = resolveCliAuth(configs, featureSet, baseUrl, configDir); | ||
yazan-abu-obaideh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const globalFlags = getGlobalFlags(configs, auth); | ||
| const workspaceName = createWorkspaceIdentifier(workspace); | ||
|
|
||
| return cliUtils.speedtest(binary, globalFlags, workspaceName, { | ||
| signal, | ||
| duration: duration.trim(), | ||
| }); | ||
| }, | ||
| { | ||
| location: vscode.ProgressLocation.Notification, | ||
| title: `Running ${duration.trim()} speed test...`, | ||
| cancellable: true, | ||
| }, | ||
| ); | ||
|
|
||
| if (!result.ok) { | ||
| if (!result.cancelled) { | ||
|
Comment on lines
+218
to
+220
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: flatten this out like: |
||
| this.logger.error("Speed test failed", result.error); | ||
| vscode.window.showErrorMessage( | ||
| `Speed test failed: ${result.error instanceof Error ? result.error.message : String(result.error)}`, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have |
||
| ); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const doc = await vscode.workspace.openTextDocument({ | ||
| content: result.value, | ||
| language: "json", | ||
| }); | ||
| vscode.window.showTextDocument(doc); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's await this one for consistency actually |
||
| } | ||
|
|
||
| /** | ||
| * View the logs for the currently connected workspace. | ||
| */ | ||
|
|
@@ -505,7 +576,7 @@ export class Commands { | |
| const configDir = this.pathResolver.getGlobalConfigDir(safeHost); | ||
| const configs = vscode.workspace.getConfiguration(); | ||
| const auth = resolveCliAuth(configs, featureSet, baseUrl, configDir); | ||
| const globalFlags = getGlobalFlags(configs, auth); | ||
| const globalFlags = getGlobalShellFlags(configs, auth); | ||
| terminal.sendText( | ||
| `${escapeCommandArg(binary)} ${globalFlags.join(" ")} ssh ${app.workspace_name}`, | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,26 @@ export async function version(binPath: string): Promise<string> { | |
| return json.version; | ||
| } | ||
|
|
||
| /** | ||
| * Run a speed test against the specified workspace and return the raw output. | ||
| * Throw if unable to execute the binary. | ||
| */ | ||
| export async function speedtest( | ||
| binPath: string, | ||
| globalFlags: string[], | ||
| workspaceName: string, | ||
| options: { signal?: AbortSignal; duration?: string }, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love mixing two different kinds of options, one for args and one for the execution itself 🤔 Actually maybe inline this, it's very similar to the one in |
||
| ): Promise<string> { | ||
| const args = [...globalFlags, "speedtest", workspaceName, "--output", "json"]; | ||
| if (options.duration) { | ||
| args.push("-t", options.duration); | ||
| } | ||
| const result = await promisify(execFile)(binPath, args, { | ||
| signal: options.signal, | ||
| }); | ||
| return result.stdout; | ||
| } | ||
|
|
||
| export interface RemovalResult { | ||
| fileName: string; | ||
| error: unknown; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| /* eslint-env node */ | ||
| // Prints each argument on its own line, so tests can verify exact args. | ||
| process.argv.slice(2).forEach((arg) => console.log(arg)); |
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 already guard on this so I would throw here instead