-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
Yes! Success! vscode is working again! No changes required, just checking out your branch seems to have fixed it. I have vanilla vscode settings:
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. |
There was a problem hiding this 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.
I remember reading that too, I dunno what the logic was, but seems like since we're using everything looks good to me, does anyone object if I merge it? |
Probably worth a whats new note. incoming... |
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 |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
Closes #xxxxTests addedUpdates entries todocs/sphinx/source/api.rst
for API changes.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`
).@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:
__init__.py
files to the test directoriesDATA_DIR
) to use relative importsCurrent 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 thansrc/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.