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

Eyes gem: Upgrade eyes_selenium gem to version 3.2.2 #14474

Merged
merged 4 commits into from Apr 25, 2017

Conversation

breville
Copy link
Member

As suggested by Applitools' support team.

(Gemfile.lock generated by running bundle install after making the change to Gemfile.)

As suggested by Applitools' support team.
@islemaster
Copy link
Contributor

Wow, no other API changes in a major version upgrade?

Copy link
Contributor

@aoby aoby left a comment

Choose a reason for hiding this comment

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

LGTM. We should babysit the next DTT to make sure this doesn't cause regressions.

@islemaster
Copy link
Contributor

Fun fact: This update includes my only Applitools bugfix which shipped in 2.39.0.

@islemaster
Copy link
Contributor

islemaster commented Apr 18, 2017

Sorry, I'm going to investigate further for possible API changes before I approve. It's not just a major version, this gem has changed GitHub repositories (before, after) since our current version - who knows what that means 🤷‍♂️.

@islemaster
Copy link
Contributor

Well, they definitely copied parts of the old repo over - here's the test I added, now attributed to an "initial import."

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Okay, I can't find anything in the documentation that suggests breaking changes and I'm not going to dig through everything in those two repos - running the tests is going to be our best validation. 👍

@islemaster
Copy link
Contributor

Hmm, this is the thing I most expect to break:

  batch = Applitools::Base::BatchInfo.new(ENV['BATCH_NAME'])
  batch.id = ENV['BATCH_ID']
  @eyes.batch = batch

The Applitools::Base namespace is definitely documented for v2.38.0 but not for v3.2.1. The docs on the Applitools wiki still refer to Applitools::Base::BatchInfo but they also still use Applitools::Eyes.new instead of Applitools::Selenium::Eyes.new, leading me to believe those docs assume a 2.x version - so it's possible the API for setting up a test batch has changed entirely. I'll open a question on their new repo about this.

@islemaster
Copy link
Contributor

Found it (while writing https://github.com/applitools/eyes.sdk.ruby/issues/28) - Applitools::Base::BatchInfo was moved to Applitools::BatchInfo. I'd update that in this PR too.

@davidsbailey
Copy link
Member

Nice! LGTM. Anything in particular we're getting from eyes_selenium 3.2.1 to look forward to?

@breville
Copy link
Member Author

@davidsbailey Nothing specific, but their support suggested we upgrade when we described the timeouts we saw in taking screenshots. If it doesn't solve this issue, then we can gather more logs and submit them.

Currently blocked on hearing from their support about an (as yet) undocumented API change.

@islemaster
Copy link
Contributor

Looks like batch support in the new library has been merged and is landing in v3.2.2.

@breville breville changed the title Eyes gem: Upgrade eyes_selenium gem to version 3.2.1 Eyes gem: Upgrade eyes_selenium gem to version 3.2.2 Apr 25, 2017
@breville breville merged commit bddb081 into staging Apr 25, 2017
@breville breville deleted the update-eyes-selenium-gem branch April 25, 2017 17:07
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.

None yet

4 participants