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

eliminate duplication of replication sequences within stseq function #617

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

jbathegit
Copy link
Collaborator

Fixes #612

Also eliminates dlloctbf_c binding which is no longer needed.

@jbathegit
Copy link
Collaborator Author

Hey @jack-woollen @edwardhartnett @AlexanderRichert-NOAA would any of you possibly have some time during the next couple of days to take a look at this?

(Thanks! :-)

Copy link
Contributor

@AlexanderRichert-NOAA AlexanderRichert-NOAA left a comment

Choose a reason for hiding this comment

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

How easy would it be to add remaining code coverage for stseq.c? I don't think you're technically adding new untested lines of code, but there are some untested lines that got shuffled around a bit, if I'm reading the diff correctly. Otherwise looks good.

@jbathegit
Copy link
Collaborator Author

How easy would it be to add remaining code coverage for stseq.c? I don't think you're technically adding new untested lines of code, but there are some untested lines that got shuffled around a bit, if I'm reading the diff correctly. Otherwise looks good.

Thanks Alex and Jack for your replies!

Alex, you're correct that I'm not adding any untested lines with this PR, and that all of the untested lines in stseq() are ones that previously existed. But many of those aren't easy to test, either because they're hard to simulate (e.g. if a malloc fails, or if pktdd() somehow encounters a corrupt counter value) or because I've never actually seen any cases of NWP centers generating sample data with some of these more esoteric BUFR features (e.g. substituted values, replaced/retained values) even though they are theoretically possible according to the WMO regulations.

If and when any of that changes, then I'll certainly be more than willing to add new test cases. But for now at least the testing coverage is as complete as I can manage for stseq().

@jbathegit jbathegit merged commit 9d4e0ee into develop Sep 5, 2024
10 checks passed
@jbathegit jbathegit deleted the jba_issue612 branch September 5, 2024 15:10
@edwardhartnett
Copy link
Contributor

Indeed in Fortan and C it is not usual to test for such error conditions. There's no reasonable way to make malloc fail, but you should certainly have code to deal with a malloc failure. So that code will be impossible to reach in testing.

In other languages, like Python, there is a concept of "mocking" which allows us to easily reach 100% test coverage. But there's no easy way to do that in Fortran/C, so I'm happy to achieve something over 80% with Fortran/C code. And I believe NCEPLIBS-bufr is well into the 90s now...

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.

look into eliminating RPSEQ### duplication in stseq function
4 participants