Refactor element explorer to work with selenium-webdriver 3 #3828

juliemr · 2016-12-15T09:26:01Z

This implementation now relies mostly on promises explicitly,
so the control flow is only used to add one large task to the queue.
This should pave the way for the eventual removal of the control flow,
as well as getting element explorer to work immediately.

Note that there are several missing items and TODOs at the moment:

Needs rebasing on top of current beta branch
This is only for elementExplorer, I haven't yet fixed browser.pause()
There's some clean-up todos to make the breakpoint more robust
Need to ensure that the test suite for interactive tests is still working and can still break
Need to ensure that error cases (e.g. bad syntax in a statement) are handled gracefully

juliemr · 2016-12-16T21:38:32Z

We can greatly simplify our code by making a change to separate the 'step by execute' and 'enter repl' debuggers more clearly. This would mean that if you do browser.pause you can no longer enter a repl loop - but you can do that by instead placing browser.enterRepl in your code.

I'll go ahead and implement this to show how much easier it is to reason about. I've seen many users not know they can get into a repl from browser.pause anyway, so I think we can use this as an opportunity to improve clarity and docs.

juliemr · 2016-12-16T22:44:51Z

One more item to fix - the tab completion for the explorer is currently broken. EDIT: fixed in next commit.

juliemr · 2016-12-18T09:00:45Z

This is now ready for review!

I'm also considering aliasing browser.enterRepl() as browser.explore(). Thoughts?

heathkit

Awesome, this is so much better! Also, I vote yes on browser.explore()

heathkit · 2016-12-20T19:33:55Z

lib/breakpointhook.js

@@ -0,0 +1,3 @@
+module.exports = function() {


It'd be good to have a comment here explaining why this is awesome. Oh, and maybe it should go under debugger/

Done adding a comment. I don't think this belongs under debugger/ because all of the code in there runs in the separate debugger process, whereas this is part of the same process as browser + friends.

heathkit · 2016-12-20T19:35:53Z

lib/debugger.ts

   * @param {Function} onStartFn Function to call when the debugger starts. The
   *     function takes a single parameter, which represents whether this is the
   *     first time that the debugger is called.
-   * @param {number=} opt_debugPort Optional port to use for the debugging
+   * @param {number=} opt_debugPort Optional port to u`se for the debugging


heathkit · 2016-12-20T19:37:22Z

lib/browser.ts

@@ -910,7 +910,7 @@ export class ProtractorBrowser extends Webdriver {
      logger.info('  e.g., list(by.binding(\'\')) gets all bindings.');
      logger.info();
    };
-    this.debugHelper.init(debuggerClientPath, onStartFn, opt_debugPort);
+    this.debugHelper.init(debuggerClientPath, true, onStartFn, opt_debugPort);


Up to you, but it'd be nice if this were an enum, or at least had a comment explaining what this flag does.

Renamed into two different functions on the theory that the best documentation is good naming.

heathkit · 2016-12-20T19:40:28Z

lib/debugger.ts

    this.dbgCodeExecutor = {
-      execPromise_: pausePromise,     // Promise pointing to current stage of flow.
+      execPromise_: null,             // Promise pointing to currently executing command.


Why does this need to be null instead of undefined?

No reason, changed.

heathkit · 2016-12-20T19:44:11Z

lib/debugger/clients/wddebugger.js

  this.dbgRepl = new DebuggerRepl(this.client);
-  this.currentRepl = this.dbgRepl;

  // We want the prompt to show up only after the controlflow text prints.
  this.dbgRepl.printControlFlow_(function() {
    // Backward compatibility: node version 0.8.14 has a number of built in 


Can we just get rid of this node 0.8.14 stuff?

Yes, thanks for catching that.

heathkit · 2016-12-20T19:49:02Z

lib/debugger/modes/debuggerRepl.js

@@ -76,19 +84,22 @@ DebuggerRepl.prototype.stepEval = function(cmd, callback) {
 * @param {function} callback
 */
 DebuggerRepl.prototype.complete = function(line, callback) {
+  console.log('calling debuggerrepl complete');


Should these be here?

Nope, gone.

juliemr · 2016-12-22T23:30:22Z

Comments addressed, and rebased on top of beta. PTAL!

juliemr · 2016-12-28T17:56:46Z

Rebase successful! Just waiting for CI to be green now.

This implementation now relies mostly on promises explicitly, so the control flow is only used to add one large task to the queue. This should pave the way for the eventual removal of the control flow, as well as getting element explorer to work immediately. BREAKING CHANGE You can no longer use the `repl` command from within `browser.pause()`. Instead, use `broser.explore()` to directly enter the repl.

googlebot added the cla: yes label Dec 15, 2016

juliemr added the pr: experimental label Dec 15, 2016

juliemr mentioned this pull request Dec 15, 2016

WIP - start fixing the debugger w/ selenium 3.0 #3788

Closed

juliemr changed the title ~~WIP - Refactor element explorer to work with selenium-webdriver 3~~ Refactor element explorer to work with selenium-webdriver 3 Dec 18, 2016

juliemr added pr: needs review and removed pr: experimental labels Dec 18, 2016

juliemr force-pushed the beta branch from 43502e1 to ef2fe60 Compare December 19, 2016 21:14

heathkit self-requested a review December 20, 2016 19:33

heathkit suggested changes Dec 20, 2016

View reviewed changes

juliemr mentioned this pull request Dec 20, 2016

Tab key shows node commands instead of protractor commands #3594

Closed

juliemr force-pushed the beta branch 2 times, most recently from e3ee71c to 98b7f5e Compare December 21, 2016 18:10

juliemr mentioned this pull request Dec 21, 2016

Improve documentation for browser.enterRepl #2349

Closed

juliemr force-pushed the beta branch from e5c7cee to 0e0490c Compare December 22, 2016 21:24

juliemr force-pushed the beta-debugger-fix branch from bd394d6 to 71a8d94 Compare December 22, 2016 23:30

heathkit approved these changes Dec 23, 2016

View reviewed changes

juliemr force-pushed the beta branch from 0e0490c to 07e1915 Compare December 28, 2016 17:34

juliemr force-pushed the beta-debugger-fix branch from 71a8d94 to 2d23860 Compare December 28, 2016 17:48

googlebot added cla: no and removed cla: yes labels Dec 28, 2016

juliemr force-pushed the beta-debugger-fix branch from 2d23860 to 47590d8 Compare December 28, 2016 17:55

googlebot added cla: yes and removed cla: no labels Dec 28, 2016

sjelin force-pushed the beta branch from 07e1915 to 3644f01 Compare December 28, 2016 19:54

juliemr force-pushed the beta-debugger-fix branch from 47590d8 to bf92d02 Compare December 28, 2016 20:02

juliemr merged commit 530ca59 into angular:beta Dec 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor element explorer to work with selenium-webdriver 3 #3828

Refactor element explorer to work with selenium-webdriver 3 #3828

juliemr commented Dec 15, 2016 •

edited

Loading

juliemr commented Dec 16, 2016

juliemr commented Dec 16, 2016 •

edited

Loading

juliemr commented Dec 18, 2016

heathkit left a comment •

edited

Loading

heathkit Dec 20, 2016

juliemr Dec 22, 2016

heathkit Dec 20, 2016

juliemr Dec 22, 2016

heathkit Dec 20, 2016

juliemr Dec 22, 2016

heathkit Dec 20, 2016

juliemr Dec 22, 2016

heathkit Dec 20, 2016 •

edited

Loading

juliemr Dec 22, 2016

heathkit Dec 20, 2016

juliemr Dec 22, 2016

juliemr commented Dec 22, 2016

juliemr commented Dec 28, 2016

Refactor element explorer to work with selenium-webdriver 3 #3828

Refactor element explorer to work with selenium-webdriver 3 #3828

Conversation

juliemr commented Dec 15, 2016 • edited Loading

juliemr commented Dec 16, 2016

juliemr commented Dec 16, 2016 • edited Loading

juliemr commented Dec 18, 2016

heathkit left a comment • 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

heathkit Dec 20, 2016 • 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

juliemr commented Dec 22, 2016

juliemr commented Dec 28, 2016

juliemr commented Dec 15, 2016 •

edited

Loading

juliemr commented Dec 16, 2016 •

edited

Loading

heathkit left a comment •

edited

Loading

heathkit Dec 20, 2016 •

edited

Loading