TST: Move tests/scripts to scripts/tests #22531
Conversation
gfyoung
added
Refactor
gfyoung
force-pushed
the
scripts-own-tests
branch
from
412fbd9 to
3f10e75
Compare
Codecov Report
@@ Coverage Diff @@
## master #22531 +/- ##
=======================================
Coverage 92.04% 92.04%
=======================================
Files 169 169
Lines 50787 50787
=======================================
Hits 46745 46745
Misses 4042 4042
Continue to review full report at Codecov.
|
gfyoung
force-pushed
the
scripts-own-tests
branch
2 times, most recently
from
f75d9bb to
01bf8e5
Compare
gfyoung
changed the title
|
Relevant build: |
| import pytest | ||
| import string | ||
| import random |
There was a problem hiding this comment.
standard library imports should go before third-party module imports
There was a problem hiding this comment.
Indeed, they should! Done.
| import random | ||
| import numpy as np | ||
|
|
||
| validate_docstrings = pytest.importorskip("validate_docstrings") |
There was a problem hiding this comment.
I guess I'm missing something obvious, but why is this needed? can't we simply import validate_docstrings?
There was a problem hiding this comment.
That's vestigial paranoia from when we skipped tests if we couldn't see validate_docstrings. I agree that we can just import this without the protection.
| for i in range(length)) | ||
| length = random.randint(1, 10) | ||
| letters = "".join(random.choice(string.ascii_lowercase) | ||
| for _ in range(length)) |
There was a problem hiding this comment.
I know this comes from the original code, but we don't need a comprehension here, we can simply do ''.join(random.sample(string.ascii_lowercase, length))
.travis.yml
Outdated
| @@ -57,6 +57,9 @@ matrix: | |||
| - dist: trusty | |||
| env: | |||
| - JOB="3.6, coverage" ENV_FILE="ci/travis-36.yaml" TEST_ARGS="--skip-slow --skip-network" PANDAS_TESTING_MODE="deprecate" COVERAGE=true DOCTEST=true | |||
| - dist: trusty | |||
There was a problem hiding this comment.
i don't think we need another build
There was a problem hiding this comment.
you can just put on an existing build
There was a problem hiding this comment.
That's fair, and I agree we can do that. I don't have access to my computer ATM (which is why I haven't addressed the comments).
| @@ -0,0 +1,3 @@ | |||
| def pytest_addoption(parser): | |||
There was a problem hiding this comment.
As it says in the code: it's needed to placate setup.cfg.
--strict-data-files is enabled across the board for all tests, so we need to "accommodate" that option here.
|
Think we also need to update the Makefile to ensure this still gets linted (?) |
.travis.yml
Outdated
| @@ -57,6 +57,9 @@ matrix: | |||
| - dist: trusty | |||
| env: | |||
| - JOB="3.6, coverage" ENV_FILE="ci/travis-36.yaml" TEST_ARGS="--skip-slow --skip-network" PANDAS_TESTING_MODE="deprecate" COVERAGE=true DOCTEST=true | |||
| - dist: trusty | |||
There was a problem hiding this comment.
you can just put on an existing build
gfyoung
force-pushed
the
scripts-own-tests
branch
from
01bf8e5 to
2e2da73
Compare
@WillAyd : AFAICT we don't use the |
gfyoung
force-pushed
the
scripts-own-tests
branch
from
2e2da73 to
868f6bf
Compare
|
@jreback @datapythonista @WillAyd : I've addressed all of your comments, and everything is still green! PTAL. |
|
@jreback : Is this good to merge since you approved it? (you generally have merged after approval) |
|
yep |
Moves the
tests/scriptsdirectory toscripts/tests, as these tests don't really belong in the top-levelpandasdirectory (they test thescriptsdirectory, notpandas).