Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Add debugger launch test #989

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Add debugger launch test #989

merged 1 commit into from
Jan 29, 2024

Conversation

vinistock
Copy link
Member

Motivation

We had a few debugger related issues recently, so we need to continue on our quest to improve our test coverage of the extension. This PR adds a launch test for the debugger that actually spawns it, so that we can catch more issues on CI.

Implementation

The test performs all of the necessary setup, like creating a Gemfile and activating Ruby. The it attempts to launch the debugger with a simple 1 + 1 Ruby program, waits for the debugger to finish and performs clean up.

I also bumped our Ruby version to 3.3.0.

@vinistock vinistock self-assigned this Jan 17, 2024
@vinistock vinistock requested a review from a team as a code owner January 17, 2024 15:06
}

// By default, VS Code always saves all open files when launching a debugging session. This is a problem for tests
// because it attempts to save an untitled test file and then we get stuck in the save file dialog with no way of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which file is the untitled one here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a new untitled file that gets created when we run tests.

.getConfiguration("debug")
.update("saveBeforeStart", currentSaveBeforeStart, true, true);
});
}).timeout(30000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a long test to read through, I wonder if we can extract out behaviour into helper methods?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely do it, but since it's not used anywhere else I'm now sure what to extract. Do you have a suggestion?

@vinistock vinistock force-pushed the vs/add_debugger_launch_test branch 8 times, most recently from 6a2a3ae to 48913b0 Compare January 19, 2024 14:39
@vinistock vinistock force-pushed the vs/add_debugger_launch_test branch 10 times, most recently from a1cc3c3 to aec5297 Compare January 26, 2024 22:28
@vinistock vinistock requested a review from andyw8 January 26, 2024 22:29
import { Workspace } from "./workspace";

class TerminalLogger {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This terminal logger is so that we can see logs from the debug server when a test fails

try {
sockets = execSync(cmd, {
cwd: workspaceFolder.uri.fsPath,
const result = await asyncExec("bundle exec rdbg --util=list-socks", {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just switching to asyncExec instead of synchronous.

@@ -192,16 +209,19 @@ export class Debugger
const configuration = session.configuration;
const workspaceFolder = configuration.targetFolder;
const cwd = workspaceFolder.path;
const port =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using UNIX sockets doesn't work on Windows, but we can easily spawn the debugger with a port.

@@ -253,7 +277,7 @@ export class Debugger
// If the Ruby debug exits with an exit code > 1, then an error might've occurred. The reason we don't use only
// code zero here is because debug actually exits with 1 if the user cancels the debug session, which is not
// actually an error
this.debugProcess.on("exit", (code) => {
this.debugProcess.on("close", (code) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exit event actually happens before the stdin, stdout and stderr pipes are closed. If we raise an error on exit, we never capture the backtrace of the server printed to stderr.

But if we use the close event, then everything gets printed and it's significantly easier to understand why the debug server failed to start.

Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to have a timeout for the test if there isn't any reasonable one.

src/debugger.ts Outdated Show resolved Hide resolved
@vinistock vinistock enabled auto-merge (squash) January 29, 2024 19:21
@vinistock vinistock merged commit 3b7befc into main Jan 29, 2024
10 checks passed
@vinistock vinistock deleted the vs/add_debugger_launch_test branch January 29, 2024 19:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants