Skip to content

Get clearsky.reno tests passing again#128

Merged
kandersolar merged 3 commits into
pvlib:masterfrom
kandersolar:reno_tests
Jan 27, 2022
Merged

Get clearsky.reno tests passing again#128
kandersolar merged 3 commits into
pvlib:masterfrom
kandersolar:reno_tests

Conversation

@kandersolar

@kandersolar kandersolar commented Jan 27, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

@cwhanse cwhanse Jan 27, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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