Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Made more tests green #122

Merged
merged 1 commit into from Feb 13, 2013

Conversation

Projects
None yet
2 participants
Contributor

kaspergrubbe commented Feb 12, 2013

I identified some problems between versions of ImageMagick.

1.8 ruby still fails due to some buffer issues with opening files.

Can we get a release soon ? 👍

@bensie bensie and 1 other commented on an outdated diff Feb 12, 2013

test/image_test.rb
@@ -125,7 +125,12 @@ def test_image_meta_info
assert_equal 150, image[:width]
assert_equal 55, image[:height]
assert_equal [150, 55], image[:dimensions]
- assert_equal 'PseudoClass sRGB', image[:colorspace]
+ # Difference between versions, lets match both for now:
+ # ImageMagick 6.6.9-7: identify -quiet -ping -format "%r\\n" "test/files/simple.gif"
+ # PseudoClassRGB
+ # ImageMagick 6.8.0-10: identify -quiet -ping -format "%r\\n" "test/files/simple.gif"
@bensie

bensie Feb 12, 2013

Owner

Let's just test against 6.8.x for this. The exact value of the string is less important than the fact that it is working and returning the colorspace.

@kaspergrubbe

kaspergrubbe Feb 12, 2013

Contributor

We wouldn't be able to trust Travis-ci then. 6.8 for Ubuntu and several other 'stable' distros is long away.

I think we could add a build script in the Travis-file that compiled ImageMagick 6.8 so we again could trust our tests.

@bensie

bensie Feb 12, 2013

Owner

Yeah we could do that, though that could be a nightmare of its own.

I really don't think the colorspace test is necessary at all, let's just remove it. The fact that an image renders differently below is pretty lame though. Got any ideas for that one? The goal here is to test that MiniMagick is shelling out to IM properly, not to test consistent ImageMagick output across all the possible versions.

@kaspergrubbe

kaspergrubbe Feb 12, 2013

Contributor

We could perhaps test for existence of the result.path-file. That must mean that the call to ImageMagick have gone through right?

@bensie bensie commented on an outdated diff Feb 12, 2013

test/image_test.rb
@@ -254,7 +259,12 @@ def test_simple_composite
result = image.composite(Image.open(TIFF_IMAGE_PATH)) do |c|
c.gravity "center"
end
- assert_equal Digest::MD5.hexdigest(File.read(result.path)), Digest::MD5.hexdigest(File.read("./test/files/composited.jpg"))
+ # ImageMagick renders images differently in the new 6.8.0-version
+ if MiniMagick.image_magick_version >= Gem::Version.create("6.8.0")
@bensie

bensie Feb 12, 2013

Owner

Same thing here -- 6.8.x only.

Contributor

kaspergrubbe commented Feb 12, 2013

Okay, we need to fix the complex_url test for Ruby 1.8, then we have passing test-suite. Do any have some insights to that?

@bensie bensie and 1 other commented on an outdated diff Feb 12, 2013

test/test_helper.rb
@@ -16,6 +16,6 @@ module MiniMagickTestFiles
CAP_EXT_PATH = test_files + "/trogdor_capitalized.JPG"
ANIMATION_PATH = test_files + "/animation.gif"
PNG_PATH = test_files + "/png.png"
- COMP_IMAGE_PATH = test_files + "/composited.jpg"
+ COMP_IMAGE_PATH = test_files + "/composited_post_6.8.jpg"
@bensie

bensie Feb 12, 2013

Owner

Can you rename this back to just composited.jpg? If it's not used, just remove it.

@kaspergrubbe

kaspergrubbe Feb 13, 2013

Contributor

Sure, I will rename it back to what it was.

Owner

bensie commented Feb 12, 2013

Not sure about the complex_url test for 1.8, but I will look at it later this week unless you can knock it out first.

Owner

bensie commented Feb 13, 2013

But 1.9 is smart enough to drop the query params to get the .png extension?

Contributor

kaspergrubbe commented Feb 13, 2013

I've made a check in the read-method, to deal with the issue where the input variable stream is a StringIO-object instead of a file.

Any comments? :)

Owner

bensie commented Feb 13, 2013

Looks good to me -- squash all this to a single commit and I'll merge it.

Contributor

kaspergrubbe commented Feb 13, 2013

Done, everything is squashed.

Can we get a Gem release as well? Just a 3.5-pre, perhaps.

@bensie bensie added a commit that referenced this pull request Feb 13, 2013

@bensie bensie Merge pull request #122 from kaspergrubbe/master
Made more tests green
870da5a

@bensie bensie merged commit 870da5a into minimagick:master Feb 13, 2013

Owner

bensie commented Feb 14, 2013

3.5.0 released!

Contributor

kaspergrubbe commented Feb 14, 2013

Awesome, thank you for quick response and release!

Owner

bensie commented Feb 14, 2013

No problem, it's a long time coming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment