Add tests for launching/attaching to chrome #831

EricCornelson · 2019-05-03T15:31:19Z

Adding tests for launching chrome (relies on microsoft/vscode-chrome-debug-core#438)

digeff · 2019-05-03T16:36:46Z

test/int/adapter.test.ts

+            const DEBUGGER_LINE = 2;
+
+            server = createServer({ root: testProjectRoot });
+            server.listen(7890);


This code is similar to line 118 and to the should stop on debugger statement in http://localhost test
Is there any way we can avoid duplicating it?

Addressed. We set this up in the setup() for this suite now

digeff · 2019-05-03T16:41:36Z

test/int/adapter.test.ts

+
+            console.log('killing all instances of chrome');
+            // force kill chrome here, as it will be left open by the debug adapter (same behavior as v1)
+            const killCmd = (process.platform === 'win32') ? 'taskkill /F /IM chrome.exe /T' : 'killall chrome';


This code is similar to the code in testSetup.tearDown. Would it be possible to unify it?

Is this behavior undesirable? If so, we should add a TODO to fix it after we release v2.

It can make troubleshooting easier. If you start Chrome, fail to attach, then kill Chrome, sometimes people might think that Chrome never actually started.

I definitely think we need to look at changing the behavior for V2 after we get the initial release out, because what essentially happens here is that the debugger attaches properly but expects to find an "about:blank" page, which we launch to when we launch chrome. If a user already has an instance of chrome running with the debug flag, it won't have that about:blank page, and the debugger bails, even if it might have been able to recover.

For now I'll create a "kill all instance of chrome" function that will get rid of the duplicated code.

digeff · 2019-05-03T16:44:25Z

test/int/adapter.test.ts


 const DATA_ROOT = testSetup.DATA_ROOT;

-suite('Chrome Debug Adapter etc', function () {
+suite('Chrome Debug Adapter etc', () => {


Mocha suggests using normal functions instead of arrow functions, so we can access the this pointer if needed: https://mochajs.org/#arrow-functions

I think you can still do this with arrow functions: https://stackoverflow.com/questions/15971167/how-to-increase-timeout-for-a-single-test-case-in-mocha. I would vote for using arrow functions here.

Yeah, the reason I changed this to an arrow function was mainly just because of our lint rules that will flag any regular function declarations, and were red-lining the entire test in the editor. The other option is just to add tslint exceptions for all the tests, but that's also kind of annoying. There's a big long discussion about arrow functions vs normal here. There's been some chatter about redesigns in mocha to better support arrow functions, but so far it doesn't seem to have gone anywhere.

FWIW, Jasmine also recommends function over ()=>{}
https://stackoverflow.com/questions/38915090/are-arrow-functions-recommended-in-jasmine-test-cases.

Personally, I'd prefer to use arrow functions almost everywhere since it makes javascript behave more like C#, Java, and others in regards to this, and of course I'm more familiar with those languages, but it seems like the JS community (at least right now) still relies pretty heavily on the current this behavior.

So I can go either way on this.

Let's use the arrow function then. If we ever need to do something which the only way to do it is to get the this pointer, then we can change that particular call to use a function instead.

digeff · 2019-05-03T16:45:05Z

test/int/adapter.test.ts

+
+            server = createServer({ root: testProjectRoot });
+            server.listen(7890);
+            const browser = await puppeteer.launch(<puppeteer.LaunchOptions>{ headless: false, args: ['http://localhost:7890', '--remote-debugging-port=7777'] });


Why is this cast needed?

Not needed for anything except intellisense. For whatever reason, VS Code doesn't provide intellisense on anonymous objects, even when the type of the argument is known for that function unless you provide a type assertion. We can remove it if you'd prefer, I typically just toss them on there when I need to check what the possible values can be.

**EDIT: figured out the problem on my end with the intellisense, turns out it was an extension (TypeScript Importer) causing the problems just for me.

roblourens · 2019-05-07T22:55:51Z

src/launcherAndRuner/chromeLauncher.ts

@@ -30,7 +30,8 @@ class NotYetLaunched implements IChromeLauncherLifetimeState {
    }

    public stop(): Promise<void> {
-        throw new Error(`Can't stop the chrome process because it hasn't been launched yet`);
+        // no-op here because in the case that we attach to an existing instance the launcher will still be in this state
+        return new Promise(a => a());


Promise.resolve()

roblourens · 2019-05-07T23:01:13Z

test/int/adapter.test.ts

+
+            console.log('killing all instances of chrome');
+            // force kill chrome here, as it will be left open by the debug adapter (same behavior as v1)
+            const killCmd = (process.platform === 'win32') ? 'taskkill /F /IM chrome.exe /T' : 'killall chrome';


It can make troubleshooting easier. If you start Chrome, fail to attach, then kill Chrome, sometimes people might think that Chrome never actually started.

digeff · 2019-05-10T17:11:07Z

test/int/adapter.test.ts

+            try {
+                await Promise.all([
+                    dc.configurationSequence(),
+                    dc.launch({ url: 'http://localhost:7890', timeout: 2000, webRoot: testProjectRoot }),


I'd specify the port here as an explicit parameter, to make it clearer that we are going to be using the same port...

digeff · 2019-05-10T17:12:09Z

test/int/adapter.test.ts

+                    dc.launch({ url: 'http://localhost:7890', timeout: 2000, webRoot: testProjectRoot }),
+                ]);
+            } catch (err) {
+                expect(err.message).to.satisfy( (x: string) => x.startsWith('Cannot connect to runtime process, timeout after 2000 ms'));


Will this test fail if dc.launch somehow succeeds?

Yes. Right now since we're just baselining V1 behavior, we are expecting this to throw an exception. Once we decide it's an appropriate time to fix this, then we can change the test accordingly. I'll add a note in the test that explains this so that it's clear what we're expecting from the test

digeff · 2019-05-10T17:13:24Z

test/int/adapter.test.ts

+                dc.assertStoppedLocation('debugger_statement', { path: breakFile, line: DEBUGGER_LINE } )
+            ]);
+
+            await browser.close();


It might be a good idea to put the .close() in a finally block, so we'll close the browser even if the test fails...

Good suggestion. I added a finally block for both tests that close with puppeteer.

digeff · 2019-05-10T17:14:19Z

test/testUtils.ts

+ * Kills all running instances of chrome on the test host
+ */
+export function killAllChrome() {
+    const killCmd = (process.platform === 'win32') ? 'taskkill /F /IM chrome.exe /T' : 'killall chrome';


Will this kill all the chrome.exe instances on my desktop every time I run the tests?

Yes. Unless there's an easy way to get the PID of the instance that the debug adapter launched so we can just close that specific instance, I don't see another option other than just force killing all chrome instances here. Let me know if you know of a better way.

I tested on my PC and "wmic process where ExecutablePath='path goes here\chrome-win\chrome.exe' delete" can kill only the chrome.exe instances at a specific path

-Make debugging port explicit in tests -Put puppeteer browser.close() in finally blocks -Add context around the "should throw error when launching if chrome debug port is in use" test.

command. The kill command (or taskkill) will return with non-zero codes if the process is already dead. In which case, we don't care (we do the same thing elsewhere)

…we expect)

EricCornelson added 2 commits May 2, 2019 19:17

Make stop() in NotYetLaunched a no-op to deal with attach scenario

f7b396d

Adding tests around launching chrome

88dc3d1

vscodebot bot assigned roblourens May 3, 2019

digeff suggested changes May 3, 2019

View reviewed changes

roblourens approved these changes May 7, 2019

View reviewed changes

EricCornelson added 4 commits May 8, 2019 11:26

Consolidate chrome killing code

e01f9d8

Applying feedback and fixing lint issues

e5cf644

Moving userRequestedUrl from IDebugeeLauncher to configuration

58f0e06

Addressing feedback

8241fa7

digeff reviewed May 10, 2019

View reviewed changes

EricCornelson added 2 commits May 10, 2019 11:01

Addressing feedback,

d5bcd1c

-Make debugging port explicit in tests -Put puppeteer browser.close() in finally blocks -Add context around the "should throw error when launching if chrome debug port is in use" test.

Fix missing mock param in unit test, and ignore errors from kill chrome

96b2de0

command. The kill command (or taskkill) will return with non-zero codes if the process is already dead. In which case, we don't care (we do the same thing elsewhere)

EricCornelson force-pushed the v2 branch from be9cf7f to 96b2de0 Compare May 11, 2019 04:39

EricCornelson added 2 commits May 15, 2019 16:03

Force test to fail if chrome launching succeeds (since it's not what …

1e78576

…we expect)

Only kill test-launched instances of chrome (on windows)

b8408c0

digeff approved these changes May 15, 2019

View reviewed changes

roblourens merged commit 0123ff8 into microsoft:v2 May 16, 2019

sirinartk mentioned this pull request Nov 4, 2022

[Snyk] Fix for 1 vulnerabilities sirinartk/vscode-chrome-debug#50

Open

snyk-bot mentioned this pull request Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities sirinartk/vscode-chrome-debug#54

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for launching/attaching to chrome #831

Add tests for launching/attaching to chrome #831

EricCornelson commented May 3, 2019

digeff May 3, 2019

EricCornelson May 10, 2019

digeff May 3, 2019

roblourens May 7, 2019

EricCornelson May 8, 2019

digeff May 3, 2019

roblourens May 4, 2019

EricCornelson May 7, 2019

EricCornelson May 7, 2019

digeff May 7, 2019

digeff May 3, 2019

EricCornelson May 7, 2019 •

edited

Loading

roblourens May 7, 2019

roblourens May 7, 2019

digeff May 10, 2019

EricCornelson May 10, 2019

digeff May 10, 2019

EricCornelson May 10, 2019

digeff May 10, 2019

EricCornelson May 10, 2019

digeff May 10, 2019

EricCornelson May 10, 2019

digeff May 14, 2019

Add tests for launching/attaching to chrome #831

Add tests for launching/attaching to chrome #831

Conversation

EricCornelson commented May 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricCornelson May 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricCornelson May 7, 2019 •

edited

Loading