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

Remove output from tests #37

Merged
merged 1 commit into from
Aug 4, 2014
Merged

Conversation

alexgenco
Copy link
Contributor

Tabber accepts an :output option, which points to $stdout by default.
Tests set this option to a StringIO object instead.

If you like the output for acceptance testing, feel free to close this. But a high-level test that makes explicit assertions about the format of the output would be better imo. In fact, I might just write that test :)

I'll rebase if #36 gets merged first and you like this change.

@marklarr
Copy link
Member

marklarr commented Aug 4, 2014

This is really cool. I merged #36 before I saw this one, so just let me know when this is ready to go and I'll merge it.

@marklarr
Copy link
Member

marklarr commented Aug 4, 2014

Actually, though, I usually try to avoid adding functionality to a class just to make it more testable. What's your opinion on that? Would it be possible to stub out the $stdout object as a StringIO instead, or otherwise suppress the output without adding functionality to the class?

@alexgenco
Copy link
Contributor Author

That's a fair objection. I personally don't see this as a test-only benefit though. At the very least, it mitigates the dependency on the global constant $stdout. It also makes a change like writing to a file instead of stdout trivial.

At the risk of getting too philosophical, I write implementation code to make classes more testable all the time. I think easily testable classes are often easier to maintain and change.

That being said, I could go either way on this. I don't think the benefit is really all that huge at this point, so I'll defer to your judgement. I'll rebase in a little bit so you can merge if you want.

@marklarr
Copy link
Member

marklarr commented Aug 4, 2014

Go ahead and I'll merge. I think you're right about writing it to a file, that sounds like it could be helpful. 👍

@alexgenco
Copy link
Contributor Author

Oops lemme look into these failures

Tabber accepts an :output option, which points to $stdout by default.
Tests set this option to a StringIO object instead.
@alexgenco
Copy link
Contributor Author

Sorry, bad rebase. Should be ready to go now. Thanks!

marklarr added a commit that referenced this pull request Aug 4, 2014
@marklarr marklarr merged commit 9d7052d into venmo:master Aug 4, 2014
@marklarr
Copy link
Member

marklarr commented Aug 4, 2014

🍻

@alexgenco alexgenco deleted the remove_test_output branch August 4, 2014 23:01
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.

2 participants