Refactor diary tests #1285

merged 6 commits into from Sep 15, 2016


None yet

3 participants


This PR refactors that diary entry and diary comment tests to remove the associated fixtures.

Note that you may need to remove the fixtures from your test database, since there's nothing in the rails code that removes unnecessary fixtures after their definitions are deleted. Either reload the test database completely, or run "delete from" via psql for diary_entries and diary_comments.

gravitystorm added some commits Sep 8, 2016
@gravitystorm gravitystorm Test DiaryComment body validation
Replaces unnecessary test.
@gravitystorm gravitystorm Change the display_name of suspended user fixuture
The original name is reserved, which means the user was invalid,
and this causes problems when building objects using it via
@gravitystorm gravitystorm Refactor tests to use factories instead of fixtures ea610c8
@gravitystorm gravitystorm Remove diary_entry and diary_comment fixtures, and refactor tests.
Note that you might need to empty the tables in your local test
database, since the removed fixtures may still linger there.
@gravitystorm gravitystorm Remove unneccessary fixture requirement.
@tomhughes tomhughes commented on an outdated diff Sep 14, 2016
def test_list_user
+ diary_entry = create(:diary_entry)
+ geo_entry = create(:diary_entry, :latitude => 51.50763, :longitude => -0.10781)
tomhughes Sep 14, 2016 Member

So the problem here is that by only creating those two entries we are no longer testing that the right posts get selected because there are no non-matching posts in the database. Much the same applies to the other list tests.


Ugh, implicit tests. Testing negatives are bad enough (just as likely to have an incorrect test as to be successfully testing that something is missing), but implicit ones are the worst. Due to the fixtures our tests are full of implicit negatives.

I'll fix these though, update soon.

In general, our tests have too high ratio of assertions to tests, so it's very hard to discern that e.g. "test_list_user" is supposed to both test that particular things show, and also that other things aren't. But it's nowhere near as bad as e.g. test_new, which crams 5 different tests (even needs comments to delineate them) and 40 assertions into one MegaTest(tm). But I'm trying to avoid biting off too much at once.

@gravitystorm gravitystorm Add additional list items to ensure they are not selected
The other list tests already have assetions showing the different
lengths of lists.
@tomhughes tomhughes merged commit 516979b into openstreetmap:master Sep 15, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.009%) to 89.757%
continuous-integration/travis-ci/pr The Travis CI build passed
gorn commented Sep 16, 2016

I am reasonably experienced rails dev, but I have never seed osmsite codebase. Therefore a question which is quite important to me is - how good/exhaustive are the tests right now? Even if I know the codebase I always start with enrighing the test to maximum extend BEFORE refactoring, because it sort of preserves the quality of the code brought in by years in wild. Basically at this point of development I test the added test by the codebase itself, if I beleive it its correctness :)

If you feel they are comprehensive enough, I would be much more inclined to jump in to refactoring, because it would feel less nervous about breaking any functionality I am not aware of. Of course I would start with syntactic stuff which theoretically should not change the behavior of code at all, but you know - that is a theory.


@gorn The tests are substantial - there are over 290,000 assertions in the test suite. So you can feel confident at getting started with developing.

@gravitystorm gravitystorm deleted the gravitystorm:diarytests branch Oct 19, 2016
@gravitystorm gravitystorm restored the gravitystorm:diarytests branch Oct 19, 2016
@gravitystorm gravitystorm deleted the gravitystorm:diarytests branch Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment