search

rubyforgood / human-essentials

Human Essentials is an inventory management system for diaper, incontinence, and period-supply banks. It supports them in distributing to partners, tracking inventory, and reporting stats and analytics.
https://humanessentials.app
MIT License
471 stars 498 forks source link

[BUG]: Address flakiness of a distribution_system_spec due to DistributionPdf failing to find a logo #3161

Closed edwinthinks closed 2 years ago

edwinthinks commented 2 years ago

Is there an existing issue for this?

Current Behavior

We randomly see this spec fail:

Failures:

  1) Distributions When creating a distribution from a donation when editing that distribution User creates a distribution from a donation then edits it
     Failure/Error: StringIO.open(organization.logo.download)

     ActiveStorage::FileNotFoundError:
       ActiveStorage::FileNotFoundError

     [Screenshot Image]: /home/runner/work/human-essentials/human-essentials/tmp/screenshots/failures_r_spec_example_groups_distributions_when_creating_a_distribution_from_a_donation_when_editing_that_distribution_user_creates_a_distribution_from_a_donation_then_edits_it_483.png

     # ./app/pdfs/distribution_pdf.rb:13:in `initialize'
     # ./app/mailers/distribution_mailer.rb:26:in `new'
     # ./app/mailers/distribution_mailer.rb:26:in `partner_mailer'
     # <internal:kernel>:90:in `tap'
     # ./app/jobs/partner_mailer_job.rb:6:in `perform'
     # ./app/controllers/distributions_controller.rb:205:in `send_notification'
     # ./app/controllers/distributions_controller.rb:136:in `update'
     # ------------------
     # --- Caused by: ---
     # Errno::ENOENT:
     #   No such file or directory @ rb_sysopen - /home/runner/work/human-essentials/human-essentials/tmp/storage/qy/8q/qy8qtup4tgfnfc7nljxlqlkp0j7j
     #   ./app/pdfs/distribution_pdf.rb:13:in `initialize'

  2) Distributions When creating a distribution from a donation when editing that distribution User creates duplicate line items
     Failure/Error: StringIO.open(organization.logo.download)

     ActiveStorage::FileNotFoundError:
       ActiveStorage::FileNotFoundError

     [Screenshot Image]: /home/runner/work/human-essentials/human-essentials/tmp/screenshots/failures_r_spec_example_groups_distributions_when_creating_a_distribution_from_a_donation_when_editing_that_distribution_user_creates_duplicate_line_items_7.png

     # ./app/pdfs/distribution_pdf.rb:13:in `initialize'
     # ./app/mailers/distribution_mailer.rb:26:in `new'
     # ./app/mailers/distribution_mailer.rb:26:in `partner_mailer'
     # <internal:kernel>:90:in `tap'
     # ./app/jobs/partner_mailer_job.rb:6:in `perform'
     # ./app/controllers/distributions_controller.rb:205:in `send_notification'
     # ./app/controllers/distributions_controller.rb:136:in `update'
     # ------------------
     # --- Caused by: ---
     # Errno::ENOENT:
     #   No such file or directory @ rb_sysopen - /home/runner/work/human-essentials/human-essentials/tmp/storage/qy/8q/qy8qtup4tgfnfc7nljxlqlkp0j7j
     #   ./app/pdfs/distribution_pdf.rb:13:in `initialize'

Finished in 2 minutes 12.6 seconds (files took 8.23 seconds to load)
118 examples, 2 failures, 1 pending

Failed examples:

rspec ./spec/system/distribution_system_spec.rb:[331](https://github.com/rubyforgood/human-essentials/actions/runs/3105923312/jobs/5032144011#step:7:332) # Distributions When creating a distribution from a donation when editing that distribution User creates a distribution from a donation then edits it
rspec ./spec/system/distribution_system_spec.rb:[354](https://github.com/rubyforgood/human-essentials/actions/runs/3105923312/jobs/5032144011#step:7:355) # Distributions When creating a distribution from a donation when editing that distribution User creates duplicate line items

Randomized with seed 59233

Stopped processing SimpleCov as a previous error not related to SimpleCov has been detected
Coverage report generated for RSpec to /home/runner/work/human-essentials/human-essentials/coverage. 2891 / 5149 LOC (56.15%) covered.
/opt/hostedtoolcache/Ruby/3.1.2/x64/bin/ruby -I/home/runner/work/human-essentials/human-essentials/vendor/bundle/ruby/3.1.0/gems/rspec-core-3.10.2/lib:/home/runner/work/human-essentials/human-essentials/vendor/bundle/ruby/3.1.0/gems/rspec-support-3.10.3/lib /home/runner/work/human-essentials/human-essentials/vendor/bundle/ruby/3.1.0/gems/rspec-core-3.10.2/exe/rspec  --default-path spec "spec/system/distribution_system_spec.rb" "spec/system/request_system_spec.rb" "spec/system/product_drive_system_spec.rb" "spec/system/vendor_system_spec.rb" "spec/system/donation_site_system_spec.rb" "spec/system/question_system_spec.rb" "spec/system/dashboard_system_spec.rb[1:2:1:1]" "spec/system/dashboard_system_spec.rb[1:2:4:1]" "spec/system/dashboard_system_spec.rb[1:2:6:7:1:1]" "spec/system/dashboard_system_spec.rb[1:2:4:10:1:1]" "spec/system/dashboard_system_spec.rb[1:2:5:5:1:2]" "spec/system/dashboard_system_spec.rb[1:2:5:8:1:2]" "spec/system/dashboard_system_spec.rb[1:2:5:3:1:3]" "spec/system/dashboard_system_spec.rb[1:2:3:4:1:1]" "spec/system/dashboard_system_spec.rb[1:2:5:7:1:3]" "spec/system/dashboard_system_spec.rb[1:2:6:3:1:1]" "spec/system/dashboard_system_spec.rb[1:2:5:2:1:2]" "spec/system/dashboard_system_spec.rb[1:2:3:1]" "spec/requests/transfers_requests_spec.rb" "spec/system/partners/helps_system_spec.rb" "spec/system/dashboard_system_spec.rb[1:2:5:11:1]" "spec/system/dashboard_system_spec.rb[1:2:6:1]" "spec/requests/requests_requests_spec.rb" "spec/requests/admin/account_requests_requests_spec.rb" "spec/requests/partners/user_requests_spec.rb" failed
Coverage report generated for RSpec to /home/runner/work/human-essentials/human-essentials/coverage. 28 / 8354 LOC (0.34%) covered.
Stopped processing SimpleCov as a previous error not related to SimpleCov has been detected
Error: Process completed with exit code 1.

Expected Behavior

The specs should always pass or always fail.

Steps To Reproduce

N/A

Environment

N/A

Criteria for Completion

Anything else?

There are some options that we could explore which would make this more resilent. Stubbing is an option that may better ensure that this doesn't occur randomly

Code of Conduct

armahillo commented 2 years ago

I'm unable to repro this, even with bundle exec rspec --bisect spec/system/distribution_system_spec.rb

Will attempt to review the examples manually and trace through where the errors might be occurring. @edwinthinks Is this limited to specific environments / situational?

edwinthinks commented 2 years ago

It isn't clear, @armahillo; I've only personally seen it occur in CI.

A side note to your comment, I'am wondering if we want to redesign the test to be not dependent on a file to make this more resilent?

armahillo commented 2 years ago

It isn't clear, @armahillo; I've only personally seen it occur in CI.

OK!

A side note to your comment, I'am wondering if we want to redesign the test to be not dependent on a file to make this more resilent?

Yeah that's what I was thinking too

edwinthinks commented 2 years ago

@armahillo I was thinking maybe as a way to side step this issue is to avoid the DistributionPDF class by using the invoice theme - https://adminlte.io/themes/v3/pages/examples/invoice.html that they can print. What do you think?

armahillo commented 2 years ago

Oh yeah I am always a fan of <link rel="stylesheet" media="print" type="text/css" src=".../path/to/print.css" />

I would consider this to be a non-trivial, but worthwhile conversion to explore.

One caveat I'd have is that the output becomes more client-dependent, and will require more extensive testing, particularly on non-desktop clients. But I don't think that should be an obstacle large enough to block this.

armahillo commented 2 years ago

OK I was able to repro (sort of) by extracting

logo_image = if @organization.logo.attached?
                   StringIO.open(@organization.logo.download)
                 else
                   Organization::DIAPER_APP_LOGO
                 end

to a private memoizing method and then raising the exception in it:

private
  def logo_image
    raise Errno::ENOENT
    @logo_image ||= if @organization.logo.attached?
                   StringIO.open(@organization.logo.download)
                 else
                   Organization::DIAPER_APP_LOGO
                 end
  end

It's a little louder than it is in CI, but it will work for at least determining that the stubbing I'm about to do addresses the issue.

These tests are ambivalent to the mailer async jobs, so I'm going to stub out those schedule methods. I think this points to a separate issue of responsibilities (like, should ~schedule_reminder_email~ send_notification be in the controller or should it be in the service object?) but those are out of scope for this issue.

Attempting to stub it out and seeing if that addresses the issue.

armahillo commented 2 years ago

After fixing these two, it's now failing for two different specs (on the :print method)