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

Port redmine issues from 1024 to 1170 #367

Merged
merged 3 commits into from
Jul 31, 2018
Merged

Port redmine issues from 1024 to 1170 #367

merged 3 commits into from
Jul 31, 2018

Conversation

piponazo
Copy link
Collaborator

In this PR I'm porting most of the remaining redmine issues. However I skipped few of them:

  • 1043 & 1074 Need further analysis.
  • 1137 Is testing the piping of two exiv2 executions and our python test infrastructure is not prepared for that.

I just want to contribute what I have done until now. In the next weeks I will be less available and I did not want to block the removal of the printStructure function.

@piponazo piponazo requested a review from D4N June 11, 2018 19:32
@piponazo
Copy link
Collaborator Author

piponazo commented Jul 5, 2018

@D4N Did you have the chance to look at these changes?

@D4N
Copy link
Member

D4N commented Jul 5, 2018 via email


url = "http://dev.exiv2.org/issues/1153"

filenames = ["$data_path/exiv2-bug1153Aa.exv",
Copy link
Member

Choose a reason for hiding this comment

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

Have you written the file names here for a reason? Since you can create both lists via:

    filenames = [
        "$data_path/exiv2-bug1153{E}{i}.exv".format(E=E, i=i)
        for E, i in itertools.product(
                ['A', 'J'],
                "a b c d e f g h i j k".split()
        )
    ]

    commands = [
        "$exiv2 -pa -g Lens {!s}".format(fname) for fname in filenames
    ]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Laziness. I will follow your proposal ;)

@D4N
Copy link
Member

D4N commented Jul 7, 2018

Could you try to re-create 15727cf with a different diff algorithm or change the commit message? To be honest, I haven't got a clue what got changed.

@D4N
Copy link
Member

D4N commented Jul 7, 2018

On my machine test 1054 fails with this diff:

-                       "DateTime": "2005:09:07 22:09:51",
?                                             ^^^^
+                       "DateTime": "2005:09:08 00:09:51",
?                                             ^^^^
                        "ImageDescription": "XMPFiles BlueSquare test file, created in Photoshop CS2, saved as .psd, .jpg, and .tif.",
                        "Make": "Nikon"
                },
                "Photo": {
                        "ColorSpace": 1,
                        "PixelXDimension": 360,
                        "PixelYDimension": 216,
-                       "DateTimeDigitized": "2005:09:07 22:07:40"
?                                                      ^^^^
+                       "DateTimeDigitized": "2005:09:08 00:07:40"
?                                                      ^^^^
                }

I'll take a look at that later, maybe it's just a local issue.

@D4N
Copy link
Member

D4N commented Jul 9, 2018

I have spent a while in gdb debugging the exiv2json call and found out that it is indeed a timezone issue. It seems that during metadata conversion, the dates are converted from the original timezone (which seems to be saved in xmp) into the local time zone (afaik exif metadata have no clue about timezones).

Anyway, long story short, in the bash testsuite there is a sneaky: export TZ=BST-1 in line 434 in functions.source, which explains why it works in the bash test suite, but not in the python one. I'll try to come up with a fix for this (I guess a new section in the test suite would be suitable) and then report back.

@piponazo
Copy link
Collaborator Author

piponazo commented Jul 9, 2018

Yes, I noticed this problem about the timezones, and at the end I decided to leave the code as in the original test. Maybe we can investigate this issue in the future (And create an issue to not forget about it). Otherwise the printStructure EPIC will be delayed.

@piponazo
Copy link
Collaborator Author

piponazo commented Jul 9, 2018

Could you try to re-create 15727cf with a different diff algorithm or change the commit message? To be honest, I haven't got a clue what got changed.

The main() function did not have curly brackets to delimit its body. It was the only change ... I do not know why the commit diff looks so odd. I think it is caused because my editor also fixed some indentation issues.

@D4N
Copy link
Member

D4N commented Jul 9, 2018

Maybe we can investigate this issue in the future

The problem with that is, that the python test suite will only work on local machines where the timezone is set to UTC (mine isn't). I would suggest that we temporarily skip the test unless os.getenv('TZ') == "UTC"? We could export TZ='UTC' on travis, so that we still retain the test coverage.

I have a rough idea how we could add this into the test suite, which would also fix the issue with piping exiv2 commands together. In a nutshell, the user could provide a Command class instance to a system test:

# -*- coding: utf-8 -*-

import system_tests


class aTest(metaclass=system_tests.CaseMeta):

    commands = [
        system_tests.Command(
            "$exiv2 $filename", stdin=Variable # or string,
            stdou=other_variable_where_stdout_is_written_to,
            env={"TZ": "UTC", "LANG": "C"}
        )
    ]
    # rest as expected

That will require some additional testing & fiddling with the metaclass, but it should be possible. I can create a new issue for a discussion of this.

The main() function did not have curly brackets to delimit its body. It was the only change ... I do not know why the commit diff looks so odd. I think it is caused because my editor also fixed some indentation issues.

Ah, I see. I am afraid there is not much that can be done, such diffs tend to look ugly. You can retry it with the patience or histogram diff algorithms, but I doubt they'll improve the situation.

@D4N
Copy link
Member

D4N commented Jul 28, 2018

@piponazo Could you squash your commits together, disable the test for issue 1054 unless os.getenv('TZ') == "UTC" and add an export TZ=UTC to travis? I'd merge this PR then.

@piponazo
Copy link
Collaborator Author

disable the test for issue 1054 unless os.getenv('TZ') == "UTC"

@D4N Is there an existing way to disable tests easily? I am not sure how to do this.

@D4N
Copy link
Member

D4N commented Jul 29, 2018

@D4N Is there an existing way to disable tests easily? I am not sure how to do this.

unittest.skip should work in this case. I think something like this should do the trick:

import os
import unittest

import system_tests

@unittest.skipUnless(os.getenv('TZ') == 'UTC', "Testcase only works with the timezone set to UTC")
class Exiv2jsonRecursiveJsonTreeWithXMP(metaclass=system_tests.CaseMeta):
    # rest here

piponazo added 3 commits July 31, 2018 17:55
 * Port redmine issue 1024
 * Port redmine issue 1026
 * Port redmine issue 1040
 * Port redmine issue 1044
 * Port redmine issue 1053
 * Port redmine issue 1054 (not finished yet; I found some issues there)
 * Port redmine issue 1058
 * Port redmine issue 1062
 * Port redmine issue 1080
 * Port redmine issue 1108
 * Port redmine issue 1112
 * Port redmine issue 1114
 * Port redmine issue 1122
 * Port redmine issue 1140
 * Port redmine issue 1144
 * Port redmine issue 1145
 * Port redmine issue 1153
 * Port redmine issue 1155
 * Port redmine issue 1166
 * Port redmine issue 1167
 * Port redmine issue 1170
 * Replace escaped chars in 1054
 * Add brackets in exiv2json::main()
 * Link all sample apps against exiv2lib
 * Changes in 1054
 * test 1054: fix dates depending on Local time
 * Do not run make with VERBOSE=1
 * Use system_tests.path
 * Fix windows issues with quotes
 * Use system_tests.path
 * Use itertools to simplify test code
This is needed in some tests which are using exiv2json.
It seems that during metadata conversion, the dates are converted from the
original timezone (which seems to be saved in xmp) into the local time zone.
Copy link
Member

@D4N D4N left a comment

Choose a reason for hiding this comment

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

Thanks for the grind work!

@piponazo piponazo merged commit 3d35b93 into Exiv2:master Jul 31, 2018
@piponazo piponazo deleted the redmine-1024-1170 branch July 31, 2018 16:18
@D4N D4N mentioned this pull request Jul 31, 2018
38 tasks
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.

2 participants