TST: Made s3 related tests mock boto
State: closed

Kept a couple tests un-mocked  for testing things like accessing a private bucket as that's hard to mock.

- [ ] closes #17325 
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: CI
description: Continuous Integration

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=h1) Report
> Merging [#17388](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/fa557f7391589f351b1260f46b3b3db22492f50b?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17388/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17388      +/-   ##
==========================================
+ Coverage   91.18%    91.2%   +0.01%     
==========================================
  Files         163      163              
  Lines       49545    49545              
==========================================
+ Hits        45179    45188       +9     
+ Misses       4366     4357       -9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.98% <ø> (+0.03%)` | :arrow_up: |
| #single | `40.21% <ø> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.77% <0%> (-0.1%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.43% <0%> (-0.1%)` | :arrow_down: |
| [pandas/io/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=tree#diff-cGFuZGFzL2lvL2NvbW1vbi5weQ==) | `68.64% <0%> (+0.84%)` | :arrow_up: |
| [pandas/io/s3.py](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=tree#diff-cGFuZGFzL2lvL3MzLnB5) | `85% <0%> (+85%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=footer). Last update [fa557f7...8f149d8](https://codecov.io/gh/pandas-dev/pandas/pull/17388?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> why are you adding tips files (as .csv) that already exist elsewhere?

I don't think we have compressed versions of those in the repo.
> do not add moto to EVERY ci file, this completely defeats the purpose. only add where s3fs is atm. we specifically have certain types of tests marked as disabled. you need to handle moto NOT being installed (via a pytest.importorskip..). on windows only add on a single build, ok with adding the pip install but only add for the 3.6 build.

@jreback what is the purpose I've defeated if I may ask? I suppose I can understand skipping some tests based on s3fs not being installed to test that pandas can handle import errors / can run without optional dependencies or something like that, but installing different _test_ dependencies per build is unnecessary and potentially dangerous. It also makes the build process needlessly complicated. I spent an extensive amount of time reading the build scripts and studying the failed build output to figure out which requirements files were being read in each build. I was finally frustrated enough that I just added it to every .pip file so at least all the environments would have that test-only dependency. Most of my time was spent waiting for the build since it took about 25 minutes or so to witness a failure.

Skipping tests based on test-only dependencies can also be dangerous as you could implement a test that has a skip, and never realize the test would actually fail when it should have due to a bug the test would have otherwise found. 48 requirements files seems a bit excessive. Skipping based on test-only dependencies is a bad idea, IMHO.
Part of the reason for the complicated requirements files is the need to support multiple versions of libraries like numpy, matplotlib, etc. We'll need one file per environment for them.

For optional deps though, it may be simpler to install them in every environment, especially test-only deps like moto or mock.

I like how dask handles checking for accidental use of optional dependencies [here](https://github.com/dask/dask/blob/d32e8b7b91e130037701daffabf663d8f1bae5de/continuous_integration/travis/test_imports.sh)
@TomAugspurger dask's handling of that is nice! Good call on the different versions; that makes sense.
I'm still fairly new to the github interface for pull requests. Is there a proper protocol to notify @jreback that I believe the requested changes have been dealt with?
Hello @kirkhansen! Thanks for updating the PR.





Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on September 14, 2017 at 02:26 Hours UTC
@kirkhansen with regards to https://github.com/pandas-dev/pandas/pull/17388#issuecomment-326469063

we have *many* builds and have a fairly comprehensive test infrastructure in order to support many goals.

- backcompat testing of specific versions
- slow/fast testing
- having builds not take excessive time
- easy interpretation of failing results

so we don't need every dependency running on every single build. sure the vast majority of them do, but the io sub-systems in particular are somewhat segregated to specific builds.

Imagine you are a contributor diagnosing failures, having *some* builds succeed, while having failures isolated to a single or small number of builds is generally a good thing.

testing everything on every build is nice to do in a smaller project, but becomes infeasible in larger ones.

finally not having a test mock in every build forces the test writer care of mocking properly (e.g. making sure imports are satisfied and such, making testing more robust).
> so we don't need every dependency running on every single build.

That's true for some dependencies, but I think docs and test-only dependencies should always be installed:

- pytest
- pytest-xdist
- pytest-cov
- flake8
- sphinx
- nbsphinx
- moto

> finally not having a test mock in every build forces the test writer care of mocking properly

How so? The behavior of

- `ImportError` if `s3fs` isn't installed
- succeed if it is

is the same, regardless of whether moto is installed or not.
ok I'll buy that we should make ``moto`` a standard test install. so remove ``moto`` from *all* .pip/.build files, and simply add where we install ``pytest`` (for both windows and linux), then it will always be available. would be ok removing the import skips as well. pls add to the ``ci\requirements_dev.txt`` though
> so remove moto from all .pip/.build files, and simply add where we install pytest

It appears pytest is being installed in the `install_travis.sh`, `install_circle.sh`, and `appveyor.yml` via conda. It also appears that the conda-forge channel is only added to some tests causing lots of build fails. I assume there is good reason to not include the conda-forge channel on all builds. How would you like me to proceed, @jreback? 
> It appears pytest is being installed in the install_travis.sh, install_circle.sh, and appveyor.yml via conda. It also appears that the conda-forge channel is only added to some tests causing lots of build fails. I assume there is good reason to not include the conda-forge channel on all builds. How would you like me to proceed, @jreback?

dont' install the conda version, just pip install, its right under the conda line (e.g. we pip install pytest-xdist)
thanks @kirkhansen nice PR!
Thanks Kirk!

On Thu, Sep 14, 2017 at 5:15 AM, Jeff Reback <notifications@github.com>
wrote:

> thanks @kirkhansen <https://github.com/kirkhansen> nice PR!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/17388#issuecomment-329438438>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIvN6bbTTSr_TXApF5fDpRpRMGjeFks5siPyqgaJpZM4PIPA_>
> .
>

my pleasure!