feat(mobile): add extended wd commands for appium #3326

sjelin · 2016-07-01T20:08:52Z

No description provided.

sjelin · 2016-07-01T20:09:39Z

Had to add a lot of types to promises to get everything to compile

juliemr · 2016-07-06T19:33:49Z

Can we split this into two PRs, one to add the promise types and one to actually do the extend webdriver business?

sjelin · 2016-07-06T20:58:04Z

#3333

juliemr · 2016-07-20T20:50:42Z

I believe this is unblocked now, changing labels.

sjelin · 2016-08-02T11:34:07Z

Yeah, it was broken, not blocked. Should be fixed now. We'll see what travis says in the morning.

sjelin · 2016-08-02T11:34:48Z

But honestly it's probably gonna need some cleaning regardless

sjelin · 2016-08-08T10:44:07Z

Just gonna make sure I didn't screw up doc generation and then this PR will be good to go

sjelin · 2016-08-09T23:31:22Z

@juliemr ready for review

juliemr · 2016-08-21T17:00:04Z

lib/browser.ts

+    try {
+      extendWDInstance = extendWD(webdriverInstance);
+    } catch (e) {
+      // Must not have a driver that can be extended (e.g. directly provided)


What does 'directly provided' mean here? Can you add to the comment?

cnishina · 2016-12-06T17:26:07Z

I'll take a look at this today...

cnishina · 2016-12-06T19:30:58Z

lib/browser.ts

+   * @type {ExtendedWebDriver}
   */
-  driver: WebDriver;
+  driver: ExtendedWebDriver;


Looks like this is taken care of. This does not appear to have typing implications.

sjelin · 2016-12-07T22:28:57Z

@cnishina can I get a re-review?

sjelin · 2016-12-07T22:32:09Z

This is what the website looks like btw:

cnishina · 2016-12-08T01:14:33Z

lib/browser.ts

       opt_message?: string) => wdpromise.Promise<any>;
 }

+export class ExtendedWebdriver {


Nit on naming: We are importing ExtendedWebDriver interface and also have an ExtendedWebdriver class? By chance could this class be called something else? I almost thought these were the same thing because they differ by d vs D. Maybe WebDriverExtended?

Also, should this extend Webdriver above?

For consistency, I'll rename Webdriver to AbstractWebDriver and this to AbstractExtendedWebDriver

Yes

cnishina · 2016-12-08T01:14:58Z

lib/browser.ts

 * @alias browser
 * @constructor
- * @extends {webdriver.WebDriver}
+ * @extends {webdriver_extensions.ExtendedWebDriver}


this extends the class above and not really the webdriver_extensions

In order for js doc to work, this is the correct type (see old version pointing to webdriver.WebDriver not Webdriver)

I see. Alright, that makes sense.

cnishina · 2016-12-08T01:17:04Z

lib/browser.ts

    return new ProtractorBrowser(webdriver, baseUrl, rootElement, untrackOutstandingTimeouts);
  }
+
+  // Extended webdriver methods


Should we remove this comment since there are no methods?

sjelin

replied to comments, will implement in a second

sjelin · 2016-12-08T01:26:09Z

lib/browser.ts

 * @alias browser
 * @constructor
- * @extends {webdriver.WebDriver}
+ * @extends {webdriver_extensions.ExtendedWebDriver}


In order for js doc to work, this is the correct type (see old version pointing to webdriver.WebDriver not Webdriver)

sjelin · 2016-12-08T01:31:01Z

lib/browser.ts

    return new ProtractorBrowser(webdriver, baseUrl, rootElement, untrackOutstandingTimeouts);
  }
+
+  // Extended webdriver methods


sjelin · 2016-12-08T01:33:27Z

lib/browser.ts

       opt_message?: string) => wdpromise.Promise<any>;
 }

+export class ExtendedWebdriver {


For consistency, I'll rename Webdriver to AbstractWebDriver and this to AbstractExtendedWebDriver

Yes

cnishina · 2016-12-08T20:28:30Z

Please rebase with changes.

sjelin · 2016-12-13T10:08:35Z

All comments addressed

Had to make some minor changes to the website to handle longer inheritance chains

cnishina · 2016-12-21T18:58:27Z

lib/runner.ts

+    // have to be removed.
+    patch(
+        require('selenium-webdriver/lib/command'), require('selenium-webdriver/executors'),
+        require('selenium-webdriver/http'));


with the new types, could we import these with

import * as http from 'selenium-webdriver/http'; import * as executors from 'selenium-webdriver/executors';

?

sjelin · 2016-12-21T20:10:30Z

Closed in favor of #3860

sjelin added the pr: needs review label Jul 1, 2016

googlebot added the cla: yes label Jul 1, 2016

juliemr added the pr: needs cleanup label Jul 6, 2016

sjelin mentioned this pull request Jul 6, 2016

chore(docs): update docs to use webdriver-manager for mobile setup #3320

Merged

sjelin added the status: blocked label Jul 6, 2016

sjelin force-pushed the extend-wd branch from 4994770 to fc53bf1 Compare July 13, 2016 20:18

juliemr removed the status: blocked label Jul 20, 2016

sjelin force-pushed the extend-wd branch from fc53bf1 to 3e25791 Compare August 2, 2016 11:30

sjelin changed the title ~~feat(mobile): add extended wd commands for appium~~ (WIP) feat(mobile): add extended wd commands for appium Aug 2, 2016

juliemr removed the pr: needs review label Aug 3, 2016

sjelin force-pushed the extend-wd branch from 3e25791 to e9f718d Compare August 8, 2016 10:43

sjelin force-pushed the extend-wd branch from e9f718d to c4d790e Compare August 9, 2016 23:30

sjelin changed the title ~~(WIP) feat(mobile): add extended wd commands for appium~~ feat(mobile): add extended wd commands for appium Aug 9, 2016

sjelin added pr: needs review and removed pr: needs cleanup labels Aug 9, 2016

sjelin assigned juliemr Aug 12, 2016

juliemr reviewed Aug 21, 2016
View reviewed changes

juliemr added pr: needs cleanup and removed pr: needs review labels Aug 21, 2016

sjelin assigned cnishina Dec 6, 2016

cnishina unassigned sjelin Dec 6, 2016

cnishina approved these changes Dec 6, 2016

View reviewed changes

sjelin force-pushed the extend-wd branch 3 times, most recently from da69654 to 09c3ddc Compare December 7, 2016 22:27

cnishina suggested changes Dec 8, 2016

View reviewed changes

cnishina added pr: needs cleanup and removed pr: needs review labels Dec 8, 2016

cnishina assigned sjelin and unassigned cnishina Dec 8, 2016

sjelin commented Dec 8, 2016

View reviewed changes

sjelin force-pushed the extend-wd branch from 09c3ddc to 0f582cc Compare December 13, 2016 10:08

sjelin added pr: needs review and removed pr: needs cleanup labels Dec 13, 2016

sjelin force-pushed the extend-wd branch 2 times, most recently from 6499713 to 6a1f627 Compare December 15, 2016 23:27

sjelin mentioned this pull request Dec 21, 2016

Protractor doesn't support angular synchronization on iOS #2840

Open

sjelin force-pushed the extend-wd branch from 6a1f627 to 5a2add8 Compare December 21, 2016 18:28

feat(mobile): add extended wd commands for appium

47df870

Had to make some minor changes to the website to handle longer inheritance chains

sjelin force-pushed the extend-wd branch from 5a2add8 to 47df870 Compare December 21, 2016 18:34

cnishina reviewed Dec 21, 2016

View reviewed changes

sjelin closed this Dec 21, 2016

sjelin mentioned this pull request Dec 22, 2016

feat(mobile): add extended wd commands for appium #3860

Merged

feat(mobile): add extended wd commands for appium #3326

feat(mobile): add extended wd commands for appium #3326

Uh oh!

Conversation

sjelin commented Jul 1, 2016

Uh oh!

sjelin commented Jul 1, 2016

Uh oh!

juliemr commented Jul 6, 2016

Uh oh!

sjelin commented Jul 6, 2016

Uh oh!

juliemr commented Jul 20, 2016

Uh oh!

sjelin commented Aug 2, 2016

Uh oh!

sjelin commented Aug 2, 2016

Uh oh!

sjelin commented Aug 8, 2016

Uh oh!

sjelin commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnishina commented Dec 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjelin commented Dec 7, 2016

Uh oh!

sjelin commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjelin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnishina commented Dec 8, 2016

Uh oh!

sjelin commented Dec 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjelin commented Dec 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sjelin commented Dec 7, 2016 •

edited

Loading