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

Unit tests for all Flowcb/Accept plugins #757

Merged
merged 39 commits into from
Sep 7, 2023
Merged

Unit tests for all Flowcb/Accept plugins #757

merged 39 commits into from
Sep 7, 2023

Conversation

gcglinton
Copy link
Contributor

This is a full test suite for all 23 FlowCB Accept plugins.

  • Tests have 100% coverage
    • It might be overkill, but the plugins were (mostly) simple, and getting perfect coverage wasn't much harder than 90+
  • Some asserts are less than ideal (checking the number of log messages), as changes to other parts of the code could have negative effects
    • Those asserts could be probably be removed, as I'm generally also checking that a particular message was logged, which is more telling
  • Small config change to pytest, in order to ensure that only classes prefixed with Test_ are treated as test classes (default is just Test)

Notable innovations with these tests

  • Used Paramaterized tests in with wmotypesuffix_test.py
    • Very simple parameters made this a no-brainer, without having to do a for loop inside the test
    • Other tests might have similar use-cases, so good to have the patter defined
  • More mocking examples, mostly with time, and random, but also one with urllib
    • Really handy/important to tightly control the test environment, and get be able to assert on known values
  • Extensive use of existing SR3 classes (config, and message), in order to properly build tests
    • It does mean these tests depend on those classes operating properly, but it should be fairly easy to eventually have dependencies set if that was wanted/needed

There's also a couple of late fixes to the actual plugins here, which are required to make them operate as designed

gcglinton and others added 30 commits July 28, 2023 00:13
THey were broken because of the 'set_notice' header being wrong
flowcb/accept/postoverride
flowcb/accept/tohttp
flowcb/accept/wmotypesuffix
That item was removed as part of writing unit tests
THey were broken because of the 'set_notice' header being wrong
flowcb/accept/postoverride
flowcb/accept/tohttp
flowcb/accept/wmotypesuffix
That item was removed as part of writing unit tests
THey were broken because of the 'set_notice' header being wrong
flowcb/accept/postoverride
flowcb/accept/tohttp
flowcb/accept/wmotypesuffix
That item was removed as part of writing unit tests
That item was removed as part of writing unit tests
downloadbaseurl
renamedmf
tolocalfile

All with 100% coverage
THey were broken because of the 'set_notice' header being wrong
flowcb/accept/postoverride
flowcb/accept/tohttp
flowcb/accept/wmotypesuffix
That item was removed as part of writing unit tests
That item was removed as part of writing unit tests
downloadbaseurl
renamedmf
tolocalfile

All with 100% coverage
Should have been part of the inital set of fixes, but they weren't known until the tests were fixed.. sort of circular problem.
There's a class we're testing that starts with 'Test', and it was throwing warnings for pytest. Updating the config to only look for classes that start with 'Test_*' now.
These fixes reflect the changes that were done in #756
Some of them also are fixes to make sure these tests are successful when run as part of the whole suite (caplog message counts among them). Those assertions probably aren't all that purposeful, and checking that the desired logs are in the list is better, but here we are..
@petersilva petersilva merged commit 58fbbba into MetPX:v03_wip Sep 7, 2023
15 of 19 checks passed
@petersilva
Copy link
Contributor

they will not affect the integration tests, so it should be fine to fix any lingering issues post-merge.

@gcglinton gcglinton deleted the FlowCBTests branch September 7, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants