feat: prepare code for a new image processor #30
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The spec is now visually comparing the images so it does not need a mock
The exact computed height is 243.75. Pixbuf seems to trim the fraction, but 244 feels more accurate
…mpty Some libraries may treat it as unknown type (Vips), the others as corrupt image (Pixbuf)
Vips-based processor will not support all the pixbuf operations, this commit extracts the ones which we plan to support into a shared_examples block, so that they can be reused when the time comes
One of the background styles (dominant) was not included; the quality is actually an integer
In case of shared examples, line number may be duplicated leading to overwriting the images from other specs
MrLukeSmith
approved these changes
Nov 7, 2024
spec/morandi_spec.rb
Outdated
Comment on lines
54
to
56
| (expect { process_image }).to(raise_error do |err| | ||
| err.is_a?(Morandi::UnknownTypeError) or err.is_a?(Morandi::CorruptImageError) | ||
| end) |
There was a problem hiding this comment.
Suggested change
| (expect { process_image }).to(raise_error do |err| | |
| err.is_a?(Morandi::UnknownTypeError) or err.is_a?(Morandi::CorruptImageError) | |
| end) | |
| expect { process_image }.to raise_exception(Morandi::UnknownTypeError, Morandi::CorruptImageError) |
Untested, but couldn't you do something like this instead?
There was a problem hiding this comment.
The second argument of raise_exception is an expected error message :-)
Expected raised exception #<Morandi::CorruptImageError "Image file “sample/sample.jpg” contains no data">
to match #<Morandi::UnknownTypeError Morandi::CorruptImageError>
Alternatively I can do the following if you find it nicer (?)
expect { process_image }.to raise_error(
an_instance_of(Morandi::UnknownTypeError)
.or(an_instance_of(Morandi::CorruptImageError))
)There was a problem hiding this comment.
Ah, of course. 🤦. I've no strong opinions, it was mainly that it felt like there should be a simpler way to achieve that.
| @@ -73,7 +73,7 @@ def expose_from(reference_path:, tested_path:, diff_path:) | |||
| return true if @normalized_mean_error <= tolerance | |||
|
|
|||
| metadata = RSpec.current_example.metadata | |||
| spec_name = "#{metadata[:absolute_file_path].split('/spec/').last}:#{metadata[:line_number]}" | |||
| spec_name = "#{metadata[:absolute_file_path].split('/spec/').last}:#{metadata[:scoped_id]}" | |||
MrLukeSmith
approved these changes
Nov 8, 2024
There was a problem hiding this comment.
I've no issue with it in its current guise, it was just on the off-chance there was a simpler way to achieve the same thing. 😄
The approach you chose is still easily digestible with clear intentions. Wouldn't have approved it the first time around otherwise. 🚀
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
I'm prototyping
libvips-based Morandi on a separate branch in hope of improving its memory profile and performance. We want to provide ability to render using either Pixbuf of Vips within a single version of Morandi.Problems
Solutions
shared_examplesgroup (unchanged within a dedicated commit)Notes