Skip to content
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

Get clearsky.reno tests passing again #128

Merged
merged 3 commits into from Jan 27, 2022

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Jan 27, 2022

Description

The sphinx build in #127 is failing because of #126, and I figured might as well fix #105 while we're at it.

Checklist

  • Partially addresses Tests for clearsky.reno fail with pvlib 0.8.1 #105
  • Closes Error in clearsky.reno in newer pandas and numpy versions #126
  • [ ] Added new API functions to docs/api.rst
  • [ ] Clearly documented all new API functions with PEP257 and numpydoc compliant docstrings
  • Adds description and name entries in the appropriate "what's new" file
    in docs/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`).
  • [ ] Non-API functions clearly documented with docstrings or comments as necessary
  • [ ] Added tests to cover all new or modified code
  • Pull request is nearly complete and ready for detailed review

@kandersolar kandersolar added bug Something isn't working tests Something is wrong with the test suite labels Jan 27, 2022

@pytest.mark.skip(reason="GH #105")

@pytest.mark.skipif(is_old_pvlib, reason="GH #105")
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this sufficient for #105? It's not clear to me what the outcome of that issue was.

Copy link
Member

@cwhanse cwhanse Jan 27, 2022

Choose a reason for hiding this comment

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

I'm OK with marking this test to be skipped, as a way to get beyond #105, but I think #105 is still open. IIRC, the action here is to replace the expected result in the test. The test was created using pvlib v0.8.1, which had a bug corrected in v0.9.0 via 1242.

Copy link
Member Author

Choose a reason for hiding this comment

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

What test values are there to update? These tests don't seem to use particularly precise hardcoded values anywhere, unless I'm missing something.

An aside: the commit message on 09e93a0 will automatically close #105, and I'm not sure how to prevent that now, so we'll have to manually reopen that issue if this is merged.

Copy link
Member

Choose a reason for hiding this comment

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

This test fails with pvlib v0.8.1. Looking back at stuff, there's nothing to fix here; the fix was made in pvlib v0.9.0. So I now think skipping the test with old pvlib closes #105

@kandersolar kandersolar added this to the v0.2 milestone Jan 27, 2022
@kandersolar kandersolar merged commit 6fb4c87 into pvlib:master Jan 27, 2022
@kandersolar kandersolar deleted the reno_tests branch January 27, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tests Something is wrong with the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in clearsky.reno in newer pandas and numpy versions Tests for clearsky.reno fail with pvlib 0.8.1
2 participants