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

Port redmine issues 831 to 937 to the new testsuite #353

Merged
merged 2 commits into from
Jun 6, 2018
Merged

Port redmine issues 831 to 937 to the new testsuite #353

merged 2 commits into from
Jun 6, 2018

Conversation

D4N
Copy link
Member

@D4N D4N commented Jun 4, 2018 •
edited
Loading

  • The decorators which copy & delete files now call the setUp & tearDown functions in a more logical order (setUp is called after the copy is made and tearDown before the file is deleted).
  • I have modified the actual test for issue 937a. The original test would run the following:
    runTest exiv2 -pa    $filename | sort            > $num-before.txt
    runTest exiv2 -m     $dataname  $filename
    runTest exiv2 -pa    $filename | sort            > $num-after.txt
    diff $diffargs $num-before.txt $num-after.txt    > $num.txt
    diff $diffargs $num.txt        $diffname

However, the first command produces no output at all, thus the diff $num.txt is just $num-after.txt prepended with > . To be honest, I then don't really get the point of comparing that diff to another diff (if I understood the redmine discussion correctly, the original diff is the output before the respective patch got applied). For simply testing the behavior of exiv2, it should be sufficient to just check the output of the second invocation of exiv2 -pa $filename.

@D4N D4N requested review from piponazo and clanmills June 4, 2018 22:40
@D4N D4N force-pushed the redmine_issues_831_to_937 branch from e65f1e6 to 23c73b9 Compare June 4, 2018 22:53
@D4N
Copy link
Member Author

D4N commented Jun 4, 2018

@clanmills Could you take a look at the test for issue 836? It is failing on Travis on OSX (the only platform that has resource forks), but I am unsure if it is a regression or my mistake. Also, please note that resource forks are now accessible via $filename/..namedfork/rsrc and not $filename/rsrc.

@D4N D4N force-pushed the redmine_issues_831_to_937 branch 3 times, most recently from 03052ca to 4b65458 Compare June 5, 2018 05:40
@D4N
Copy link
Member Author

D4N commented Jun 5, 2018

@clanmills Could you take a look at the test for issue 836? It is failing on Travis on OSX (the only platform that has resource forks), but I am unsure if it is a regression or my mistake. Also, please note that resource forks are now accessible via $filename/..namedfork/rsrc and not $filename/rsrc.

Nevermind, I forgot to copy the resource fork correctly. It seems to work now.

piponazo
piponazo approved these changes Jun 5, 2018
View reviewed changes
Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

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

I just made few suggestions, feel free to take actions about them or not. Good job!

tests/bugfixes/redmine/test_issue_841.py Outdated
from system_tests import CaseMeta, path


class CrashOnInput(metaclass=CaseMeta):
Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion] Should we rename the test from crash... to throw... ? The application was crashing before the fix, but now it is throwing an exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh, yeah, now that I re-read the class name, it sounds like it should crash...

tests/bugfixes/redmine/test_issue_855.py Outdated
from system_tests import CaseMeta, path


class SegfaultWhenAccessingFocalLength(metaclass=CaseMeta):
Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion] Same comment here. I guess the application was crashing before, but now we are controlling the situation.

tests/bugfixes/redmine/test_issue_884.py Outdated
"""Exif.Pentax.LensType Byte 2 PENTAX-F 28-80mm F3.5-4.5
"""
]
stderr = [""] * 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion] What about removing the hard-coded value (3) and use [""] * len(commands) ?

@D4N D4N mentioned this pull request Jun 5, 2018
Closed
38 tasks
D4N added 2 commits June 6, 2018 13:59
@D4N
[testsuite] Port redmine issues 831 to 937 to the new testsuite
39b7151
@D4N
[testsuite] Change the calling order of setUp & tearDown in decorator
35e7037 
The FileDecoratorBase injects a new setUp & tearDown function. These new
functions would call the old setUp & tearDown in an inconvenient order: e.g. the
child class CopyFiles would at first call the user provided setUp and then copy
the files. This makes it impossible to perform some action on the file copy in
setUp.
=> This commit changes the call order, so that setUp & tearDown always "see" the
finished environment after file copies are in place and before any cleanup took
place.
@D4N D4N force-pushed the redmine_issues_831_to_937 branch from fabf3de to 35e7037 Compare June 6, 2018 12:00
@D4N D4N merged commit 493b728 into master Jun 6, 2018
@D4N D4N deleted the redmine_issues_831_to_937 branch June 6, 2018 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piponazo piponazo piponazo approved these changes

@clanmills clanmills Awaiting requested review from clanmills

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@D4N @piponazo