Skip to content

convert test dirs to packages #1204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 31, 2021
Merged

convert test dirs to packages #1204

merged 3 commits into from
Mar 31, 2021

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Mar 31, 2021

  • Closes #xxxx
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

@mikofski and I have been trying to figure out why vscode doesn't discover pvlib's test suite in microsoft/vscode-python#15398

This PR:

  • adds __init__.py files to the test directories
  • changes imports of test helpers (e.g. DATA_DIR) to use relative imports

Current pytest documentation says that you should not import from conftest.py unless you are using packages, so this PR brings us into alignment with that directive. My memory is that pytest used to discourage __init__.py in test directories, but maybe I misunderstood or I am misremembering the whole thing. The documentation example for including tests within the package (i.e. pvlib/tests rather than src/pvlib, tests) shows including an __init__.py file within the test directory.

@mikofski does this also fix the problem for you? You might need to pip uninstall pvlib; pip install -e . and you might need to reload vscode - I didn't carefully track what was important as I banged away on my keyboard.

@mikofski
Copy link
Member

Yes! Success! vscode is working again! No changes required, just checking out your branch seems to have fixed it. I have vanilla vscode settings:

{
    "python.testing.pytestArgs": [],
    "python.testing.unittestEnabled": false,
    "python.testing.nosetestsEnabled": false,
    "python.testing.pytestEnabled": true,
    "python.pythonPath": "venv\\Scripts\\python.exe"
}

based on a pip editable install into a virtual environment.

Nope, I didn't have to reinstall or change anything. Thanks so much for figuring this out. I thought I had run through all these options, and Brett Cannon couldn't figure it out either. I really appreciate the time you put into this.

Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, all vscode tests passing, everything looks good, thanks for moving location/solpos tests up to conftests and for cleaning up imports.

@mikofski
Copy link
Member

My memory is that pytest used to discourage __init__.py in test directories

I remember reading that too, I dunno what the logic was, but seems like since we're using conftest.py better for tests to be a subpackage, and doesn't hurt.

everything looks good to me, does anyone object if I merge it?

@wholmgren
Copy link
Member Author

Probably worth a whats new note. incoming...

@wholmgren wholmgren added this to the 0.9.0 milestone Mar 31, 2021
@mikofski
Copy link
Member

closed microsoft/vscode-python#15398

@cwhanse
Copy link
Member

cwhanse commented Mar 31, 2021

Packages and subpackages are somewhat mysterious to me, so I can't comment on using or not using them. +1 to moving those few fixtures into conftest.py

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok with me. Maybe I should be using vscode too!

times = pd.date_range(datetime.datetime(2003, 10, 17, 13, 30, 30),
periods=1, freq='D', tz=golden.tz)
ephem_data = solarposition.get_solarposition(times, golden.latitude,
golden.longitude,
altitude=altitude,
temperature=11)
if isinstance(expected, str) and expected == 'expected_solpos':
expected = expected_solpos
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish pytest could parametrize across fixtures directly instead of having to do this sort of tapdance. FYI in cases with more than one fixture lookup like this, using request and request.getfixturevalue(request.param) can be handy: https://stackoverflow.com/a/42599627

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks I didn't know about that!

@wholmgren wholmgren merged commit 709daa6 into pvlib:master Mar 31, 2021
@wholmgren wholmgren deleted the testinit branch March 31, 2021 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants