ExifRewriterRoundtripTest fixed, completed & enabled

[StefanOltmann]

This completes the work started in PR #275.

This unit test will prove that the fix in PR #359 / #203 is correct.

Drop this testfile.jpg into src/test/resources/data/images/jpg to make the test fail.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 71.58%. Comparing base (fd2b5b8) to head (6bccf18). Report is 78 commits behind head on master.

Files	Patch %	Lines
...ng/formats/tiff/write/TiffImageWriterLossless.java	0.00%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #366      +/-   ##
============================================
+ Coverage     71.48%   71.58%   +0.09%     
- Complexity     3580     3584       +4     
============================================
  Files           352      352              
  Lines         16919    16905      -14     
  Branches       2648     2648              
============================================
+ Hits          12095    12101       +6     
+ Misses         3785     3764      -21     
- Partials       1039     1040       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[garydgregory]

Hello @StefanOltmann

I don't think I understand what you are saying here. What I see in this PR is:

• the addition of a utility method to main and
• changes to a test case
• the claim that this PR proves that another PR (#359) fixes an issue

But, this means (to me) that the test should fail without the fix in #359, it does not, since that PR is open. So the test does not test what it claims to test.

In general, you should include a test with a PR, which #359 does not. So I would ask that you provide one PR with the main and test changes in the same PR, not split into two.

TY!

[StefanOltmann]

But, this means (to me) that the test should fail without the fix in #359, it does not, since that PR is open. So the test does not test what it claims to test.

Please read carefully what I wrote here:

Drop this testfile.jpg into src/test/resources/data/images/jpg to make the test fail.

You won't accept a PR with a failing test, would you? So I did not include the test file.

And if I fix the test and the code in one PR, you will say "nothing changed".

The test now does not fail, because none of the test files present raises the issue.

@kinow Can you take a look at this?

[StefanOltmann]

@garydgregory As you requested #367 is a combined PR of bugfix and repaired unit test plus a file that shows the problem.

[garydgregory]

A PR should include main and test changes, not one PR for test changes, and a different PR for main changes.

[StefanOltmann]

A PR should include main and test changes, not one PR for test changes, and a different PR for main changes.

Yes, but the bugfix and the test are unrelated.

One PR fixes a bug without a test. The other PR completes a test that’s right now disabled, because it wasn’t working.

[garydgregory]

Hi @kinow Any thoughts?

[StefanOltmann]

I have a working copy with some modifications, so let's try not to touch it just for a few days 👍

If you test it locally don't forget to drop in the test file as described in my first post. This will show you the problem my PR #359 fixes.

[kinow]

If you test it locally don't forget to drop in the test file as described in my first post.

Noted :+1: You did a great job on the comments (in the code and in the pull request). We do not always get pull requests like this :slightly_smiling_face: Apologies for the delay, but wait just a bit more and this should be fixed.

For future PR's; it's common to have partial reviews. In some cases a reviewer might leave a note-to-self or to other committers (and maybe bring it to discuss over Slack or mailing list) in one of the comments, or may leave a partial review about something that, once the rest of the code is reviewed, does not need to be changed.

But once the review is done, you can be sure that someone will tag you, like "@StefanOltmann, I've reviewed it and it looks great! LGTM!", or "@StefanOltmann, can you look at my comment about the change you did in the code XPTO.java, please? I did not understand why you modified this or that".

Thanks again!

[StefanOltmann]

@garydgregory Let me quote myself from https://github.com/apache/commons-imaging/pull/367#issuecomment-1967723718:

I think by now I did everything that I can do to contribute back something and you can either take my donation or leave it. That will be up to you.

I won't do any of the changes you requested.

[kinow]

• Replaced test code from TiffOutputField by a public getter/setter for the bytes data (the getter is a copy, like in other formats)
• Updated tests to use assertArrayEquals from JUnit instead of custom test code
• Updated a few lines to match the formatting of other classes
• Added link to the issue/PR where the test was added (for traceability)
• Added Javadocs to public methods changed

I added a second commit so we can still revert/remove things if needed, but if there are no objections I think this should be ready to be merged as soon as CI is happy (will take a look later, gotta go :wave: :runner: )

p.s. will hold merging this until the current ongoing release is out, I think, to avoid causing any issues with the release progress.

[kinow]

Build errors are on macos (works on macos + java 8, fails for java 11).

Error: Java heap space

I will spend some minutes checking if there's any Maven setting that we can use to fix the build, but I think that shouldn't be a blocker.

I still want to test this part from @StefanOltmann 's comment before merging

Drop this testfile.jpg into src/test/resources/data/images/jpg to make the test fail.

Then I believe a good course of action here would be to merge this to get the test passing (thanks Stefan!), and continue working on #359. If I recall correctly we may not be able to include the test file, so I will check whether I can find another one, find another way to test that, or check with legal/others other options (no need to worry about this for now).

[kinow]

And tests passed after re-running the failed jobs on GitHub Actions :man_shrugging:

[garydgregory]

@kinow Thank you for your work here. I'll merge this once 1.0.0-alpha4 is out (in 3 days).

[kinow]

Rebased, added changes.xml entry, and squashed commits. The changes.xml has the due-to field set to Stefan and to Gary Lucas (as I believe Stefan used Gary's suggestion from JIRA :+1:).

Waiting for GH Actions now to complete before merging.

[StefanOltmann]

@kinow Thanks for merging it. :)

The changes.xml has the due-to field set to Stefan and to Gary Lucas (as I believe Stefan used Gary's suggestion from JIRA 👍).

This PR completes the work done in PR #275, started by Charles Hope. But since Gary Lucas found the bugfix and the test was done to showcase the bug it fixes, it's fine to give him credit. :)

[StefanOltmann]

I don't know eactly the policy is on the changes.xml, but I just noticed that there seems to be no entry for the initial ExifRewriterRoundtripTest.

So to give everyone involved proper credit maybe "Charles Hope" should be mentioned.

[kinow]

I don't know eactly the policy is on the changes.xml, but I just noticed that there seems to be no entry for the initial ExifRewriterRoundtripTest.

So to give everyone involved proper credit maybe "Charles Hope" should be mentioned.

Sounds good to me! I will do some searching in the git history to confirm where it should be added. Thanks @StefanOltmann !