percy / percy-capybara

Visual testing for Capybara with Percy.
https://docs.percy.io/docs/capybara
MIT License
45 stars 23 forks source link

Issues with Apparition driver in 4.3.x #150

Closed geoffharcourt closed 4 years ago

geoffharcourt commented 4 years ago

We just did consecutive builds with 4.3.0 and 4.3.1. The 4.3.1 build had every CI job end with this output:


[percy] stopping percy...
--
  | [percy] waiting for 0 snapshots to complete...
  | [percy] done.
Robdel12 commented 4 years ago

Hmm, which driver are you using and which version is it? Setting LOG_LEVEL=debug will also put more info out.

Robdel12 commented 4 years ago

I'm curious to know more details. 🤔 It looks like the example app we have passes (as well as my local reproduction apps/scripts): https://github.com/percy/example-rails/pull/99

geoffharcourt commented 4 years ago

Ran another build with LOG_LEVEL true. Percy is there at start, but no snapshots get taken in any of our build jobs.


[percy] Current config file path: /app/.percy.yml
--
  | [percy] Using config: {
  | version: 1,
  | snapshot: {
  | 'percy-css': '#toast-container {\n' +
  | '  visibility: hidden;\n' +
  | '}\n' +
  | '.navbar-fixed-top {\n' +
  | '  position: absolute !important;\n' +
  | '}\n' +
  | '[data-test="written-response-counts-chart"] {\n' +
  | '  display: none;\n' +
  | '}\n',
  | widths: [ 1020, 1370 ]
  | },
  | agent: { 'asset-discovery': { 'network-idle-timeout': 5000 } }
  | }
  | [percy] created build #13044: https://percy.io/CommonLit/commonlit/builds/6724578
  | [percy] -> assetDiscoveryService.puppeteer.launch
  | [percy] -> assetDiscoveryService.createPagePool
  | [percy] -> assetDiscoveryService.setup
  | [percy] percy has started.
  | [percy] -> assetDiscoveryService.browser.newPage

And then our specs run, and at the end:


41 examples, 0 failures, 1 pending
--
  | [percy] stopping percy...
  | [percy] waiting for 0 snapshots to complete...
  | [percy] done.
  | [percy] finalized build #13044: https://percy.io/CommonLit/commonlit/builds/6724578
geoffharcourt commented 4 years ago

It appears that execute_script is not required to return a value: https://makandracards.com/makandra/12317-capybara-selenium-evaluate_script-might-freeze-your-browser-use-execute_script It looks like all the Poltergeist API descendant drivers (including Apparition, Cuprite) are going to have this problem. I think this includes all the newer CDP-driven Capybara drivers (vs. Selenium-driven ones).

Apparition returns nil from any use of execute_script. Not sure what other drivers do, but that would explain what's happening in the only change in 4.3.1, where the results of execute_script is expected to be the snapshot: https://github.com/percy/percy-capybara/commit/be5929103c1a8c5884d639a6cd7ec2573eb9759f

geoffharcourt commented 4 years ago

I thought I had posted our driver data yesterday, but we run Apparition (0.6.0) and Capybara (3.33.0).

Robdel12 commented 4 years ago

Thanks for that! It's unfortunate they're specifically returning nil: https://github.com/twalpole/apparition/blob/master/lib/capybara/apparition/driver.rb#L133 (looks like this is a specific design decision: https://github.com/twalpole/apparition/blob/ba431173024c56354a86ef8369c3493e8a1a39bb/lib/capybara/apparition/page.rb#L207-L214)

Seems to deviate from how all other drivers behave. I'll hack around with it and see what can be done. If it's simple enough to solve in the SDK I probably will. Another part of me wants to send a PR to fix that & stop it from returning nil.

geoffharcourt commented 4 years ago

@Robdel12 Apparition and Cuprite try to be as backward-compatible as possible with Poltergeist, so I think they are doing that to adhere to Poltergeist's API contract.

FWIW: Cuprite does the same thing intentionally: https://github.com/rubycdp/cuprite/blob/1f66a3d42a5694278db9b463d2d19c50ff53eff2/lib/capybara/cuprite/driver.rb#L105-L108