Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix failing test for creation of JPEG2000 derivatives. #142

Merged
merged 5 commits into from
Mar 31, 2017
Merged

Fix failing test for creation of JPEG2000 derivatives. #142

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f9c61e4
Add coverage/ to .gitignore
dlpierce Mar 31, 2017
2945c07
Un-protect some class methods to fix #118
dlpierce Mar 31, 2017
4570561
Specify .jp2 mime type to prevent .txt being used by default.
dlpierce Mar 31, 2017
af2af77
Tests for newly unprotected methods.
dlpierce Mar 31, 2017
c196e6e
Cleanup tests.
dlpierce Mar 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ pkg/
.ruby-gemset
fcrepo4-data/
fcrepo4-test-data/
coverage/
22 changes: 10 additions & 12 deletions lib/hydra/derivatives/processors/jpeg2k_image.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,20 +73,18 @@ def layer_rates(layer_count, compression_numerator)
rates.map(&:to_s).join(',')
end

protected
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing that the following methods are now becoming public. What was your reason for removing the protected declaration?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failing spec that he fixed had this error:

8) Transcoding with an attached tiff transcodes
     Failure/Error: long_dim = self.class.long_dim(image)
     
     NoMethodError:
       protected method `long_dim' called for Hydra::Derivatives::Processors::Jpeg2kImage:Class

"protected" for class methods apparently doesn't behave in the way you might expect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @val99erie for the clarification; Always hesitant to public-ize new methods, but this sounds like a necessary reason to open the public facing methods.

Copy link
Contributor

@val99erie val99erie Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an older explanation of why it won't work to have a protected class method and then try to call it from within an instance method. The only receiver that is allowed is self; self.class won't work.
http://stackoverflow.com/questions/11461262/what-is-the-value-of-self-in-a-rails-model-and-why-arent-obvious-instance-metho/11461333#11461333

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up. I missed the context that these were class methods as well. I'm all for this change.


def encode(path, recipe, output_file)
kdu_compress = Hydra::Derivatives.kdu_compress_path
execute "#{kdu_compress} -quiet -i #{path} -o #{output_file} #{recipe}"
end
def encode(path, recipe, output_file)
kdu_compress = Hydra::Derivatives.kdu_compress_path
execute "#{kdu_compress} -quiet -i #{path} -o #{output_file} #{recipe}"
end

def tmp_file(ext)
Dir::Tmpname.create(['sufia', ext], Hydra::Derivatives.temp_file_base) {}
end
def tmp_file(ext)
Dir::Tmpname.create(['sufia', ext], Hydra::Derivatives.temp_file_base) {}
end

def long_dim(image)
[image[:width], image[:height]].max
end
def long_dim(image)
[image[:width], image[:height]].max
end
end

def process
Expand Down
23 changes: 23 additions & 0 deletions spec/processors/jpeg2k_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

describe Hydra::Derivatives::Processors::Jpeg2kImage do
let(:object) { ActiveFedora::Base.new }
let(:filename) { File.expand_path('../../fixtures/test.tif', __FILE__) }
let(:image) { MiniMagick::Image.open(filename) }

describe "#calculate_recipe" do
it "calculates the number of levels from a size" do
Expand Down Expand Up @@ -54,4 +56,25 @@
expect(r).to eq(described_class.calculate_recipe(args, 'grey', 7200))
end
end

describe "#encode" do
it "executes the external utility" do
expect(described_class).to receive(:execute) { 0 }
described_class.encode('infile', 'recipe', 'outfile')
# expect(Hydra::Derivatives::Processors::ShellBasedProcessor).to have_received(:execute)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the commented out code. It looks like this is tested in the above expect segment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, missed it in the rush for lunch.

end
end

describe "#tmp_file" do
it "returns a temp file with the correct extension" do
f = described_class.tmp_file('test')
expect(f).to match(/sufia.*test$/)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjcolvar You had made mention of being concerned about the "sufia" assumption in file names. Could you take a look here?

I'm wondering if it would be okay to ignore the sufia part of the regular expression?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That conversation was based on #99 . I agree that it seems possible to remove the sufia from the regex and maybe change this to an ends_with? ".test".

end
end

describe "long_dim" do
it "returns the image's largest dimension" do
expect(described_class.long_dim(image)).to eq(386)
end
end
end
9 changes: 5 additions & 4 deletions spec/units/transcoding_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ def create_derivatives(_filename)
when 'image/tiff'
Jpeg2kImageDerivatives.create(self, source: :original_file,
outputs: [
{ label: :resized, recipe: :default, processor: 'jpeg2k_image', resize: "600x600>", url: "#{uri}/resized" },
{ label: :config_lookup, recipe: :default, processor: 'jpeg2k_image', url: "#{uri}/config_lookup" },
{ label: :string_recipe, recipe: '-quiet', processor: 'jpeg2k_image', url: "#{uri}/string_recipe" },
{ label: :diy, processor: 'jpeg2k_image', url: "#{uri}/original_file_diy" }])
{ label: :resized, format: 'jp2', recipe: :default, processor: 'jpeg2k_image', resize: "600x600>", url: "#{uri}/resized" },
{ label: :config_lookup, format: 'jp2', recipe: :default, processor: 'jpeg2k_image', url: "#{uri}/config_lookup" },
{ label: :string_recipe, format: 'jp2', recipe: '-jp2_space sRGB', processor: 'jpeg2k_image', url: "#{uri}/string_recipe" },
{ label: :diy, format: 'jp2', processor: 'jpeg2k_image', url: "#{uri}/original_file_diy" }])
when 'image/x-adobe-dng'
ImageDerivatives.create(self, source: :original_file,
outputs: [
Expand Down Expand Up @@ -318,6 +318,7 @@ def create_derivatives(_filename)
let(:file) do
GenericFile.new(mime_type_from_fits: 'image/tiff').tap do |t|
t.original_file.content = attachment
t.original_file.mime_type = 'image/tiff'
t.save
end
end
Expand Down