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 fixtures #1556

Merged
merged 20 commits into from Jun 1, 2017
Merged

Remove fixtures #1556

merged 20 commits into from Jun 1, 2017

Conversation

@gravitystorm
Copy link
Collaborator

@gravitystorm gravitystorm commented Jun 1, 2017

This PR removes the test fixtures, and associated helper methods. It also fixes some tests that were implicitly relying on the test fixtures being in the database.

gravitystorm added 19 commits May 31, 2017
These now include explicit tests for the changesets expected, as
well as refactoring so that they don't rely on fixtures.
This is no longer required, as the tests no longer use fixtures.
@@ -2036,37 +2046,46 @@ def test_list_max_id
##
# This should display the last 20 changesets closed
def test_feed
changeset = create(:changeset, :num_changes => 1)
_empty_changeset = create(:changeset, :num_changes => 0)

This comment has been minimized.

@tomhughes

tomhughes Jun 1, 2017
Member

I think we should create a changeset that is still open here, to make sure it doesn't get included in the feed result - that was (admittedly very non-obviously) being tested before,

This comment has been minimized.

@gravitystorm

gravitystorm Jun 1, 2017
Author Collaborator

I think this might be a misleading comment - the controller code doesn't appear to check for open vs closed. In fact the comment in the code suggests that it should only be /open/ changesets, which I think is a second misleading comment!

The changeset in the test here is open anyway, but I can add a closed one to test the behaviour.

Do you think I should update both comments to simply reference "changesets" rather than open or closed?

This comment has been minimized.

@tomhughes

tomhughes Jun 1, 2017
Member

Ah yes you're right... I misread the code in check_feed_result and thought it as filtering out open changesets.

This comment has been minimized.

@tomhughes

tomhughes Jun 1, 2017
Member

Updating the comments sounds good though as they are very misleading.

This comment has been minimized.

@gravitystorm

gravitystorm Jun 1, 2017
Author Collaborator

Done in 060230f

@tomhughes tomhughes merged commit 060230f into openstreetmap:master Jun 1, 2017
1 of 3 checks passed
1 of 3 checks passed
coverage/coveralls Coverage decreased (-0.3%) to 78.974%
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gravitystorm gravitystorm deleted the gravitystorm:remove-fixtures branch Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.