Extract option logic from Grover class #46

willkoehler · 2020-02-23T03:56:59Z

@abrom I'm pushing up a WIP to get your feedback on the general shape of the solution. If it looks ok, I'll refine and add some specs.

More specifically

What do you think about deriving OptionsBuilder from Hash? Too "tricky"? I could also just return the options hash from an accessor method or something.
Also not sure on the best structure for OptionsFixer.new i.e. does OptionsFixer.new(options).run look like a reasonable API?
I'm also seeing several spec errors like expected: ["165", "42", "42"] got: [165, 42, 42] on my system (/spec/grover_spec.rb:424) which is making it hard to know if I broke anything. These specs are failing even on the Release v0.10.1 commit so I don't think they're caused by my changes. But I don't remember seeing these failures before on my system so that's kind of 🤔

abrom

I could go either way on extending Hash. Fine how it is though.
OptionsFixer.new...run looks fine. It's only an internal class so not too bothered
That's odd.. must be something different about the MiniMagick install on your machine?! I've tested it on Linux and Mac and not come across that before. In any case, easy fix will be to modify the mean_colour_statistics method ~L578 in the spec/grover_spec.rb file to something like:

  def mean_colour_statistics(image)
    %w[red green blue].map { |colour| image.data.dig('channelStatistics', colour, 'mean').to_s }
  end

(i.e. convert the mean colour coming from the image data hash into a string).

Weird!

lib/grover/options_builder.rb

lib/grover/options_fixer.rb

willkoehler · 2020-02-23T13:34:20Z

In any case, easy fix will be to modify the mean_colour_statistics method ~L578 in the spec/grover_spec.rb file to something like:
  def mean_colour_statistics(image)
    %w[red green blue].map { |colour| image.data.dig('channelStatistics', colour, 'mean').to_s }
  end

👍 I was thinking the same thing. Thanks for confirming. I'll add it to the PR.

...and Code Climate is at it again 😬 https://codeclimate.com/github/Studiosity/grover/pull/46 But I was actually thinking of making a helper for all the fix_xx_options! methods that took an array of options and a transformation method/block. Maybe I'll look into that and see if that makes Code Climate happy.

abrom · 2020-02-23T14:00:35Z

...and Code Climate is at it again 😬 https://codeclimate.com/github/Studiosity/grover/pull/46 But I was actually thinking of making a helper for all the fix_xx_options! methods that took an array of options and a transformation method/block. Maybe I'll look into that and see if that makes Code Climate happy.

I commented about that: #46 (comment)

willkoehler · 2020-02-23T16:13:40Z

That's odd.. must be something different about the MiniMagick install on your machine?!

It actually turned out to be a change in ImageMagick (I geeked out and dug through the source) ImageMagick/ImageMagick@bc978e0#diff-b911bfc2e4a717df4fc996ef7decb2ffL713-R716

This fixes expected: ["165", "42", "42"] got: [165, 42, 42] spec errors caused be a change in ImageMagick. See ImageMagick/ImageMagick@bc978e0#diff-b911bfc2e4a717df4fc996ef7decb2ffL713-R716

willkoehler · 2020-02-23T17:00:06Z

Edit: Nevermind. I get it now. It converts strings that contains arrays into arrays.
Ex "['--something']" --> ['--something']

Can you clarify what this does?

grover/lib/grover.rb

Lines 322 to 326 in d455f75

    
           def fix_array_options!(options) 
        
             return unless options['launch_args'].is_a? String 
        
             options['launch_args'] = YAML.safe_load options['launch_args'] 
        
           end

From what I can see options['launch_args'] is usually (always?) an array.

Example: ["--disable-speech-api"] in

grover/spec/grover_spec.rb

Line 275 in d455f75

let(:launch_args) { ['--disable-speech-api'] }

So return unless options['launch_args'].is_a? String will return early and this method doesn't do anything?

willkoehler · 2020-02-23T21:26:39Z

FYI this spec is failing on my machine

grover/spec/grover_spec.rb

Lines 115 to 119 in d455f75

    
           if /darwin/ =~ RUBY_PLATFORM 
        
             it { expect(pdf_reader.pages.first.attributes).to include(MediaBox: [0, 0, 841.91998, 1188]) } 
        
           else 
        
             it { expect(pdf_reader.pages.first.attributes).to include(MediaBox: [0, 0, 841.91998, 1189.91992]) } 
        
           end

It's using the expectation in the first branch. But on my system, I have MediaBox: [0, 0, 841.91998, 1189.91992], which matches the second expectation.

I'm running MacOS Catalina 10.15.3 with ImageMagick:

Version: ImageMagick 6.9.10-84 Q16 x86_64 2020-01-06 https://imagemagick.org
Copyright: © 1999-2020 ImageMagick Studio LLC
License: https://imagemagick.org/script/license.php
Features: Cipher DPC Modules
Delegates (built-in): bzlib freetype jng jp2 jpeg lcms ltdl lzma png tiff webp xml zlib

I can ignore for now. May be due to the newer version of ImageMagick on my system?

willkoehler · 2020-02-23T21:38:24Z

Also FYI this spec is failing.

grover/spec/grover_spec.rb

Lines 313 to 317 in d455f75

    
           it do 
        
             expect do 
        
               to_pdf 
        
             end.to raise_error Schmooze::JavaScript::Error, %r{Failed to launch chrome! spawn /totes/invalid/path} 
        
           end

With

expected Schmooze::JavaScript::Error with message matching /Failed to launch chrome! spawn \/totes\/invalid\/path/, got #<Schmooze::JavaScript::Error: Failed to launch the browser process! spawn /totes/invalid/path ENOENT

And this spec is failing

grover/spec/grover_spec.rb

Line 395 in d455f75

    
           it { expect(image.data.dig('imageStatistics', 'all', 'mean').to_f).to be_within(1).of 97.7473 }

With

Failure/Error: it { expect(image.data.dig('imageStatistics', 'all', 'mean').to_f).to be_within(1).of 97.7473 }
       expected 161.497 to be within 1 of 97.7473

These failures are on master and not caused by my changes. I can ignore for now.

willkoehler · 2020-02-24T00:08:54Z

Please hold off on merge until I add a few specs...

abrom · 2020-02-24T00:29:56Z

Edit: Nevermind. I get it now. It converts strings that contains arrays into arrays.
Ex "['--something']" --> ['--something']

Correct. It allows the launch_args to be passed in through a meta tag (string value) and parse it as an array

abrom · 2020-02-24T00:38:18Z

Also FYI this spec is failing.

grover/spec/grover_spec.rb

Lines 313 to 317 in d455f75

it do

expect do

to_pdf

end.to raise_error Schmooze::JavaScript::Error, %r{Failed to launch chrome! spawn /totes/invalid/path}

end

With
expected Schmooze::JavaScript::Error with message matching /Failed to launch chrome! spawn \/totes\/invalid\/path/, got #<Schmooze::JavaScript::Error: Failed to launch the browser process! spawn /totes/invalid/path ENOENT

Right. Puppeteer have been moving towards making the library driver agnostic and it looks like they've released a new version that does that (along with some changes to the error message copy). Best solution is likely going to be to make the copy match a regex to allow for chrome or browser process. You're welcome to make that change, or I'll get to it when I can.

And this spec is failing

grover/spec/grover_spec.rb

Line 395 in d455f75

it { expect(image.data.dig('imageStatistics', 'all', 'mean').to_f).to be_within(1).of 97.7473 }

With
Failure/Error: it { expect(image.data.dig('imageStatistics', 'all', 'mean').to_f).to be_within(1).of 97.7473 }
       expected 161.497 to be within 1 of 97.7473
These failures are on master and not caused by my changes. I can ignore for now.

I've never been a fan of the method used here, but it was a lot more reliable than pixel testing. It appears to be passing in Travis.. no doubt slightly different versions of the ImageMagick toolset etc

willkoehler · 2020-02-24T01:24:02Z

Right. Puppeteer have been moving towards making the library driver agnostic and it looks like they've released a new version that does that (along with some changes to the error message copy). Best solution is likely going to be to make the copy match a regex to allow for chrome or browser process. You're welcome to make that change, or I'll get to it when I can.

I added fix for this. See 0d1b1c9

I've never been a fan of the method used here, but it was a lot more reliable than pixel testing. It appears to be passing in Travis.. no doubt slightly different versions of the ImageMagick toolset etc

I added a variant to fix the spec on my system See 3d10ae5. Feel free to pull if this isn't a direction you want to go.

…gick

willkoehler · 2020-02-24T01:45:28Z

This is ready for another review. We might want to keep 561127f un-squashed to document that change. I'm happy to rebase to keep the commits we want and squash everything else.

willkoehler · 2020-02-24T17:30:55Z

Had some insight overnight. I'm adding some specs for OptionsBuilder please don't merge until those are in.

willkoehler · 2020-02-24T18:47:22Z

OptionsBuilder specs are in. Everything is ready for a review 👍

With the addition of OptionsBuilder specs, it looks like you can move some of the Grover specs to OptionsBuilder and eliminate a few of the dependencies on image content and PDF generation? I'm not sure of the full intention of those specs, so I'll leave that to you if it makes sense.

abrom

Change is looking really good @willkoehler 👍

I've noticed a few things that I think should be addressed before merging though. Please see comments below.

lib/grover/options_builder.rb

lib/grover/options_fixer.rb

spec/grover_spec.rb

spec/grover/options_fixer_spec.rb

spec/grover/options_builder_spec.rb

abrom · 2020-02-25T14:07:07Z

This is ready for another review. We might want to keep 561127f un-squashed to document that change. I'm happy to rebase to keep the commits we want and squash everything else.

I'm not too bothered either way. It's a pretty trivial fix and only relates to the specs so not sure it really needs much documenting

abrom · 2020-02-25T14:08:16Z

OptionsBuilder specs are in. Everything is ready for a review 👍

With the addition of OptionsBuilder specs, it looks like you can move some of the Grover specs to OptionsBuilder and eliminate a few of the dependencies on image content and PDF generation? I'm not sure of the full intention of those specs, so I'll leave that to you if it makes sense.

Not sure I following your meaning here? The image/PDF reader dependencies are necessary to validate the conversion process is doing what it's supposed to. How would this change eliminate that need?

willkoehler · 2020-02-25T15:52:01Z

Not sure I following your meaning here? The image/PDF reader dependencies are necessary to validate the conversion process is doing what it's supposed to. How would this change eliminate that need?

I though some of those specs may have just been checking that parameters were parsed and combined correctly. (Just a guess on my part since I don't have much context.) But if they are validating the conversion process, I agree, it wouldn't make sense to change them.

willkoehler · 2020-02-25T15:52:57Z

I'm not too bothered either way. It's a pretty trivial fix and only relates to the specs so not sure it really needs much documenting

Good point. We should just squash it in.

willkoehler · 2020-02-25T18:20:20Z

I fixed the last remaining broken spec on my system: c4abfb4

willkoehler · 2020-02-25T18:39:26Z

The failure is a flickering spec: https://travis-ci.org/Studiosity/grover/jobs/655011626. Is there a way I can re-run these? 👇

abrom · 2020-02-26T12:02:28Z

No, but I can :)

I'm thinking I might add https://github.com/NoRedInk/rspec-retry at some point to help with the flake. It's often an issue with puppeteer just not booting. Bit odd..

abrom

Nice one @willkoehler, looks great! 👍

willkoehler · 2020-02-26T12:38:39Z

Nice one @willkoehler, looks great! 👍

Awesome 🙌 Let me know if anything else comes up that I can help with. It's a pleasure working with you.

abrom · 2020-02-26T12:51:15Z

Great, thanks @willkoehler. I can't think of any pressing changes needed at the moment. I'm currently updating the OptionsFixer to include some more of the missing type casting (some viewport options and some screenshot options).

There are plenty of things that Puppeteer can do that Grover doesn't support, but to be honest I'm not sure the merits of going all in on trying to get full API support.

WIP

7e4f0e3

abrom reviewed Feb 23, 2020

View reviewed changes

willkoehler added 2 commits February 23, 2020 11:23

Ensure color stats are strings

561127f

This fixes expected: ["165", "42", "42"] got: [165, 42, 42] spec errors caused be a change in ImageMagick. See ImageMagick/ImageMagick@bc978e0#diff-b911bfc2e4a717df4fc996ef7decb2ffL713-R716

Start with Grover.configuration.options when building options

2bbd156

willkoehler added 2 commits February 23, 2020 16:40

Add newlines to end of files

5972cf9

Fix fix_array_options! to match behavior in Grover class

ee94464

willkoehler force-pushed the extract_option_logic branch from 2c52623 to ee94464 Compare February 23, 2020 22:15

willkoehler added 2 commits February 23, 2020 17:18

Extract shared logic from xx_fix_options! methods

306a936

Rubocop fixes

8c6502d

Add OptionsFixer specs

fd5705f

willkoehler added 2 commits February 23, 2020 20:32

Allow either "chrome" or "the browser process" in Grover specs

0d1b1c9

Add variant of imageStatistics in Grover specs to match newer ImageMa…

3d10ae5

…gick

willkoehler force-pushed the extract_option_logic branch from 68593e5 to 3d10ae5 Compare February 24, 2020 01:33

Add OptionsBuilder specs

6106856

willkoehler force-pushed the extract_option_logic branch from dfa1767 to 6106856 Compare February 24, 2020 18:36

abrom requested changes Feb 25, 2020

View reviewed changes

willkoehler added 7 commits February 25, 2020 11:13

Add spec to test precedence of meta options

840f0ed

Remove unneeded method parentheses

ea6f361

Add PDFKit attribution

a19b550

Rename options to paths

6cab3ef

Restore spec removed in 6106856

a8fd431

Add comment explaining different imageStatistics values

d90733d

Fix typos in spec example names

70c16de

Fix spec to work with Mac platform with newer version of ImageMagick

30c5cb8

willkoehler force-pushed the extract_option_logic branch from c4abfb4 to 30c5cb8 Compare February 25, 2020 18:27

willkoehler requested a review from abrom February 25, 2020 18:40

abrom approved these changes Feb 26, 2020

View reviewed changes

abrom merged commit 10e1adc into Studiosity:master Feb 26, 2020

willkoehler deleted the extract_option_logic branch February 26, 2020 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract option logic from Grover class #46

Extract option logic from Grover class #46

willkoehler commented Feb 23, 2020

abrom left a comment

willkoehler commented Feb 23, 2020 •

edited

Loading

abrom commented Feb 23, 2020

willkoehler commented Feb 23, 2020

willkoehler commented Feb 23, 2020 •

edited

Loading

willkoehler commented Feb 23, 2020 •

edited

Loading

willkoehler commented Feb 23, 2020

willkoehler commented Feb 24, 2020 •

edited

Loading

abrom commented Feb 24, 2020

abrom commented Feb 24, 2020

willkoehler commented Feb 24, 2020 •

edited

Loading

willkoehler commented Feb 24, 2020

willkoehler commented Feb 24, 2020

willkoehler commented Feb 24, 2020

abrom left a comment

abrom commented Feb 25, 2020

abrom commented Feb 25, 2020

willkoehler commented Feb 25, 2020

willkoehler commented Feb 25, 2020

willkoehler commented Feb 25, 2020

willkoehler commented Feb 25, 2020 •

edited

Loading

abrom commented Feb 26, 2020

abrom left a comment

willkoehler commented Feb 26, 2020

abrom commented Feb 26, 2020

Extract option logic from Grover class #46

Extract option logic from Grover class #46

Conversation

willkoehler commented Feb 23, 2020

abrom left a comment

Choose a reason for hiding this comment

willkoehler commented Feb 23, 2020 • edited Loading

abrom commented Feb 23, 2020

willkoehler commented Feb 23, 2020

willkoehler commented Feb 23, 2020 • edited Loading

willkoehler commented Feb 23, 2020 • edited Loading

willkoehler commented Feb 23, 2020

willkoehler commented Feb 24, 2020 • edited Loading

abrom commented Feb 24, 2020

abrom commented Feb 24, 2020

willkoehler commented Feb 24, 2020 • edited Loading

willkoehler commented Feb 24, 2020

willkoehler commented Feb 24, 2020

willkoehler commented Feb 24, 2020

abrom left a comment

Choose a reason for hiding this comment

abrom commented Feb 25, 2020

abrom commented Feb 25, 2020

willkoehler commented Feb 25, 2020

willkoehler commented Feb 25, 2020

willkoehler commented Feb 25, 2020

willkoehler commented Feb 25, 2020 • edited Loading

abrom commented Feb 26, 2020

abrom left a comment

Choose a reason for hiding this comment

willkoehler commented Feb 26, 2020

abrom commented Feb 26, 2020

willkoehler commented Feb 23, 2020 •

edited

Loading

willkoehler commented Feb 23, 2020 •

edited

Loading

willkoehler commented Feb 23, 2020 •

edited

Loading

willkoehler commented Feb 24, 2020 •

edited

Loading

willkoehler commented Feb 24, 2020 •

edited

Loading

willkoehler commented Feb 25, 2020 •

edited

Loading