Daniel O'Connor [2009-01-28 23:30 UTC] Tests:
With the unit tests; is there any way you can mock out large chunks of it so you don't have to setup a configuration file?
For instance,
http://devel.2bopen.org/cgi-bin/trac/pear/browser/trunk/Net_OpenSSH/Net/OpenSSH.php#L268
proc_open creates streams for you to write to, and there's no way for the unit tests to replace that with something else (say, a php in memory stream) - think about ways to mock that out; and you can simulate a lot of behaviour / control your test environment.
You might also be interested in PHPUnit's data provider features; which allow you to write 1x test and supply many assorted types of input.
Also the tests are often a good way to demonstrate how the code is used - the way you've written them doesn't quite do that (though it does provide coverage)
Finally Consider naming your tests in an agile fashion (http://www.phpunit.de/manual/3.1/en/other-uses-for-tests.html)
OpenSSH
Code:
Might be worth swapping around some of the places where you do:
function foo() {
if (condition) {
... large part of function ...
return true;
} else {
throw new Exception("BLEHHHHH!");
}
}
to be:
function foo() {
if (!condition) {
throw new Exception("BLEHHHHH!");
}
// ... large part of function ...
return true;
}
Other than that, LGTM
Kevin van Zonneveld [2009-01-30 10:47 UTC] You don't have to make system calls to execute remote commands with ssh. You can check-for & use libssh instead.
http://kevin.vanzonneveld.net/techblog/article/make_ssh_connections_with_php/
Works for scp as well.
Christian Weiske [2009-06-25 08:07 UTC] Is this proposal dead, or are you going to finish it?
Chuck Burgess [2009-09-08 22:27 UTC] I can't download the tarball or see the PHPS files online... 2bopen.org is prompting for a username/password...
Chuck Burgess [2009-09-09 15:19 UTC] This looks pretty good. I built something very similar to the OpenSSH piece at work last year, but talked myself out of proposing it since it was mainly all shell exec stuff ;-) I could probably help you maintain this package if it's accepted.
Matthew Fonda [2009-09-09 18:10 UTC] Good looks great, nice work. I'd love to see this make its way into pear
Till Klampaeckel [2009-09-09 19:45 UTC] Hey Luca,
first off, I'm too looking forward to this code in PEAR.
I also briefly looked at your code and here are some suggestions:
* Move constants into the Net_SSH2 class.
* & is not necessary in PHP5.
* Always work with visibility (public, static, etc.) on methods (e.g. the factory).
* Add optional dependency on PEAR::System and ext/proc to package.xml. (PEAR::File should be optional as well).
* Maybe briefly test for File and System, before use (maybe in __construct()). Or introduce a small autoload, or require_once the files in between file and class comment.
* Maybe wrap the options into a Net_SSH2::$data array, instead of $this->$key = $val, $this->data[$key] = $val (in the drivers __construct()).
* In Net_SSH2_LibSSH2 you could support the .dll version (on Windows), you could also attempt loading the php_ssh2.dll/ssh2.so before you error out.
* Document thrown Exceptions with @throws.
* Instead of die() in your tests, please use $this->markTestIncomplete() or $this->markTestSkipped().
* Also, for some tests, take a look at $this->setExpectedException().
Anyway, your code is great regardless of what I find. Especially nice that you put a lot of effort into your test suite. :)
Cheers,
Till
|
