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

Added tests with additional clean up. #58

Closed
wants to merge 3 commits into from

Conversation

pwoodworth
Copy link
Contributor

This includes the tests from https://github.com/turn/ttorrent/pull/46, cleaned up to minimize unnecessary changes to existing code, and reformat the test code to match existing project code style.

@mpetazzoni
Copy link
Owner

Hi guys,

Thanks for this amazing work. Any chance I could get a clean branch to merge in that doesn't show the addition+removal of IDEA stuff and that collapses the code style changes into the commit adding the tests? I'd like to keep the history as clean as possible before merging.

Once we have a clean commit history I'll make my comments on the actual code.

Thanks!
/Max

@pwoodworth
Copy link
Contributor Author

I'd be fine with Sergey collapsing my cleanup into his own branch, but I'm hesitant to squash his code into my own commit (credit where credit is due). To effect the best compromise I can, I've re-pushed my branch so the PR includes only two commits instead of four. The commits related to adding and removing the IntelliJ project files are gone. Now we only have Sergey's commit adding the tests, and a second commit from me with a little cleanup.

@pwoodworth
Copy link
Contributor Author

If it helps mitigate the annoyance of merging this with the two commits instead of collapsed into one, I have a fix for issue https://github.com/turn/ttorrent/issues/42 ready just waiting on these tests to go in first.

@mpetazzoni
Copy link
Owner

Thanks for working on this. Now that the commit history is cleaner we can focus on the code itself. See my comments on the commit.

* commit 'ee7538e03a76dbacb5599ed19810ae9b99605f13':
  Added tests and slightly updated code for convenience
@pwoodworth
Copy link
Contributor Author

I've repushed again with a clean commit history, this time including merge-out from master resolving conflicts. I've also addressed your comments (that unfortunately seem to have been lost due to push --force for clean history). Specifically I've

  1. Eliminated FileUtil in favor of the commons-io FileUtils.
  2. Set the tests tracker port to be configurable by system property.
  3. Cleaned up the wrongness in WaitFor

@pwoodworth pwoodworth closed this Aug 28, 2013
@pwoodworth pwoodworth deleted the addTests branch August 28, 2013 23:22
This was referenced Aug 28, 2013
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