skip known http failures

[marqh]

No description provided.

[ajdawson]

I think it would be better to take a decision on what to do about the broken functionality rather than hide the tests that fail due to it.

[marqh]

I think it would be better to take a decision on what to do about the broken functionality rather than hide the tests that fail due to it.

i agree, this is the wrong path. Unfortunately none of us has managed to find the right path yet and it is quite some time since these failures began appearing on all PRs

so, what course of action should we be looking at to deal with the SRTM HTTP failures?

the SRTM download site has access permissions set up
http://e4ftl01.cr.usgs.gov/SRTM/
http://e4ftl01.cr.usgs.gov/SRTM/SRTMGL30.002/

Downloading these data requires a NASA Earthdata Login username and password.
To obtain a NASA Earthdata Login account, please visit
https://urs.earthdata.nasa.gov/users/new/.

So, should we really be using this in this way?

I'm not sure that signing up with a magic cartopy account and baking that into the code base comes under fair usage

perhaps some nice folks with connections to USGS could help us out with a few clues or resources to investigate:
@rsignell-usgs
@ocefpaf
@dblodgett-usgs
Is there any help or advice you could offer us on this?

much obliged
mark

[dblodgett-usgs]

We had a project with NASA in the midst of implementation of the login requirement. We were told to get a system account to use for our users, but the system we were working with is a third-party processing capability (http://cida.usgs.gov/gdp/). I think that would be valid for your test purposes, but I'm not sure that's tenable if the tests are going to run "out in the open". As far as I know, there is no plan to implement an API key method rather than user login, but I do have some continued contact with NASA on this and will continue to push for it.

[ajdawson]

I'm not sure that signing up with a magic cartopy account and baking that into the code base comes under fair usage

This is not desirable either. If there is no other way than username/password authentication then maybe we just cannot offer this feature anymore.

[dblodgett-usgs]

Our system can pull in the user/pass at run time from local-system configuration and we don't have tests that reach out to such services. The only way to offer this kind of thing is to have your users go sign up for a URS account and pass their credentials in at run time.

[marqh]

thanks @dblodgett-usgs that is really helpful

I suspect that we will need to think about how to allow users to sign up for an account and use their own account to download as part of the API.
of course, testing this is a different challenge

this may link into aims to move from urllib to requests, with better support for such things as login and session
there would be API changes needed, so for now, I am suggesting that we remove the test cases and put a warning into the API that such downloads are likely to fail due to permissions

[ajdawson]

Might this be a case for a spin-out/plugin cartopy-srtm package? We could keep awkward stuff like this out of the core, which might be good for everyone.

[QuLogic]

A login method for users is available at #789. For testing, we (and by that, I mean someone at the Met Office) can create an account and encrypt the login details in a secure variable on Travis. It won't be possible to test locally without your own key, but it will unbreak CI.

[marqh]

@ajdawson i have added warnings to these two downloaders that highlight that access rights are required to use and removed the failing tests

do you think this is a resonable compromise for now?

I have created a new issue for each of srtm #819 and mapquest #820
in the hope that we can address these soon

[marqh]

A login method for users is available at #789. For testing, we (and by that, I mean someone at the Met Office) can create an account and encrypt the login details in a secure variable on Travis.

thank you for the information @QuLogic

I have looked at #789 and I am not convinced that this is the approach we should adopt within the library.

It won't be possible to test locally without your own key, but it will unbreak CI.

this concerns me, I'm wary of having test infrastructure which puts this level of requirement on developers.
I'd like to make some time to investigate this and look for a better solution. that is unlikely to be this week or next

At present, I'm really keen to unblock the CI so that PRs get sensible pass/fail reports from Travis and make it easy to run tests locally for developers.

I think that a cost of reducing test coverage in these cases is acceptable, if upsetting, as a temporary measure which I want to see reverted

[QuLogic]

I wouldn't assume that would be the actual API; it's more of a workaround at the moment in that it's a global setting. However, once we expose some sort of username/password combo, then that's the way to implement it (just use OpenerDirector.open instead of urlopen.)

[QuLogic]

Also, just because tests would require the key on Travis, doesn't mean that the local tests must run them. They could be disabled by default much like here except when some environment variable is set.

[marqh]

I wouldn't assume that would be the actual API; it's more of a workaround at the moment in that it's a global setting.

As I said, I am not convinced that this is a good work around at this time. I think more thought is required

However, once we expose some sort of username/password combo, then that's the way to implement it (just use OpenerDirector.open instead of urlopen.)

I am not convinced that this is the way to implement it; it wouldn't be my first choice. It may turn out to be a good way, but i'd like some time and space to investigate this; hard things to find just now

[QuLogic]

Any updates? I'd like tests to get moving again (though I'd prefer @skip instead of outright deletion here.)

[marqh]

Any updates? I'd like tests to get moving again

I want to get tests moving again, the current status is not good

(though I'd prefer @Skip instead of outright deletion here.)

I have changed lib/cartopy/tests/mpl/test_images.py, which is a composite image, so this doesn't fit a skip pattern

None of lib/cartopy/tests/io/test_srtm.py is currently functional. Are you suggesting that I put in a skip pattern for this whole module to enable this PR to proceed?

[QuLogic]

The advantage of a skip is that it's a constant, if rather subtle, reminder that something needs fixing. It's a pretty minor advantage though, so I could be convinced to just merge anyway.

[marqh]

The advantage of a skip is that it's a constant, if rather subtle, reminder that something needs fixing. It's a pretty minor advantage though, so I could be convinced to just merge anyway.

i've glanced into this and due to cartopy's hybrid testing strategy, mixing unittest and nose, there is not a clean skip pattern available (that I can see)

if it's required, this could be fixed somehow. the code is still in the history, ready for revival ,so it's not a great loss to delete it in this PR

@QuLogic please merge if you are content or request a skip pattern implementation if you feel it is required.
thank you

[ajdawson]

Skipping explicitly sounds like a good solution. Can you use the @unittest.skip decorator?

[marqh]

Skipping explicitly sounds like a good solution. Can you use the @unittest.skip decorator?

this does not work without significant refactoring of tests, due to the hybrid use of unittest and nose across cartopy's testing infrastructure

[ajdawson]

OK, will raising a nose nose.SkipTest exception do the trick?

[marqh]

OK, will raising a nose nose.SkipTest exception do the trick?

this also failed to deliver for me locally, not working nicely with the unittest instances

[QuLogic]

A change to skip seems to have worked above.

[marqh]

I have updated this proposal, putting the test_srtm module into valid unittest testcases and implementing a skip pattern. this now works on travis and for me locally

[QuLogic]

I'm not sure why the classes were necessary for you locally, but if it works...

[marqh]

I'm not sure why the classes were necessary for you locally, but if it works...

thank you @QuLogic
does this mean you are content to merge this change?