Merged
Conversation
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.
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.
Updated the new instrument instructions for the clean method and `_clean_warn` test variable.
Updated the changelog with a description of this pull request.
Updated the reference style in the docs.
Fixing bug in reference format.
10 tasks
Fixed logic in clean method, allowing key search only if input is a dict.
Member
Author
|
|
aburrell
commented
Jul 6, 2023
Updated example for `_clean_warn` in docs to include inst_id and tag.
aburrell
commented
Jul 6, 2023
Removed lines that cannot be reached, as the clean method doesn't run if the clean level is 'none'.
Updated the `_clean_warn` format to accept a list of tuples.
This was referenced Jul 6, 2023
Moved the discussion of the new clean_warn test attribute to its own section and updated the reference.
aburrell
commented
Jul 6, 2023
Added a missing quotation mark.
Fixed the dict access for the expected clean warnings.
Ensure the strict time flag is not raising a ValueError when testing the clean warning messages.
Cannot assess other warnings when an error is raised, only test multiple non-fatal warnings.
Improved the logging error message in the clean warning test to be more useful.
jklenzing
reviewed
Jul 10, 2023
Member
jklenzing
left a comment
There was a problem hiding this comment.
Looks good. I would suggest restructuring so that tests where nothing is run are skipped to be consistent with similar tests elsewhere.
Fixed spelling and line length in the docs.
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.
Member
Author
|
The decrease in coverage is consistent with tests not being skipped and edge cases for ecosystem packages not being triggered. |
jklenzing
reviewed
Jul 11, 2023
Member
jklenzing
left a comment
There was a problem hiding this comment.
The generalized function as written will cause a change in test_load
Fixed comments to be accurate. Co-authored-by: Jeff Klenzing <19592220+jklenzing@users.noreply.github.com>
jklenzing
reviewed
Jul 11, 2023
Allow the strict flag function to run with or without the clean method.
jklenzing
requested changes
Jul 12, 2023
Rename the test function as per discussion. Co-authored-by: Jeff Klenzing <19592220+jklenzing@users.noreply.github.com>
11 tasks
jklenzing
approved these changes
Jul 13, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
Updated the documentation to describe how this testing should be done.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added unit tests. Also added this line to
ace_epam.pyin pysatSpaceWeather and ran the local unit tests.Test Configuration:
Checklist:
develop(notmain) branchCHANGELOG.md, summarizing the changesIf 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