hotsh / rstat.us

Simple microblogging network based on the ostatus protocol.
http://rstat.us/
Other
722 stars 215 forks source link

Test cleanup #725

Closed carols10cents closed 11 years ago

carols10cents commented 11 years ago

These are things I found while removing unnecessary fabrications that I wanted to clean up but had nothing to do with that change. Some of these commits remove some tests that I don't think are worth keeping.

Before these changes:

$ time rake test:models
real    1m41.507s

After these changes:

$ time rake test:models
real    1m37.926s

Sooo meh. But yay getting rid of some code baggage?

wilkie commented 11 years ago

I'm not sure I can fully support the deletion of tests without the deletion of features. You haven't given too many details as to why you've made those decisions. Let's not delete tests just so its faster without proof that we have equivalent coverage/confidence. Instead of haphazardly fiddling with tests to see how much time we shave, we could actually profile them?

carols10cents commented 11 years ago

I went down this path motivated by making the tests faster, and in that sense removing these tests doesn't help, that's true.

But I basically ended up doing an audit of the unit tests, and tests are code, and I like deleting unnecessary code. If you think these tests are worth keeping around, though, I'm willing to be overruled :) They just feel like we're testing that rails validations work, and rails has its own tests for that.

wilkie commented 11 years ago

If the presence of the validators is tested so that assurance is made that invalid states cannot be reached, then they are useful. These tests may very well be providing that confidence, right? Are they provably unnecessary?

carols10cents commented 11 years ago

Resubmitting without the commits that remove the validations :)

carols10cents commented 11 years ago

Pulling rank and merging :)