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.

Sign up for GitHub

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

Jump to bottom

TST: Clean warnings #1129

Merged
merged 22 commits into from
Jul 13, 2023
Merged

TST: Clean warnings #1129

merged 22 commits into from
Jul 13, 2023

Conversation

aburrell
Copy link
Member

@aburrell aburrell commented Jul 5, 2023 •
edited
Loading

Description

Addresses #1009 by adding a general test triggered by an optional testing variable to the standard set of Instrument tests. Adapted the clean method in the test instruments to run all potential iterations of the allowed behaviour, which includes:

  • logging messages
  • warnings
  • errors
  • changing the clean level

Updated the documentation to describe how this testing should be done.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Added unit tests. Also added this line to ace_epam.py in pysatSpaceWeather and ran the local unit tests.

# Set clean warning tests
_clean_warn = {'': {'': {'dusty': [('logger', 'WARN',
                         "unused clean level 'dusty', reverting to 'clean'",
                         'clean')]}}}

Test Configuration:

Checklist:

  • Make sure you are merging into the develop (not main) branch
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • Add a note to CHANGELOG.md, summarizing the changes

If this is a release PR, replace the first item of the above checklist with the release
checklist on the wiki: https://github.com/pysat/pysat/wiki/Checklist-for-Release

aburrell added 6 commits July 5, 2023 16:23
@aburrell
TST: added a test for warnings in clean
066a0e9 
Added a test for logging messages, warnings, and errors that may occur in Instrument `clean` methods.  Also tests to see if the clean level has been reset correctly.
@aburrell
TST: expanded test Instrument cleaning
71f6d9d 
Added level resetting, logging messages, warnings, and errors as potential outputs to the clean method for test instruments.  Also added a unit test for each of these elements.
@aburrell
DOC: updated new instrument instructions
6227fc9 
Updated the new instrument instructions for the clean method and `_clean_warn` test variable.
@aburrell
DOC: updated changelog
665ab42 
Updated the changelog with a description of this pull request.
@aburrell
STY: updated reference style
b84e005 
Updated the reference style in the docs.
@aburrell
BUG: fixing reference
67c1d8c 
Fixing bug in reference format.
@aburrell aburrell added this to the 3.2.0 Release milestone Jul 5, 2023
@aburrell aburrell linked an issue Jul 5, 2023 that may be closed by this pull request
Standards for Instruments without cleaning. #1009
Closed
@aburrell aburrell requested a review from rstoneback July 5, 2023 20:45
@aburrell aburrell mentioned this pull request Jul 5, 2023
Merged
10 tasks
@aburrell
BUG: fixed dict evalutation
ac2eb50 
Fixed logic in clean method, allowing key search only if input is a dict.
@aburrell
Copy link
Member Author

aburrell commented Jul 6, 2023

_clean_warn needs inst_id and tag incorporated.

aburrell
aburrell commented Jul 6, 2023
View reviewed changes
@aburrell
DOC: Update docs/new_instrument.rst
2b5ae2e 
Updated example for `_clean_warn` in docs to include inst_id and tag.
aburrell and others added 2 commits July 6, 2023 11:47
@aburrell
ENH: removed unreachable lines
56dadc7 
Removed lines that cannot be reached, as the clean method doesn't run if the clean level is 'none'.
@aburrell
MAINT: updated _clean_warn format
cd75aee 
Updated the `_clean_warn` format to accept a list of tuples.
This was referenced Jul 6, 2023
Open
Merged
@aburrell
DOC: improved documentation flow
cf273b5 
Moved the discussion of the new clean_warn test attribute to its own section and updated the reference.
aburrell
aburrell commented Jul 6, 2023
View reviewed changes
@aburrell
MAINT: added missing quotation
728153b 
Added a missing quotation mark.
@aburrell aburrell requested a review from jklenzing July 6, 2023 17:29
aburrell added 4 commits July 6, 2023 16:33
@aburrell
BUG: fixed dict access
150e691 
Fixed the dict access for the expected clean warnings.
@aburrell
BUG: handle strict time flag before messages
78e85aa 
Ensure the strict time flag is not raising a ValueError when testing the clean warning messages.
@aburrell
BUG: remove error from mult
0d2c069 
Cannot assess other warnings when an error is raised, only test multiple non-fatal warnings.
@aburrell
TST: improved error message
0c3798a 
Improved the logging error message in the clean warning test to be more useful.
jklenzing
jklenzing reviewed Jul 10, 2023
View reviewed changes
Copy link
Member

@jklenzing jklenzing left a comment

Choose a reason for hiding this comment

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

Looks good. I would suggest restructuring so that tests where nothing is run are skipped to be consistent with similar tests elsewhere.

@aburrell
MAINT: fixed spelling
c769ba8 
Fixed spelling and line length in the docs.
@aburrell
ENH: improved clean warning test
1eadc88 
Improved the clean warning test by:
- Creating a function for testing/setting the strict time flag,
- Removing 'none' from the clean level parametrization,
- Fixing the docstrings,
- Adding more comments, and
- Adding pytest skip statements.
@aburrell aburrell requested a review from jklenzing July 10, 2023 19:50
@aburrell
Copy link
Member Author

The decrease in coverage is consistent with tests not being skipped and edge cases for ecosystem packages not being triggered.

jklenzing
jklenzing reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@jklenzing jklenzing left a comment

Choose a reason for hiding this comment

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

The generalized function as written will cause a change in test_load

@aburrell @jklenzing
DOC: Update pysat/tests/classes/cls_instrument_library.py
35fdc51 
Fixed comments to be accurate.

Co-authored-by: Jeff Klenzing <[email protected]>
@aburrell aburrell requested a review from jklenzing July 11, 2023 16:25
@aburrell
TST: added clean_level input
3131341 
Allow the strict flag function to run with or without the clean method.
@aburrell aburrell requested review from jklenzing July 11, 2023 18:09
jklenzing
jklenzing requested changes Jul 12, 2023
View reviewed changes
aburrell and others added 2 commits July 12, 2023 13:26
@aburrell @jklenzing
STY: rename function
a356931 
Rename the test function as per discussion.

Co-authored-by: Jeff Klenzing <[email protected]>
@jklenzing
STY: pep8
3bacdef
@jklenzing jklenzing mentioned this pull request Jul 13, 2023
Merged
11 tasks
@aburrell aburrell merged commit 09673ae into develop Jul 13, 2023
@aburrell aburrell deleted the clean_warnings branch July 13, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklenzing jklenzing jklenzing approved these changes

@rstoneback rstoneback Awaiting requested review from rstoneback

Assignees
No one assigned
Labels
testing
Projects
None yet
Milestone
3.2.0 Release
Development

Successfully merging this pull request may close these issues.

Standards for Instruments without cleaning.
2 participants
@aburrell @jklenzing