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

Adding pFUnit Support for Unit Testing in obs2ioda #23

Closed
wants to merge 13 commits into from

Conversation

amstokely
Copy link
Collaborator

@amstokely amstokely commented Nov 14, 2024

This PR introduces support for using pFUnit to write unit tests in the obs2ioda project, a key step in refactoring legacy Fortran code. Unit testing is essential for verifying that code changes do not introduce subtle, breaking issues. Given the current lack of a unit testing framework in obs2ioda, maintaining code quality and making major changes safely have been challenging.

This PR addresses these issues by:

  • Adding pFUnit support to the CMake build, enabling the creation and integration of unit tests.

The installation instructions have been updated with guidance on installing pFUnit and enabling testing during the CMake build process.

By establishing a unit testing framework, this PR sets the foundation for future improvements and enhances the reliability of code refactoring efforts.

**Closing this PR due to compilation issues with pFUnit and the nvhpc compilers (pFUnit Issue 476)

@jim-p-w
Copy link
Collaborator

jim-p-w commented Nov 15, 2024

Can you update the links in top level README.md which point to the obs2ioda-v1/README.md and obs2ioda-v2/README.md? Right now they point to files in a different git repo, so I can't see your changes to the `obs2ioda-v2/README.md by clicking on the link in the top level README.
Thanks for fixing this.

obs2ioda-v2/README.md Outdated Show resolved Hide resolved
obs2ioda-v2/README.md Outdated Show resolved Hide resolved
obs2ioda-v2/README.md Show resolved Hide resolved
…tests.

As of now, there are no unit tests but that will change in the future. Also updated installation section of The README to include instructions on how to install pFUnit and build obs2ioda with pFUnit support.
The command now is executed with bash -c '<prev command>' to make it possible to run from any shell, given the system has bash installed.
Removed seemingly hard requirement that build directories have to be named 'build' in the install instructions.
obs2ioda-v2/README.md Outdated Show resolved Hide resolved
obs2ioda-v2/README.md Outdated Show resolved Hide resolved
@amstokely amstokely requested a review from jim-p-w December 3, 2024 20:50
@mgduda
Copy link
Collaborator

mgduda commented Dec 6, 2024

Is the code in f_c_string_t_mod.f90 and f_c_string_1D_t_mod.f90 intended to be used by obs2ioda, or is it just there to demonstrate how the pFUnit testing works? If the former, then I think this PR is doing more than just adding pFUnit support.

@amstokely
Copy link
Collaborator Author

Is the code in f_c_string_t_mod.f90 and f_c_string_1D_t_mod.f90 intended to be used by obs2ioda, or is it just there to demonstrate how the pFUnit testing works? If the former, then I think this PR is doing more than just adding pFUnit support.

I meant to include these files in a later PR. Thanks for pointing this out!.

CMakeLists.txt Outdated Show resolved Hide resolved
If you have an environment preconfigured for `mpas-jedi`, simply source that environment prior to building `obs2ioda`.
Prior to building `obs2ioda`, ensure that you have installed the following libraries:

- **CMake**: Required (version 3.20 or higher).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should now read 3.12.

@@ -54,9 +73,34 @@ To install the NCEP BUFR library, follow these steps:
```
5. To locate the NCEP BUFR library, run:
```bash
find . -name *libbufr*
bash -c "find . -name *libbufr*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific reason this find command needs to be run in the bash shell?

@amstokely amstokely closed this Dec 11, 2024
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.

3 participants