Skip to content

Conversation

@wholmgren
Copy link
Member

  • 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.

I had been running into odd behavior with vscode no longer able to automatically discover pvlib tests and with strange errors when trying to run pytest from the root pvlib directory - sometimes I'd get numpy import errors in good environments and sometimes I'd see errors with old pvlib version numbers. But pytest pvlib worked just fine. It turns out that the presence of conda environments in benchmarks/ led to bad numpy imports and the presence of old pvlib versions in dist/ led to other issues. Adding these directories to the setup.cfg pytest section solved the issues, and I added a bunch of extra directories for safety.

@wholmgren wholmgren added this to the 0.9.0 milestone Jan 20, 2021
@wholmgren
Copy link
Member Author

@kanderso-nrel @mikofski can you merge this if you have no concerns with it? It feels like just a bit more than I should do without a second opinion.

@kandersolar kandersolar self-requested a review January 21, 2021 17:52
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.

If all of our tests are in one place (/pvlib/tests), would it make more sense to tell pytest where to look with testpaths (docs) instead of where not to look with norecursedirs?

Also the pytest docs recommend not using setup.cfg except for very simple cases. So if at some point in the future we find ourselves looking at this config section again, might be worth switching to one of the recommended options.

@wholmgren
Copy link
Member Author

If all of our tests are in one place (/pvlib/tests), would it make more sense to tell pytest where to look with testpaths (docs) instead of where not to look with norecursedirs?

that is more clear and I think it might have been faster to collect too.

Also the pytest docs recommend not using setup.cfg except for very simple cases. So if at some point in the future we find ourselves looking at this config section again, might be worth switching to one of the recommended options.

I saw that too. I'm not sure what qualifies as too complicated for setup.cfg. Maybe we could condense a few configuration files into a pyproject.toml file.

@kandersolar
Copy link
Member

I think the config in the most recent commit qualifies as simple. No objections from me to merging, but I don't have benchmark envs handy so I didn't reproduce the issue locally to confirm the fix works.

@wholmgren wholmgren changed the title add norecursedirs to setup.cfg pytest section add testpaths to setup.cfg pytest section Jan 23, 2021
@wholmgren wholmgren merged commit 1385943 into pvlib:master Jan 23, 2021
@wholmgren wholmgren deleted the vscodepytest branch January 23, 2021 19:51
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.

2 participants