-
Notifications
You must be signed in to change notification settings - Fork 123
Add developer workflow with test coverage calculation #1350
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
Add developer workflow with test coverage calculation #1350
Conversation
…A/UPP into as_test_coverage
clyden-noaa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WenMeng-NOAA @BenjaminBlake-NOAA @AlysonStahl-NOAA
I have completed testing on the RDHPCs.
All tests were successful and finished within timing thresholds
There were no baseline changes, as expected
Please let me know if any additional testing will be required once the build error is resolved on WCOSS2.
|
@AlysonStahl-NOAA Hi Alyson, have you had a chance to look into this? As we discussed, this project does not involve IFI and GTG. Thanks.
|
|
Apologies for the delay, I have been looking at this. I have been getting that same warning about the libjpeg and libjasper conflict, but the test program builds fine for me. I don't have access to GTG or IFI, so I can't tell if those options in particular are what's causing the issue. I did go back to some of my changes and found that I introduced redundant CMake flags, which I since removed. But I can't see how they would cause any issues. It's not clear to me yet if that libjpeg/libjasper conflict is related to this problem, even if it did manage to build for me. I've been trying to figure out the source of the conflict. |
|
@AlysonStahl-NOAA Thanks for looking into it! I think the libjpeg/libjasper conflict error can be ignored because I also get those messages when compiling the UPP code, and it still builds successfully. I just tried building the UPP executable (including GTG and IFI) with your latest changes, but it still has trouble. Here is my working directory on Cactus if that would be helpful for debugging: Within the tests/build directory you can find the files that are referenced inside the compile.20251126.log file. |
|
@AlysonStahl-NOAA Hope you had a nice Thanksgiving! It looks like this line inside build/unit_tests/CMakeFiles/test_dewpoint.dir/build.make is where the error occurs: I compared the build.make from my failed build to a build.make file from a successful build, and I noticed the following 4 lines are only included in the build.make of the failed build: I wonder if the paths to the libIFI.fd directory should be the full paths instead of the relative paths listed above, or if they should be removed entirely from build,make. What do you think? |
|
@BenjaminBlake-NOAA You as well! Apologies for my slowness in responding here. I just returned from time off. I don't know if I can recommend completely removing them because I don't entirely understand yet how libIFI is used by UPP or if moving the paths will break anything. I do think that the full path is generally preferred. It's a little more difficult for me to debug without direct access to GTG/IFI. I see that your build uses "INTERNAL_IFI" and I can see the IFI library under sorc in your UPP-alyson directory. Would I be able to copy this and test it out? Also, is this build the only one that uses internal IFI? I'm wondering if this is a path issue. |
|
I should also note, that it is possible to build without the unit tests using BUILD_TESTING=OFF. As far as I know, they will mostly be run in the GitHub workflows. Although if there is an actual issue exposed here we probably don't want to just get around them by turning unit tests off. |
|
@AlysonStahl-NOAA No worries! Yes I think you should be able to copy the code from my directory and use it for testing/debugging. I think UPP is the only build that uses internal IFI since it is related to aviation products (in-flight icing). If the issue becomes troublesome we could alternatively set BUILD_TESTING=OFF when building with the IFI code, that's also true. |
|
Oh, by internal IFI I just mean the Cmake build option INTERNAL_IFI. I noticed that there seems to be a separate option to load it as an existing external module. I wanted to see if maybe the issue was arising with INTERNAL_IFI specifically. I copied libIFI over and tried building UPP again and ended up getting the same error. One thing I did notice is that, with IFI enabled, it tries to link the test program as a CXX executable rather than a Fortran one. Using IFI seems to add additional compilation flags for the executable. |
|
I added a small change. This fixed it on my end while building with your copy of libIFI. Let me know if that fixes it for you. |
|
Ahh okay, understood about INTERNAL_IFI. Your change to set the linker language to Fortran also fixed the error for me. So I think we are good to continue with processing your PR. Thanks for your help! |
|
@clyden-noaa Now that the compilation error related to IFI has been fixed, let's continue processing this PR. When you get a chance, could you re-run the UPP RTs on the R&D machines? Thanks! |
|
@BenjaminBlake-NOAA @WenMeng-NOAA @AlysonStahl-NOAA |
clyden-noaa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WenMeng-NOAA @BenjaminBlake-NOAA @AlysonStahl-NOAA
All of my testing is complete.
All of the RTs were successful and finished within accepted timing thresholds across all RDHPCs.
There were no baseline changes, as expected.
|
@clyden-noaa Thanks for testing - this PR is ready to be merged. |
Fixes #1353
Fixes #1354