Yelp / Testify

A more pythonic testing framework.
Other
306 stars 67 forks source link

Client mode return value #235

Closed asottile closed 10 years ago

asottile commented 10 years ago

In client mode, the HTTPReporter was always returning None for its return value on the report method leading to testify in the client mode always doing exit(1). This changes the reporter to invariantly return True.

bukzor commented 10 years ago

I'd be more convinced if we improved the subprocess acceptance tests to cover this case; it's not clear to me how that return value relates to the exit code.

This is probably Good Enough tho. milki?

ymilki commented 10 years ago

I'm on the same page as @bukzor as to the relation between this return value and the exit code. Since you are changing None to True, are you alway implying that client mode will always return a True value (or at least not exit(1))?

What behaviours are we trying to change that relies on this? Should we include a note in a changelog (which we don't see to have o.O) or readme about this change in behaviour?

asottile commented 10 years ago

In the current case it returns 0 if the normal test reporter and the http reporter return True.

The normal test reporter returns True if all the tests pass and there are non-zero tests.

I'm not sure the http reporter should have a say in return value as it doesn't actually know or care whether tests pass or fail, and is mainly used as an interface shim to get at test results.

bukzor commented 10 years ago

This fix is fine, but I'd want to improve the tests such that it can be fixed in a more reasonable way in the future, ie extend dnephin's acceptance tests to also cover client+server.

asottile commented 10 years ago

Done. Tests are updated to cover both success and failure for client-server.

bukzor commented 10 years ago

Looks good to me. It fails when you break it yes?

asottile commented 10 years ago

of course

bukzor commented 10 years ago

Waiting for milki to get a say.

ymilki commented 10 years ago

lgtm @asottile. Thanks for adding the tests!