Fix failing test for creation of JPEG2000 derivatives. #142
Conversation
Don't use '-quiet' because it is already hardcoded in the kdu_compress command string.
1 similar comment
spec/processors/jpeg2k_spec.rb
Outdated
| 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) |
There was a problem hiding this comment.
Please remove the commented out code. It looks like this is tested in the above expect segment.
There was a problem hiding this comment.
Thanks, missed it in the rush for lunch.
spec/processors/jpeg2k_spec.rb
Outdated
| 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$/) |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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".
| @@ -73,20 +73,18 @@ def layer_rates(layer_count, compression_numerator) | |||
| rates.map(&:to_s).join(',') | |||
| end | |||
|
|
|||
| protected | |||
There was a problem hiding this comment.
I'm seeing that the following methods are now becoming public. What was your reason for removing the protected declaration?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks for the follow-up. I missed the context that these were class methods as well. I'm all for this change.
| @@ -73,20 +73,18 @@ def layer_rates(layer_count, compression_numerator) | |||
| rates.map(&:to_s).join(',') | |||
| end | |||
|
|
|||
| protected | |||
There was a problem hiding this comment.
Thanks for the follow-up. I missed the context that these were class methods as well. I'm all for this change.
Fixes #118 by un-protecting some class methods that were originally not actually protected, but then made protected by rubocop. Specs for these methods coming shortly...
.jp2 derivatives tests were still broken due to .txt being used as a default mime-type and extension, so that is explicitly set now.