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.

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

allow passing -api parameters to exiftool #59

Merged
merged 2 commits into from
Jul 10, 2022

Conversation

Blesmol
Copy link
Contributor

@Blesmol Blesmol commented Jun 15, 2022

No description provided.

@barasher
Copy link
Owner

Hi, thanks for the PR.
Is it possible to add an integration test that use this option and that would be used as a non regression test ?

@Blesmol
Copy link
Contributor Author

Blesmol commented Jun 16, 2022

HI @barasher ,

I can try, although the only exiftool api option that I use so far is QuickTimeUTC, and I need to see on how a proper integration test for this could look like. Do you have an example for other integration tests within go-exiftool?

@barasher
Copy link
Owner

In fact, when I add a new option, I try to check if it effectively does what it is supposed to do.
Since it is not possible to check every API parameter values, I would just check one. I would check on a specific file that a specific field is extracted with a specific API parameter value and that it isn't without the API parameter.
It would also indirectly check that :

  • the API parameter is still compatible with the JSON serialization
  • there is no breaking change on exiftool with its API parameter

@barasher
Copy link
Owner

Have a look at the TestExtractAllBinaryMetadata test:

  • it checks extraction without the exiftool parameter (the field X is not extracted)
  • it check extraction with the exiftool parameter (the field X is now extracted)

@Blesmol
Copy link
Contributor Author

Blesmol commented Jun 21, 2022

Sorry for the delay, currently only able to check this every couple of days.

Some open questions on my side:

  • You're bringing TestExtractAllBinaryMetadata as example. But isn't that a regression test, although initially you were mentioning non-regression tests? How do you differentiate here between both? In general I can add something like this. Need to see whether I can provide a binary testfile that is as small as possible.
  • Which JSON serialization? Happy to test this as well, but wasn't aware that there is some JSON serialization anywhere.
  • The part about "no breaking change on exiftool with its API parameter" I did not understand...

@barasher
Copy link
Owner

barasher commented Jun 22, 2022

You're bringing TestExtractAllBinaryMetadata as example. But isn't that a regression test, although initially you were mentioning non-regression tests? How do you differentiate here between both? In general I can add something like this. Need to see whether I can provide a binary testfile that is as small as possible.

The idea of this test is to check that the Api function is concretely working. The best way to test it is to try it with a concrete sample that returns something with the Api settings that exiftools does not return without it. It proves that it works at least in one use-case.

Here is a sample with an existing file in the repository :

barasher@linux:~/go/src/github.com/barasher/go-exiftool/testdata$ exiftool 20190404_131804.jpg | grep 'File Block Size'

barasher@linux:~/go/src/github.com/barasher/go-exiftool/testdata$ exiftool -api systemtags 20190404_131804.jpg | grep 'File Block Size'
File Block Size                 : 4096

I call it a non regression test because if anything changes with the exiftool's -api parameter (for instance, -api is renamed -api_parameter, this unit-test should not work anymore. In go-exiftool's point of view, if something that used to work does not work anymore, it is a regression. The test checks that there is not any regression concerning the parameter.

I think that we agree and that it is just a vocabulary issue :)


Which JSON serialization? Happy to test this as well, but wasn't aware that there is some JSON serialization anywhere.

go-exiftool uses internally exiftool's -j parameter that formats metadata in JSON:

barasher@linux:~/go/src/github.com/barasher/go-exiftool/testdata$ exiftool 20190404_131804.jpg
Orientation                     : Rotate 90 CW
X Resolution                    : 72
Y Resolution                    : 72

barasher@linux:~/go/src/github.com/barasher/go-exiftool/testdata$ exiftool -j 20190404_131804.jpg
[{
  "Orientation": "Rotate 90 CW",
  "XResolution": 72,
  "YResolution": 72
}]

I feared that exiftools -api parameter could not be compatible with the -j parameter but it seems that it's ok:

barasher@linux:~/go/src/github.com/barasher/go-exiftool/testdata$ exiftool -j -api systemtags 20190404_131804.jpg 
[{
  "FileBlockSize": 4096
}]

So, this should be OK with go-exiftool

@barasher
Copy link
Owner

barasher commented Jul 3, 2022

@Blesmol, are you still OK to work on this PR ?

@Blesmol
Copy link
Contributor Author

Blesmol commented Jul 3, 2022

@barasher Yep. Sorry for the delay, Newborn child in the family takes up a lot of time :) Will see that I find time within the next couple of days (after getting some proper sleep)

@Blesmol
Copy link
Contributor Author

Blesmol commented Jul 5, 2022

I think that we agree and that it is just a vocabulary issue :)

I agree :) What you described there I would call a regression test. The "non regression" part was what was confusing me in the wording.

I've added now another unittest that tests an actual api setting and shows that the parameter has an impact. Would that be sufficient? If I understood you correctly, then for the JSON part no additional tests are required at the moment?

@barasher
Copy link
Owner

barasher commented Jul 7, 2022

Congratulations for your newborn baby !
Thanks for the PR, I'll certainly review and merge it this weekend !

@codecov-commenter
Copy link

Codecov Report

Merging #59 (a8df253) into master (70f0835) will increase coverage by 0.83%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   79.71%   80.54%   +0.83%     
==========================================
  Files           2        2              
  Lines         281      293      +12     
==========================================
+ Hits          224      236      +12     
  Misses         40       40              
  Partials       17       17              
Impacted Files Coverage Δ
exiftool.go 72.33% <100.00%> (+1.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70f0835...a8df253. Read the comment docs.

@barasher barasher merged commit f555e83 into barasher:master Jul 10, 2022
@barasher barasher added this to the v1.9.0 milestone Jul 10, 2022
@Blesmol Blesmol mentioned this pull request Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants