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

[PR196 0/3] Cleanup of integrated test harness, other small fixes #247

Merged
merged 11 commits into from
Dec 21, 2015

Conversation

jbohren
Copy link
Contributor

@jbohren jbohren commented Dec 15, 2015

This PR is a prerequisite to merging PR #196. The main changes in this PR include unifying the integrated test harnesses and adding numerous test cases. This PR also involves a few small additions, bugfixes, and cleanup. This PR is a prerequisite to the first segment of PR#196 (new execution pipeline).

The individual changes to the different components of catkin_tools are broken up into six atomic commits. Note that commit 8e2df65 only appears to be large due to re-indenting into a try-catch block. In the future, catkin_tools/commands/catkin.py could use some additional cleanup and refactoring.

@jbohren
Copy link
Contributor Author

jbohren commented Dec 15, 2015

@wjwwood Here are some pre-req changes before merging in the new execution pipeline. Note that the added tests will fail due to a limitation of the current job server. We could fix the bug, but that job server is being replaced almost completely by the new implementation in a forthcoming PR (in which all tests pass).

@wjwwood
Copy link
Member

wjwwood commented Dec 15, 2015

These changes look ok, I opened a pr against your branch with some things I noticed while reviewing.

There are several other functions which are probably too complicated at this point and should be refactored, but we can tackle them later.

Once we iterate on and merge my pr against your branch I think this will be ready for merge. In the future go ahead and push your pr branches to the main repository, that way I can just add commits to pr's.

…ixup

Jbohren forks pre 0.4.0 cleanup fixup
@jbohren
Copy link
Contributor Author

jbohren commented Dec 15, 2015

Ignore last comment, closed wrong PR.

@jbohren jbohren changed the title Cleanup of integrated test harness, other small fixes (PR#196 0/3) [PR196 0/3] Cleanup of integrated test harness, other small fixes Dec 18, 2015
@jbohren
Copy link
Contributor Author

jbohren commented Dec 18, 2015

@wjwwood Feel free to merge this if it looks good.

wjwwood added a commit that referenced this pull request Dec 21, 2015
[PR196 0/3] Cleanup of integrated test harness, other small fixes
@wjwwood wjwwood merged commit f303165 into catkin:master Dec 21, 2015
@jbohren jbohren deleted the pre-0.4.0-cleanup branch December 21, 2015 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants