Skip to content

Fix 76#98

Merged
bmorris3 merged 8 commits intoastropy:masterfrom
eteq:fix-76
Sep 3, 2015
Merged

Fix 76#98
bmorris3 merged 8 commits intoastropy:masterfrom
eteq:fix-76

Conversation

@eteq
Copy link
Member

@eteq eteq commented Sep 3, 2015

This fixes #76 by adjusting the from_name mocking to turn on or off depending on whether a test is remote-data or not. I can confirm via the method of unplugging my ethernet cord that after this the remote_data tests fail if the internet is off.

@eteq eteq mentioned this pull request Sep 3, 2015
@eteq eteq added the testing label Sep 3, 2015
@eteq eteq added this to the 0.1 release milestone Sep 3, 2015
@cdeil
Copy link
Member

cdeil commented Sep 3, 2015

The mock is being called from docs/conf.py leading to this error:
https://travis-ci.org/astropy/astroplan/jobs/78631829#L649

See #96

@cdeil
Copy link
Member

cdeil commented Sep 3, 2015

There's also this fail!?
https://travis-ci.org/astropy/astroplan/jobs/78631828#L402

@eteq
Copy link
Member Author

eteq commented Sep 3, 2015

@cdeil - that second failure is a slightly hard-to-parse indicator that a warning isn't being raised that should be. Investigating now.

As for the second, thanks for the tip there - I didn't realize the docs used the mock... Will fix.

@cdeil
Copy link
Member

cdeil commented Sep 3, 2015

As I said in #96 ... I'm not actually sure any more the Sphinx build needs the mock ...

@eteq
Copy link
Member Author

eteq commented Sep 3, 2015

Ok, will check that and just remove if that's the case.

@eteq
Copy link
Member Author

eteq commented Sep 3, 2015

Turns out this is fixing a problem that I created in #90: it turns out that the way we're doing conftest.py, I was turning off a lot of the built-in astropy behavior. The latest commits turn it back on, and also fix a problem that had snuck in because this stuff was off.

So it's possible this will fail somewhere in some apparently unrelated place because it has substantial testing side-effects

But I think this now fixes #96 simply by not doing the mocking (it appears to not be necessary right now - I built the docs without warnings without an internet connection)

@eteq
Copy link
Member Author

eteq commented Sep 3, 2015

FYI, @jberlanga, as an aside: this is why I think you were seeing weird changes in the beahvior of summer_triangle.rst in #87 : between when you made it and when you rebased, the testing infrastructure (uintentionally) changed. So that's why I'm now skipping a lot more lines then you did there.

@cdeil
Copy link
Member

cdeil commented Sep 3, 2015

@eteq – I find this quite complicated to understand.

Could you please add a few sentences explaining the logic of what is happening in conftest.py, e.g. in a docstring at the top of the file?

Is it as simple as this?

  • Tests that use from_name should not be marked with @remote_data
  • If tests are run with --remote-data, the normal from_name is called which accesses the internet.
  • If tests are run without --remote-data, the mock from_name is called which which does not access the internet.

@eteq
Copy link
Member Author

eteq commented Sep 3, 2015

Alright, the tests passed one commit back, and the last commit adds an explanation as @cdeil requested, so this should be good to go.

To answer your sepcific quetsion here, @cdeil, the idea after this change is that something with from_name should have two tests: if remote_data is marked, then the test will run actually accessing the internet, but if it is not, then the mock will be used. So you'd write a remote_data test specifically to test the internet-based functionality (i.e. actually looking up some location that's not in the mocks), while the rest would be for when you just want from_name as a convenient way to create a FixedTarget or the like (especially for the docs we'll want this once the doc tests are up and running smoothly).

bmorris3 added a commit that referenced this pull request Sep 3, 2015
@bmorris3 bmorris3 merged commit f8c338c into astropy:master Sep 3, 2015
@eteq eteq deleted the fix-76 branch September 3, 2015 23:35
@cdeil
Copy link
Member

cdeil commented Sep 4, 2015

@eteq – Hmm.

In your setup you say that the test should be duplicated (once with, once without the @remote_data decorator), but the test that's of interest here astroplan/target.py::astroplan.target.FixedTarget.from_name, currently isn't duplicated.

I'll try to come up with a solution that doesn't require duplicating the test in #103.
If that doesn't work, I can duplicate the test there, so that the mock actually is tested.

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.

FixedTarget mock is active even when remote-data flag is used

3 participants