search

percy / percy-capybara

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

Per-snapshot widths not respected in 5.0.0.pre.4 #169

Closed geoffharcourt closed 3 years ago

geoffharcourt commented 3 years ago

Hello Percy team,

We tried to upgrade to 5.0.0.pre.4 today, but all of our mobile-only snapshots were rendering with our default width. Here's our config file (I removed the Percy CSS since it's noisy and shouldn't be relevant):

---
version: 2

snapshot:
  widths:
    - 1370

discovery:
  network-idle-timeout: 250
  disable-cache: true

We added a logging statement to check our options before we took the snapshot (so one line above the snapshot call itself) in our CI process:

Options: {:name=>"mobile: Sign-in - en", :widths=>[411]}
--
[percy] Snapshot taken: mobile: Sign-in - en

All 20 of our mobile snapshots were taken at 1370px width instead of 411px despite this call (we have a helper method that sets up mobile options, so it's the same call everywhere). Can you help us figure out what's happening?

geoffharcourt commented 3 years ago

It looks like the widths are not being passed to @percy/core. When I ran with debugging:


Options: {:name=>"Mobile: Student Views Direct Instruction Lesson", :widths=>[411]}
--
  | [percy:core] --------- (30174ms)
  | [percy:core] Handling snapshot: (1ms)
  | [percy:core] -> name: Mobile: Student Views Direct Instruction Lesson (0ms)
  | [percy:core] -> url: http://127.0.0.1:35035/students/student_lessons/1 (0ms)
  | [percy:core] -> widths: 1370px (0ms)
  | [percy:core] -> minHeight: 1024px (0ms)
  | [percy:core] -> enableJavaScript: false (0ms)
  | [percy:core] -> discovery: {} (0ms)
  | [percy:core] -> clientInfo: percy-capybara/5.0.0.pre.4 (0ms)
  | [percy:core] -> environmentInfo: capybara/3.35.3 ruby/2.7.3 (0ms)
  | [percy:core] -> domSnapshot:
Robdel12 commented 3 years ago

🤦🏼‍♂️ Oh boy, thanks for raising this issue! When rewriting the SDK I totally forgot to actually pass the options to the running Percy server. #170 fixes it! Sorry about that

geoffharcourt commented 3 years ago

Thanks for the update!

geoffharcourt commented 3 years ago

@Robdel12 we tested with this and were able to successfully migrate. Thanks for resolving this issue!

Robdel12 commented 3 years ago

Excellent news! Super happy to hear that 😃