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

[WIP] Port redmine issues #287

Merged
merged 9 commits into from
May 24, 2018
Merged

[WIP] Port redmine issues #287

merged 9 commits into from
May 24, 2018

Conversation

D4N
Copy link
Member

@D4N D4N commented Apr 25, 2018

Please do not merge this yet!

This is the port of the issues 426-800 from redmine from bugfixes-test.sh to the new python test suite. That's about half of the redmine issues, the other half still has to be ported. I would like to port them all in one big PR so that we can check that we have the same test coverage.

The branch of this PR is in the main exiv2 repository, so everyone with write access to exiv2 can push to this branch too. Please do not push -f here! I'll clean the git history up before we merge this.

I'll ask for explicit approvals later on, so please do not approve this. Approving this means "I have verified that we have exactly the same test coverage with the new test suite"

@D4N D4N mentioned this pull request Apr 25, 2018
38 tasks
@D4N D4N force-pushed the port_redmine_issues branch from a86c189 to caacd82 Compare May 5, 2018 22:55
@D4N D4N force-pushed the port_redmine_issues branch 4 times, most recently from b59d690 to 0cb32c4 Compare May 17, 2018 16:56
@piponazo piponazo self-requested a review May 22, 2018 14:56
@piponazo
Copy link
Collaborator

Ey @D4N what is remaining for this PR? Could we merge it already ?

@D4N
Copy link
Member Author

D4N commented May 22, 2018

We can, but it's still incomplete, so I would rather not merge it yet.
This is only the first half of the Redmine issues, the second half has not been ported yet (except for issues 831-937 which are currently stashed in my local copy, waiting to be commited).

@piponazo
Copy link
Collaborator

I mentioned that because I thought you were only porting here from 426 to 800. I think there is no need to do everything in one single step. IMHO it is better to do small iterations rather than trying to work on a huge tasks via a single PR. For example, I have already reviewed your changes here and they look fine to me. If we wait few days/weeks more to have the rest of things, I will probably need to review everything again, since it will be difficult to differentiate between the new code and the one that I've already reviewed.

@D4N
Copy link
Member Author

D4N commented May 22, 2018

Actually I wanted to make this a "all-in-one-sweep" pr, but small steps won't hurt. Let me clean up the git history a bit (there are some useless commits from me left over) and rebase this on master once I have created the VM pull request (as that should also fix the CentOS build on gitlab).

@piponazo
Copy link
Collaborator

I was just my point of view, feel free to do as you please 😉

@D4N D4N force-pushed the port_redmine_issues branch from 193eda7 to 09c3d58 Compare May 22, 2018 20:47
@D4N
Copy link
Member Author

D4N commented May 22, 2018

@piponazo Would it be ok with you if I squased c5f2993, 5602841 and 71008bf? At least my commit from the 3 is useless, and the backslashes look like a mistake to me. I'd squash them into one commit with the message "Fix issues with python strings & literals" and would try to commit it in your name.

@piponazo
Copy link
Collaborator

Sure, go ahead!

@D4N D4N force-pushed the port_redmine_issues branch from 09c3d58 to 36cf236 Compare May 23, 2018 10:07
@D4N
Copy link
Member Author

D4N commented May 23, 2018

Done. The gitlab build (actually only the CentOS build) will still fail as #320 is not merged yet.

@piponazo
Copy link
Collaborator

@D4N Should not we remove the tests from the bash test-suite? Otherwise we will have duplicated tests

@@ -21,6 +21,3 @@ make tests
make install
cd bin
./unit_tests

cd ../../tests/
python3 runner.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this removing the execution of tests from travis?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, they are run by make tests. @clanmills added them to the test makefile as newtests

Copy link
Collaborator

Choose a reason for hiding this comment

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

They are not running, please check the travis CI jobs. Let's continue this discussion in slack

@D4N
Copy link
Member Author

D4N commented May 24, 2018

@D4N Should not we remove the tests from the bash test-suite? Otherwise we will have duplicated tests

I'd rather not, as we do not have the same test coverage yet. I would like to port all the issues first, then compute the test coverage, deactivate the bash tests and then recheck the test coverage.

@piponazo
Copy link
Collaborator

ah ok, I thought we could remove the tests that have been ported to python without having to disable all the bash tests.

@D4N D4N force-pushed the port_redmine_issues branch from 36cf236 to fae0d88 Compare May 24, 2018 08:36
@D4N D4N force-pushed the port_redmine_issues branch from fae0d88 to b8b94dc Compare May 24, 2018 09:06
@D4N D4N merged commit 89bd7d5 into master May 24, 2018
@D4N D4N deleted the port_redmine_issues branch May 24, 2018 10:37
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