mattnenterprise / rust-ftp

FTP client for Rust
Apache License 2.0
182 stars 57 forks source link

added simple upload test and fixed anonymous login #35

Closed matt2xu closed 8 years ago

mattnenterprise commented 8 years ago

It looks like the tests are currently failing. Also for most rust projects I have seen tests at the bottom of the same file as the code that it tests. For example: https://github.com/hyperium/hyper/blob/master/src/status.rs Let me know if you have found some other documentation that counters that claim.

dbrgn commented 8 years ago

Usually unit tests go into the source files directly and integration tests go into a separate test file. I think this one looks more like an integration style test.

matt2xu commented 8 years ago

Fixed the test and the code. The "login" method did not handle anonymous logins... Now fixed. Ideally another test should test normal login.

Also, as for where the tests should be placed, it depends. Some crates have tests at the end of source files, others put them in separate files. I have seen recently a project using a separate "tests" folder (similarly to putting benchmarks in the "benches" folder), I like that as it makes the tests easier to find/modify, so I've done the same in this pull request.

matt2xu commented 8 years ago

I just found out that the Cargo documentation states that integration tests should go into a "tests" folder: http://doc.crates.io/manifest.html#the-project-layout

matt2xu commented 8 years ago

@mattnenterprise may I suggest you merge this PR or #36 ? @workanator has also fixed the problem with parsing PASV response, it's a pity that we both fixed the same bug :-/

mattnenterprise commented 8 years ago

@matt2xu I merged in #36. I would still like to keep your test if you are willing to fix merge conflicts.

matt2xu commented 8 years ago

@mattnenterprise alright, sure I'll fix the conflicts tomorrow.

matt2xu commented 8 years ago

Ok I rebased it and fixed the conflicts.

mattnenterprise commented 8 years ago

Thanks!