BLD: Install scripts tests only during inplace #22413

gfyoung · 2018-08-19T00:31:45Z

The scripts directory is not visible if the installation is not inplace.

Follow-up to #20061.

WillAyd · 2018-08-19T00:35:06Z

Would the other option be to just skip these tests if the scripts folder isn't included? I actually wonder if that would be preferable especially in cases where users install a binary and try to run pd.test() in the REPL

gfyoung · 2018-08-19T00:36:12Z

@WillAyd : I don't see why we should be packaging these tests when we do a release of pandas to the general public (the scripts directory won't be present at all). That's another reason why I would prefer exclusion vs. just skipping tests.

jreback

shouldn’t you just put a skip in the tests themselves?

gfyoung · 2018-08-19T00:52:32Z

@jreback : See my answer above. The only time these tests should be running is if we do an inplace install, since that's when the scripts directory will be visible. Otherwise, why should we be packaging those tests at all in the distribution (e.g. the public releases)?

codecov · 2018-08-19T01:27:52Z

Codecov Report

Merging #22413 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #22413   +/-   ##
=======================================
  Coverage   92.05%   92.05%           
=======================================
  Files         169      169           
  Lines       50733    50733           
=======================================
  Hits        46702    46702           
  Misses       4031     4031

Flag	Coverage Δ
#multiple	`90.46% <ø> (ø)`	⬆️
#single	`42.24% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f11d1a...c14c540. Read the comment docs.

jorisvandenbossche · 2018-08-20T09:57:50Z

Would the other option be to just skip these tests if the scripts folder isn't included?

+1

jorisvandenbossche · 2018-08-20T10:00:36Z

@gfyoung I understand your argument, but I think the added complexity (I would expect to find logic about whether the tests are ran/skipped in the tests itself, our setup.py is already very complex) is not worth the few kb's for those tests.

jreback · 2018-08-20T10:19:15Z

agree with @jorisvandenbossche here, pls simply skip these test. We don't want multiple paths for doing things like skips.

gfyoung · 2018-08-20T13:12:42Z

Fair enough. IMHO, these validation tests should not have been located in the pandas/tests directory, but oh well. 🤷‍♂️

datapythonista · 2018-08-20T13:18:29Z

Not sure what it involves in the CI configuration, but I'm +1 on moving the tests for the scripts to scripts/tests too.

gfyoung · 2018-08-20T14:49:22Z

@jreback @jorisvandenbossche : Thoughts on moving the tests? As much as I know both of you don't want the extra complexity in setup.py of "skipping" the copying of certain files during distribution, I am also not a fan of seeing tests fail locally just because the scripts isn't packaged.

We can skip the tests for now, but the long-term solution I think is just moving them elsewhere.

jorisvandenbossche · 2018-08-20T15:05:19Z

I am also not a fan of seeing tests fail locally just because the scripts isn't packaged.

They wouldn't fail, the idea is that they would be skipped.

That said, I am also fine with having the tests live in scripts/tests/, if that doesn't complicate the travis set-up too much to also run those (I suppose now we everywhere only test pandas` directory?)

WillAyd · 2018-08-20T17:12:36Z

I'm -1 on moving to scripts/tests. If we are worried about the added size of the new module then we are making micro-optimizations whose benefit would pale in comparison to the effort making sure that we can account for tests that live outside of the tests directory

gfyoung · 2018-08-20T17:51:33Z

whose benefit would pale in comparison to the effort making sure that we can account for tests that live outside

@WillAyd : I'm not sure we can make this assertion yet without giving it a shot first. 😉

jreback · 2018-08-20T22:54:51Z

pandas/tests/scripts/test_validate_docstrings.py

+        Import the validation scripts from the scripts directory.
+
+        Because the scripts directory is above the top level pandas package,
+        we need to hack `sys.path` to know where to find it for import.


avoid the term 'hack'

That word was from the original docstring, but sure thing.

jreback · 2018-08-20T22:55:17Z

pandas/tests/scripts/test_validate_docstrings.py

+            globals()[global_validate_one] = validate_one
+        except ImportError:
+            # Import will fail if the pandas installation is not inplace.
+            raise pytest.skip("scripts directory does not exist")


can you say pandas/scripts

If the pandas is not inplace, the scripts directory will not exist, and the tests will fail. Follow-up to pandas-devgh-20061.

WillAyd · 2018-08-21T02:06:33Z

pandas/tests/scripts/test_validate_docstrings.py

+            globals()[global_validate_one] = validate_one
+        except ImportError:
+            # Import will fail if the pandas installation is not inplace.
+            raise pytest.skip("pandas/scripts directory does not exist")


Not sure if this causes an issue but from what I've tested locally if using the imperative skip validate_docstrings stays in globals as does the modification to system path. Perhaps we should ensure that code gets executed in the except block here?

Fair point regarding system path modification.

Not sure how validate_docstrings stays in globals if the import fails in the first place...

@WillAyd : Addressed the change in follow-up (#22456)

jreback · 2018-08-21T10:14:38Z

let's see if this fixes the wheels

jreback · 2018-08-21T11:57:34Z

ok wheels fixed: https://travis-ci.org/MacPython/pandas-wheels/builds/418513881

Follow-up to pandas-devgh-22413.

Follow-up to gh-22413.

If the pandas is not inplace, the scripts directory will not exist, and the tests will fail. Follow-up to pandas-devgh-20061.

Follow-up to pandas-devgh-22413.

gfyoung added Bug Build Library building on various platforms labels Aug 19, 2018

gfyoung added this to the 0.24.0 milestone Aug 19, 2018

jreback reviewed Aug 19, 2018

View reviewed changes

jorisvandenbossche mentioned this pull request Aug 20, 2018

Add tests for docstring Validation Script + py27 compat #20061

Merged

4 tasks

gfyoung force-pushed the install-scripts-tests branch from 189c585 to 9f2ad74 Compare August 20, 2018 18:52

jreback requested changes Aug 20, 2018

View reviewed changes

TST: Skip scripts test if scripts doesn't exist

c14c540

If the pandas is not inplace, the scripts directory will not exist, and the tests will fail. Follow-up to pandas-devgh-20061.

gfyoung force-pushed the install-scripts-tests branch 2 times, most recently from 96ec3d7 to c14c540 Compare August 20, 2018 23:29

WillAyd reviewed Aug 21, 2018

View reviewed changes

jreback approved these changes Aug 21, 2018

View reviewed changes

jreback merged commit 9c35865 into pandas-dev:master Aug 21, 2018

gfyoung deleted the install-scripts-tests branch August 22, 2018 05:56

gfyoung added a commit to forking-repos/pandas that referenced this pull request Aug 22, 2018

MAINT: Reset sys.path if docstrings import fails

f0b0f38

Follow-up to pandas-devgh-22413.

gfyoung mentioned this pull request Aug 22, 2018

MAINT: Reset sys.path if docstrings import fails #22456

Merged

gfyoung added a commit to forking-repos/pandas that referenced this pull request Aug 22, 2018

MAINT: Reset sys.path if docstrings import fails

c7d0f3c

Follow-up to pandas-devgh-22413.

jreback pushed a commit that referenced this pull request Aug 22, 2018

MAINT: Reset sys.path if docstrings import fails (#22456)

302643d

Follow-up to gh-22413.

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

TST: Skip scripts test if scripts doesn't exist (pandas-dev#22413)

a379e0b

If the pandas is not inplace, the scripts directory will not exist, and the tests will fail. Follow-up to pandas-devgh-20061.

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

MAINT: Reset sys.path if docstrings import fails (pandas-dev#22456)

f8d2a76

Follow-up to pandas-devgh-22413.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLD: Install scripts tests only during inplace #22413

BLD: Install scripts tests only during inplace #22413

gfyoung commented Aug 19, 2018 •

edited

Loading

WillAyd commented Aug 19, 2018

gfyoung commented Aug 19, 2018 •

edited

Loading

jreback left a comment

gfyoung commented Aug 19, 2018 •

edited

Loading

codecov bot commented Aug 19, 2018 •

edited

Loading

jorisvandenbossche commented Aug 20, 2018

jorisvandenbossche commented Aug 20, 2018

jreback commented Aug 20, 2018

gfyoung commented Aug 20, 2018

datapythonista commented Aug 20, 2018

gfyoung commented Aug 20, 2018 •

edited

Loading

jorisvandenbossche commented Aug 20, 2018

WillAyd commented Aug 20, 2018

gfyoung commented Aug 20, 2018

jreback Aug 20, 2018

gfyoung Aug 20, 2018

jreback Aug 20, 2018

gfyoung Aug 20, 2018

WillAyd Aug 21, 2018

gfyoung Aug 21, 2018 •

edited

Loading

gfyoung Aug 22, 2018

jreback commented Aug 21, 2018

jreback commented Aug 21, 2018

BLD: Install scripts tests only during inplace #22413

BLD: Install scripts tests only during inplace #22413

Conversation

gfyoung commented Aug 19, 2018 • edited Loading

WillAyd commented Aug 19, 2018

gfyoung commented Aug 19, 2018 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

gfyoung commented Aug 19, 2018 • edited Loading

codecov bot commented Aug 19, 2018 • edited Loading

Codecov Report

jorisvandenbossche commented Aug 20, 2018

jorisvandenbossche commented Aug 20, 2018

jreback commented Aug 20, 2018

gfyoung commented Aug 20, 2018

datapythonista commented Aug 20, 2018

gfyoung commented Aug 20, 2018 • edited Loading

jorisvandenbossche commented Aug 20, 2018

WillAyd commented Aug 20, 2018

gfyoung commented Aug 20, 2018

jreback Aug 20, 2018

Choose a reason for hiding this comment

gfyoung Aug 20, 2018

Choose a reason for hiding this comment

jreback Aug 20, 2018

Choose a reason for hiding this comment

gfyoung Aug 20, 2018

Choose a reason for hiding this comment

WillAyd Aug 21, 2018

Choose a reason for hiding this comment

gfyoung Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

gfyoung Aug 22, 2018

Choose a reason for hiding this comment

jreback commented Aug 21, 2018

jreback commented Aug 21, 2018

gfyoung commented Aug 19, 2018 •

edited

Loading

gfyoung commented Aug 19, 2018 •

edited

Loading

gfyoung commented Aug 19, 2018 •

edited

Loading

codecov bot commented Aug 19, 2018 •

edited

Loading

gfyoung commented Aug 20, 2018 •

edited

Loading

gfyoung Aug 21, 2018 •

edited

Loading