Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(7)

Issue 29959561: Issue 6930 - Add support for Microsoft Edge to WebDriver-based test automation

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months, 2 weeks ago by geo
Modified:
8 months, 2 weeks ago
Reviewers:
tlucas, Sebastian Noack
Visibility:
Public.

Description

Issue 6930 - Add support for Microsoft Edge to WebDriver-based test automation

Patch Set 1 #

Total comments: 14
Unified diffs Side-by-side diffs Delta from patch set Stats (+182 lines, -20 lines) Patch
M README.md View 2 chunks +6 lines, -2 lines 2 comments Download
M test/all.js View 3 chunks +24 lines, -1 line 4 comments Download
A test/browsers/edge.js View 1 chunk +113 lines, -0 lines 6 comments Download
M test/wrappers/pages.js View 8 chunks +39 lines, -17 lines 2 comments Download

Messages

Total messages: 2
geo
You'll see I've added several calls to `driver.sleep`. The reason behind those calls is that ...
9 months, 2 weeks ago (2018-12-05 13:35:28 UTC) #1
Sebastian Noack
9 months, 2 weeks ago (2018-12-06 03:47:32 UTC) #2
https://codereview.adblockplus.org/29959561/diff/29959562/README.md
File README.md (right):

https://codereview.adblockplus.org/29959561/diff/29959562/README.md#newcode108
README.md:108: Also you are going to have to install [Microsoft
Webdriver](https://developer.microsoft.com/en-us/microsoft-edge/tools/webdriver/)
This will be outdated very soon, as on Insider builds and future releases,
Microsoft WebDriver will be a Windows Feature on Demand.

https://codereview.adblockplus.org/29959561/diff/29959562/test/all.js
File test/all.js (right):

https://codereview.adblockplus.org/29959561/diff/29959562/test/all.js#newcode29
test/all.js:29: if (browser == "edge")
Nit: This can go at the very top of the function.

https://codereview.adblockplus.org/29959561/diff/29959562/test/all.js#newcode30
test/all.js:30: return [{version: "installed", getPath: () =>
Promise.resolve(null)}];
Nit: The version property only exists to distinguish the oldest and latest
version of Chromium and Firefox. I think we should just omit it here (since we
only run the tests on one version of Microsoft Edge, rather than having it
listed as "Edge (installed)" in the test output.

https://codereview.adblockplus.org/29959561/diff/29959562/test/all.js#newcode64
test/all.js:64: describe.skip : describe;
Is this sufficient? I suppose the before() hook would still run and fail for
Microsoft Edge on platforms other than Windows.

https://codereview.adblockplus.org/29959561/diff/29959562/test/all.js#newcode103
test/all.js:103: return this.driver.sleep(3000)
Is this really necessary? Or is there any promise we could wait for? If not,
retrying would be at least better than a fixed delay.

https://codereview.adblockplus.org/29959561/diff/29959562/test/browsers/edge.js
File test/browsers/edge.js (right):

https://codereview.adblockplus.org/29959561/diff/29959562/test/browsers/edge....
test/browsers/edge.js:31: "AppData", "Local", "Packages",
"Microsoft.MicrosoftEdge_8wekyb3d8bbwe",
Will this work universally? Microsoft.MicrosoftEdge_8wekyb3d8bbwe seems
environment specific?

https://codereview.adblockplus.org/29959561/diff/29959562/test/browsers/edge....
test/browsers/edge.js:58: `powershell Copy-Item ${buildDir} -Destination
${extDir} -Recurse`,
Calling into powershell to recursively copy a directory seems inappropriate. We
already require the "ncp" package which can do that.

https://codereview.adblockplus.org/29959561/diff/29959562/test/browsers/edge....
test/browsers/edge.js:75: fs.exists(extDir, exists =>
Could you use util.promisify() here?

https://codereview.adblockplus.org/29959561/diff/29959562/test/browsers/edge....
test/browsers/edge.js:92: .build();
Nit: It seems wrapping this line isn't necessary.

https://codereview.adblockplus.org/29959561/diff/29959562/test/browsers/edge....
test/browsers/edge.js:102: let driver = edge.Driver.createSession(capabilities,
service);
Nit: Just return the driver without creating temporary variable.

https://codereview.adblockplus.org/29959561/diff/29959562/test/browsers/edge....
test/browsers/edge.js:106: 
Nit: Redundant blank lines at end of file.

https://codereview.adblockplus.org/29959561/diff/29959562/test/wrappers/pages.js
File test/wrappers/pages.js (right):

https://codereview.adblockplus.org/29959561/diff/29959562/test/wrappers/pages...
test/wrappers/pages.js:102: this.driver.sleep(1000).then(() =>
Please retry rather than using delay:

  this.driver.wait(until.elementLocated(By.css(".site-pagelist a"), 1000)

https://codereview.adblockplus.org/29959561/diff/29959562/test/wrappers/pages...
test/wrappers/pages.js:242: this.driver.sleep(1500).then(() =>
No delays please, use this.driver.wait() as shown here:
https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/9#note_1...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5