Skip to content

Simplify pytest remote data mock setup#103

Merged
eteq merged 1 commit intoastropy:masterfrom
cdeil:simplify-pytest-mock
Sep 4, 2015
Merged

Simplify pytest remote data mock setup#103
eteq merged 1 commit intoastropy:masterfrom
cdeil:simplify-pytest-mock

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Sep 4, 2015

This is an attempt to simplify the pytest remote data mock setup that was introduced in #98.

Please see #98 (comment) and #98 (comment) as well as the
docstring of _mock_remote_data in this PR where I try to explain how it works and why it's an improvement.

@cdeil cdeil added this to the wishlist milestone Sep 4, 2015
@cdeil cdeil mentioned this pull request Sep 4, 2015
@cdeil cdeil force-pushed the simplify-pytest-mock branch from a0c5974 to dfb6fad Compare September 4, 2015 09:04
@cdeil cdeil changed the title Simplify pytest setup for mocked FixedTarget.from_name Simplify pytest remote data mock setup Sep 4, 2015
@cdeil cdeil modified the milestones: 0.1 release, wishlist Sep 4, 2015
@cdeil cdeil modified the milestones: 0.2 release, 0.1 release Sep 4, 2015
@cdeil
Copy link
Member Author

cdeil commented Sep 4, 2015

@eteq Can you please review this?

@eteq
Copy link
Member

eteq commented Sep 4, 2015

@cdeil - I'm not sure this actually solves the problem #98 was initially meant to fix. With this version, you always turn on mocking of --remote-data is off, and not if it's on. That means there's no way to test the mocked versions if remote_data is on.

My thinking is that that's a feature, not a bug. That is, it's better to write remote_data and non-remote_data tests as distinct and different entities. Then it's clearer what the origin of failures are: the actual remote_data behavior (i.e. downloads), or the other pieces that just use remote downloads incidentally.

To be more concrete, here we should have tests that actually test if from_name downloads the correct result, in addition to tests (mostly doctests) that use from_name just to make things easier. We want the latter to use the mock results regardless of remote_data status, and the former should never use mock data. The original implementation does that, but in this version, if you turn remote-data on and SIMBAD is down, from_name will yield tons of spurious failures, rather than just one. So I would advocate keeping pytest_runtest_setup as it was in #98.

The rest of it are good changes, though.

@eteq
Copy link
Member

eteq commented Sep 4, 2015

After a lengthy discussion on slack, @bmorris3 broke the tie and said we should switch to this behavior. So I'll merge this PR, and after this, --remote-data means from_name always goes to the internet, regardless of whether a test is marked remote_data or not.

eteq added a commit that referenced this pull request Sep 4, 2015
Simplify pytest remote data mock setup and change behavior to make --remote-data mean don't use mock anywhere
@eteq eteq merged commit 1018d7a into astropy:master Sep 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants