Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade webdriverio to 6.1.20 #631

Merged

Conversation

alexandramedway
Copy link
Contributor

@alexandramedway alexandramedway commented Jun 23, 2020

What is this

WebdriverIO 5 was released in 2018:
https://webdriver.io/blog/2018/12/19/webdriverio-v5-released.html

WebdriverIO 6 was released in March 2020:
https://webdriver.io/blog/2020/03/26/webdriverio-v6-released.html

They have a lot of nice new features / better API, and it would be really nice if spectron supported this version! A few other things we would like to do with webdriverIO:

  1. Video reporting support: https://webdriver.io/docs/wdio-video-reporter.html
  2. Simplify API for reading elements from page / interacting with them
  3. ...and more! Please see the link above for more details on the changes

In this PR, we update webdriverio to be 6.1.20 and then fixed every issue that came up as a result of that!

Details

Breaking changes from webdriverIO 4 --> 5:
https://github.com/webdriverio/webdriverio/blob/v5/CHANGELOG.md

Breaking changes from webdriverIO 5 --> 6
https://github.com/webdriverio/webdriverio/blob/master/CHANGELOG.md

Test

  1. Tested with the unit tests that are already in spectron
  2. tslint & tsc
  3. travis

@christian-bromann
Copy link

So good to see Spectron updating WebdriverIO, thanks @alexandramedway. As an FYI: upgrading to v6 should not require much changes as we just dropped the Node v8 support as well as changed some command interfaces. If I can help with anything please let me know 😉

@alexandramedway
Copy link
Contributor Author

@christian-bromann Thank you so much for your help! I initially tried to update to v6 and ran into a few issues, so started with v5 - would love to get in touch so that I can continue on with v6 :) What is the best way to reach you?

@christian-bromann
Copy link

What is the best way to reach you?

I do WebdriverIO office hours every Wednesday if you want to jump on call directly. Otherwise just email me via christian[at]saucelabs.com

@alexandramedway
Copy link
Contributor Author

Just signed up - really appreciate you taking the time!

@codebytere
Copy link
Member

@alexandramedway hey thanks for this! I'll try to get it reviewed as soon as i can 🙇🏻‍♀️

@codebytere codebytere self-requested a review June 23, 2020 17:33
@alexandramedway
Copy link
Contributor Author

alexandramedway commented Jun 23, 2020

@codebytere thank you very much! I am struggling with a couple tests on CI that are not failing locally - would appreciate any advice into how to debug these!

edit: These are resolved now!

@alexandramedway alexandramedway changed the title Upgrade webdriverio to 5.23.0 Upgrade webdriverio to 6.1.20 Jun 24, 2020
@alexandramedway
Copy link
Contributor Author

Big thanks to @christian-bromann for your help getting from v5 to v6!!

Copy link

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a lot of opportunity in the code to optimise especially replacing all the .then calls with async/await. But that should be done in other PRs.

Without knowing the internals of the Spectron that much this looks good to me. I added some suggestions.

lib/api.js Show resolved Hide resolved
lib/api.js Show resolved Hide resolved
lib/api.js Show resolved Hide resolved
Comment on lines 48 to 71
return self
.exists()
.then(function () {
return self.startChromeDriver()
})
.then(function () {
return self.createClient()
})
.then(function () {
return self.api.initialize()
})
.then(function () {
return self.client.setTimeouts(
self.waitTimeout,
self.waitTimeout,
self.waitTimeout
)
})
.then(function () {
self.running = true
})
.then(function () {
return self
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of using self we could just use short functions:

Suggested change
return self
.exists()
.then(function () {
return self.startChromeDriver()
})
.then(function () {
return self.createClient()
})
.then(function () {
return self.api.initialize()
})
.then(function () {
return self.client.setTimeouts(
self.waitTimeout,
self.waitTimeout,
self.waitTimeout
)
})
.then(function () {
self.running = true
})
.then(function () {
return self
})
return this
.exists()
.then(() => this.startChromeDriver())
.then(() => this.createClient())
.then(() => this.api.initialize())
.then(() => this.client.setTimeouts(this.waitTimeout, this.waitTimeout, this.waitTimeout))
.then(() => {
this.running = true
})
.then(() => this)

package.json Outdated Show resolved Hide resolved
@christian-bromann
Copy link

Again, thank you @alexandramedway for pushing this forward 👏 !

Copy link

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thanks @alexandramedway for submitting this PR! Also thanks @christian-bromann for pitching in! Overall this looks good to me, I just have some nits that I commented on. In reviewing this PR, I realize that we should have js linting and prettier setup on this project and most of my nits would be resolved by that. I'm not asking that you add that to this PR, just a comment. Once we get this merged in, I'll followup with a PR to add linting and prettier.

lib/application.js Show resolved Hide resolved
lib/application.js Show resolved Hide resolved
it('resolves to an audit object with the results', async function () {
await app.client.waitUntilWindowLoaded()
await app.client.windowByIndex(0)
var audit = await app.client.auditAccessibility()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use const instead of var here

Suggested change
var audit = await app.client.auditAccessibility()
const audit = await app.client.auditAccessibility()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually need const as we reassign it below!

expect(audit.results).to.have.length(1)
it('ignores warnings when ignoreWarnings is specified', async function () {
await app.client.waitUntilWindowLoaded()
var audit = await app.client.auditAccessibility({ ignoreWarnings: true })

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use const instead of var here

Suggested change
var audit = await app.client.auditAccessibility({ ignoreWarnings: true })
const audit = await app.client.auditAccessibility({ ignoreWarnings: true })


it('ignores rules when ignoreRules is specified', async function () {
await app.client.waitUntilWindowLoaded()
var audit = await app.client.auditAccessibility({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use const instead of var here

Suggested change
var audit = await app.client.auditAccessibility({
const audit = await app.client.auditAccessibility({

test/example-test.js Show resolved Hide resolved
Comment on lines 32 to 33
var elem = await app.client.$('body')
var text = await elem.getText()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var elem = await app.client.$('body')
var text = await elem.getText()
const elem = await app.client.$('body')
const text = await elem.getText()

app.webContents.getTitle().should.eventually.equal('require name')
const emptyArgs = await app.electron.remote.process.execArgv()
const elem = await app.client.$('body')
var text = await elem.getText()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var text = await elem.getText()
const text = await elem.getText()

@@ -1,5 +1,6 @@
var helpers = require('./global-setup')
var path = require('path')
var expect = require('chai').expect

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var expect = require('chai').expect
const expect = require('chai').expect

test/web-view-test.js Outdated Show resolved Hide resolved
@alexandramedway
Copy link
Contributor Author

In reviewing this PR, I realize that we should have js linting and prettier setup on this project

I totally agree! My editor is actually set up to auto format which is why some changes slipped in which didn't actually update the code, only the formatting (tried to revert most of those though).

Would be awesome to go through this project, convert all var to const or let, etc (but like you mentioned, not in the scope of this PR as that would be huge and hard to review all the changes at once).

Looking forward to that update - will make future changes much easier!

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this! others already made most of my review points & this looks good now 🚀

@alexandramedway
Copy link
Contributor Author

alexandramedway commented Jun 26, 2020

Thank you for approving @codebytere - looks like I need 'write access' in order to merge this, would you be able to give it to me so that I can get these changes in? 😄 Or feel free to merge yourself!

@codebytere codebytere merged commit 9fdb353 into electron-userland:master Jun 29, 2020
@segersb
Copy link

segersb commented Jul 7, 2020

This is just what I needed! @codebytere do you know when master can be released?

@jkleinsc
Copy link

jkleinsc commented Jul 7, 2020

@segersb I just released v11.1.0 which has this update.

@vladimiry
Copy link

I just released v11.1.0 which has this update.

I wish that would be v12 / major release as in many cases the update will break things unless you updated the tests following the webdriver's breaking changes guide.

@jkleinsc
Copy link

jkleinsc commented Jul 8, 2020

@vladimiry I agree but Spectron has this weird version mapping to Electron: https://github.com/electron-userland/spectron#version-map. I guess we could have waited until Electron 10 to release this but I thought maybe folks would want to use this before then.

@vladimiry
Copy link

I thought maybe folks would want to use this before then.

Don't know about folks, but I do 👍 Big thanks to @alexandramedway.

facebook-github-bot referenced this pull request in facebook/flipper Mar 23, 2021
Summary:
allow-large-files
Bumps [spectron](https://github.com/electron/spectron) from 11.0.0 to 14.0.0.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/electron/spectron/releases">spectron's releases</a>.</em></p>
<blockquote>
<h2>v13.0.0</h2>
<p>No release notes provided.</p>
<h2>v12.0.0</h2>
<p>No release notes provided.</p>
<h2>v11.1.0</h2>
<p>Upgraded webdriverio to 6.1.20 <a href="https://github.com/electron/spectron/issues/631">https://github.com/facebook/flipper/issues/631</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li>See full diff in <a href="https://github.com/electron/spectron/commits">compare view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=spectron&package-manager=npm_and_yarn&previous-version=11.0.0&new-version=14.0.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

 ---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `dependabot rebase` will rebase this PR
- `dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `dependabot merge` will merge this PR after your CI passes on it
- `dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `dependabot cancel merge` will cancel a previously requested merge and block automerging
- `dependabot reopen` will reopen this PR if it is closed
- `dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

</details>

Pull Request resolved: #2071

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

yarn test-e2e

Test Suite Succeded after running yarn test-e2e

Reviewed By: passy

Differential Revision: D27230110

Pulled By: priteshrnandgaonkar

fbshipit-source-id: d6d2d6c1482fa563b9dde9472467f918496e2cea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants