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

Avoid fixtures in WorldwideOrganisation tests #2448

Closed

Conversation

chrisroos
Copy link
Contributor

I've updated the tests for WorldwideOrganisation to use data setup within the tests rather than data from the organisation fixtures in test/fixtures/worldwide/*.json.

I want to update the organisation fixture data to help with some changes I'm making to marriage-abroad. Updating that data at the moment causes a number of these WorldwideOrganisation tests to fail because the data they're relying on changes. The changes in this branch will allow me to update the organisation fixture data without having to also change these tests.

I want to be able to update the fixture data (it's also used by the regression
tests) without having to update these tests.

I've replaced the "finding organisations in a location" test with an
alternative that tests the `.for_location` method by stubbing the
`GdsApi::Worldwide#organisations_for_world_location` method. The previous
implementation used WebMock to stub the real API and relied on the behaviour of
`#organisations_for_world_location`.
I want to be able to update the fixture data (it's also used by the regression
tests) without having to update these tests.
I want to be able to update the fixture data (it's also used by the regression
tests) without having to update these tests.
I want to be able to update the fixture data (it's also used by the regression
tests) without having to update these tests.
I want to be able to update the fixture data (it's also used by the regression
tests) without having to update these tests.
I want to be able to update the fixture data (it's also used by the regression
tests) without having to update these tests.
It's no longer being used.
@chrisroos chrisroos force-pushed the avoid-fixtures-in-worldwide-organisation-tests branch from 4dbb076 to 8fe6be3 Compare April 14, 2016 01:41
@floehopper floehopper self-assigned this Apr 18, 2016
OpenStruct.new(title: 'organisation-1-title'),
OpenStruct.new(title: 'organisation-2-title')
]
worldwide_api = stub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I might be inclined to give this stub a name. It would make any error messages easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I might be inclined to give this stub a name. It would make any error messages easier to understand.

Addressed in e4a9921.

@floehopper
Copy link
Contributor

Minor: I think it might be good to explain why you chose to use instances of OpenStruct in the tests.

@floehopper
Copy link
Contributor

Other than my mostly minor comments, this looks good to me. 👍

@chrisroos
Copy link
Contributor Author

I'm closing this in order to rebase on master and address @floehopper's feedback.

@chrisroos chrisroos closed this Apr 19, 2016
@chrisroos chrisroos deleted the avoid-fixtures-in-worldwide-organisation-tests branch April 19, 2016 06:01
@chrisroos
Copy link
Contributor Author

chrisroos commented Apr 19, 2016

Minor: I think it might be good to explain why you chose to use instances of OpenStruct in the tests.

I've added the following paragraph to each of the commits in PR #2471 that use an OpenStruct in the tests:

I'm using an OpenStruct for the organisation data as that's what we get from
GdsApi::Worldwide#organisations_for_world_location, which is used to
instantiate WorldwideOrganisations in WorldwideOrganisation.for_location.

@chrisroos
Copy link
Contributor Author

chrisroos commented Apr 19, 2016

Thanks for the review, @floehopper. I've rebased this branch on master, addressed your comments and opened PR #2471 in preparation for merging to master.

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