test: rewrite inspector test helper #14460

eugeneo · 2017-07-24T21:56:52Z

Helper was rewritten to rely on promises instead of manually written
queue and callbacks. This simplifies the code and makes it easier to
maintain and extend.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test: 2 of the inspector tests were rewritten with a new framework.

This is a work in progress. I would be grateful for any feedback before I port other tests on this new helper API.

eugeneo · 2017-07-26T21:41:55Z

Newer helper uses port 0 by default. Also it should now be possible to run multiple children processes (no test uses this yet).

TimothyGu · 2017-07-27T06:56:19Z

Very nice! I've implemented something like this but for inspector.Session in my VM context PR so I've got a couple suggestions. I'm on my phone though so will post them later.

eugeneo · 2017-07-28T17:50:01Z

@TimothyGu I would really appreciate your suggestions.

I've been thinking about having a test framework that can run tests both through WebSockets and JS bindings to improve the coverage. But I am also working on moving WS codepath to use JS bindings to talk to inspector - that should make JS bindings tests less important.

TimothyGu · 2017-07-31T00:34:53Z

test/inspector/inspector-helper-new.js

+  return { length: bodyOffset + dataLen, message };
+}
+
+function promiseWithTimeout(promise, message, multiplicator) {


This could be added to test/common/index.js, or you can just take my version instead :) https://github.com/TimothyGu/node/blob/15a8ff8f52a8e01f41e773ea1f4e970976625aac/test/common/index.js#L820-L838 (docs)

I copied you version - but the issue I noticed is that the test will not complete until the timeout is fired. I implemented the timeout clearing.

TimothyGu · 2017-07-31T00:35:15Z

test/inspector/inspector-helper-new.js

+
+function checkHttpResponse(host, port, path) {
+  return new Promise(function(resolve, reject) {
+    const req = http.get({ host, port, path }, function(res) {


Arrow functions are preferred.

TimothyGu · 2017-07-31T00:35:53Z

test/inspector/inspector-helper-new.js

+
+class InspectorSession {
+  constructor(socket, instance) {
+    this.instance_ = instance;


In Node.js private properties are usually prefixed with _ not postfixed.

TimothyGu · 2017-07-31T00:41:13Z

test/inspector/inspector-helper-new.js

+
+  waitForNotification(methodOrPredicate, description) {
+    const desc = description || methodOrPredicate;
+    const message = `Timed out faiting for matching notification (${desc}))`;


TimothyGu · 2017-07-31T00:43:27Z

test/inspector/inspector-helper-new.js

+  }
+
+  matchesConsoleOutputNotification_(notification, type, values) {
+    if (!(values instanceof Array))


Array.isArray

TimothyGu · 2017-07-31T00:45:47Z

test/inspector/inspector-helper-new.js

+  }
+
+  kill() {
+    return this.enqueue_((callback) => {


Where is enqueue_ defined?

I deleted "kill" (for now). It was copied from the old test and "enqueu_" is a primary reason the tests have to be rewritten :)

TimothyGu · 2017-07-31T00:46:41Z

test/inspector/inspector-helper-new.js

+    });
+  }
+
+  static createChildProcess(inspectorFlags, scriptContents, scriptFile) {


Is this function intended to be public?

Made it a standalone non-exported function.

TimothyGu · 2017-07-31T00:54:13Z

test/inspector/test-inspector-break-when-eval.js

+runTests().catch((error) => {
+  console.error(error);
+  process.exit(1);
+});


You might want to use common.crashOnUnhandledRejection()

TimothyGu · 2017-07-31T00:57:44Z

test/inspector/inspector-helper-new.js

+      (message) => this.onStderrLine(message)));
+
+    this.shutdownPromise_ = new Promise((resolve) => {
+      this.process_.on('exit', (exitCode, signal) => {


I know it doesn't matter most of the cases, but I prefer using once instead of on with promises.

TimothyGu · 2017-07-31T00:59:12Z

test/inspector/inspector-helper-new.js

+      console.log('[sent]', JSON.stringify(msg));
+    const messageBuf = Buffer.from(JSON.stringify(msg));
+
+    const wsHeaderBuf = Buffer.allocUnsafe(16);


WS stuff should be factored out of this function (much like parseWSFrame above).

eugeneo · 2017-07-31T18:33:01Z

@TimothyGu Thank you for your review. Please take another look. I am more confident in this approach now so I will try to find time and port other tests now.

jasnell · 2017-07-31T19:35:58Z

test/common/index.js

@@ -816,3 +816,28 @@ exports.hijackStdout = hijackStdWritable.bind(null, 'stdout');
 exports.hijackStderr = hijackStdWritable.bind(null, 'stderr');
 exports.restoreStdout = restoreWritable.bind(null, 'stdout');
 exports.restoreStderr = restoreWritable.bind(null, 'stderr');
+
+exports.fires = function fires(promise, error = 'timeout', timeoutMs = 100) {


docs should be added to test/common/README.md

Done. I also ported a couple more test cases.

Note - I pushed a new change as a separate commit to make it easier to use the GitHub UI - I will do a proper rebase/squish before submitting the code.

refack · 2017-08-02T00:59:01Z

test/common/index.js

@@ -812,7 +812,49 @@ function restoreWritable(name) {
  delete process[name].writeTimes;
 }

+function onResolvedOrRejected(promise, callback) {


Can you call this finally as per https://github.com/tc39/proposal-promise-finally

finally is a reserved word. I considered promiseFinally or onFinally - but decided to be explicit...

One hackish way around that is to set the name property on the function after it is defined. E.g.

function onResolvedOrRejected(promise, callback) { /* ... */ } onResolvedOrRejected.name = 'finally';

But I definitely do not see that as being critical.

eugeneo · 2017-08-03T18:03:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9466/

I ported all the tests to a new framework. I also renamed test cases to remove "-inspector" from the file name - they all reside in the "inspector" folder so there is no need to repeat it in the file name.

TimothyGu

This needs a rebase...

TimothyGu · 2017-08-03T19:43:59Z

test/inspector/test-not-blocked-on-idle.js

  // 1 second wait to make sure the inferior began running the script
-  setTimeout(() => harness.runFrontendSession([shouldShutDown]).kill(), 1000);
+  console.log(1);


Debugging console.logs?

eugeneo · 2017-08-09T23:11:34Z

Performed a rebase. New CI: https://ci.nodejs.org/job/node-test-pull-request/9578/

eugeneo · 2017-08-10T16:19:10Z

Apparently, last CI run was cancelled. New CI: https://ci.nodejs.org/job/node-test-pull-request/9603/

eugeneo · 2017-08-10T23:36:52Z

CI did not show any relevant failures. This PR landed as https://github.com/eugeneo/node/commit/2296b677fb1e2ea71e86a899b028ba9e817e86ad.

This reverts commit 2296b67. That commit was landed without a green CI and is failing on Windows. Ref: nodejs#14460

addaleax · 2017-08-11T19:41:15Z

@eugeneo The last CI run linked here is for a different PR, this patch makes CI fail on Windows; revert is in #14777

This reverts commit 2296b67. That commit was landed without a green CI and is failing on Windows. Ref: #14460 PR-URL: #14777 Reviewed-By: Refael Ackermann <[email protected]>

nodejs-github-bot added dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Jul 24, 2017

TimothyGu self-assigned this Jul 31, 2017

TimothyGu reviewed Jul 31, 2017

View reviewed changes

jasnell reviewed Jul 31, 2017

View reviewed changes

refack reviewed Aug 2, 2017

View reviewed changes

jasnell approved these changes Aug 3, 2017

View reviewed changes

TimothyGu mentioned this pull request Aug 3, 2017

Report async tasks to V8 Inspector #13870

Closed

4 tasks

TimothyGu reviewed Aug 7, 2017

View reviewed changes

eugeneo closed this Aug 10, 2017

eugeneo deleted the promisify-test branch August 10, 2017 23:36

eugeneo merged commit 2296b67 into nodejs:master Aug 10, 2017

addaleax added a commit to addaleax/node that referenced this pull request Aug 11, 2017

Revert "inspector: rewrite inspector test helper"

42d8d32

This reverts commit 2296b67. That commit was landed without a green CI and is failing on Windows. Ref: nodejs#14460

addaleax mentioned this pull request Aug 11, 2017

Revert "inspector: rewrite inspector test helper" #14777

Closed

2 tasks

addaleax added dont-land-on-v6.x labels Aug 11, 2017

eugeneo mentioned this pull request Aug 13, 2017

inspector: rewrite inspector test helper #14797

Merged

3 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

test: rewrite inspector test helper #14460

test: rewrite inspector test helper #14460

Conversation

eugeneo commented Jul 24, 2017 • edited Loading

Checklist

Affected core subsystem(s)

eugeneo commented Jul 26, 2017

TimothyGu commented Jul 27, 2017

eugeneo commented Jul 28, 2017

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

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

eugeneo commented Jul 31, 2017

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

eugeneo commented Aug 3, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Aug 9, 2017

eugeneo commented Aug 10, 2017

eugeneo commented Aug 10, 2017

addaleax commented Aug 11, 2017

eugeneo commented Jul 24, 2017 •

edited

Loading