-
Notifications
You must be signed in to change notification settings - Fork 131
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 Testing #531
Comments
I was going to suggest pFUnit before skimming the CESM tutorial and seeing that's what they use. Looking at the pFUnit README, it seems we could use it without major changes to our build system, which is good. I definitely think that we should use an existing framework for unit testing instead of writing something ourselves. I don't think the requirements for pFUnit should discourage us from using it; CMake is pretty standard these days and the rest of the requirements are already in our own requirements. When we have unit tests, they should be run as part of the weekly testing, yes, but also in PR builds. |
Initial implementation of unit testing added in #566 |
Just an update that a calendar unit test was added as part of the new time manager implementation, #566. This unit test has it's own driver, is written in Fortran, and leverages all the script and test infrastructure that already exists. To invoke it, use "--test unittest -s calchk". Other tools like pFUnit are still on the table, but this provides a starting point. |
We have added several other unit tests and can continue to do so. I think we have a framework using our current build and ability to create a separate test driver. We now have several unit tests testing global sums, the calendar, and the broadcast infrastructure.. I am going to close this. As we move forward, we should continue to evaluate whether the unit testing infrastructure is working for us and if not, consider pfUnit and/or other methods. See for instance, #606. |
We will want unit testing for our new time manager. We may also want to add unit testing for other infrastructure interfaces where a simple driver can be created to test the interfaces outside of CICE. The question is how to best do that. There are many tools that facilitate unit testing.
One relatively inelegant but familiar way would be to create unit test drivers in Fortran that would build and run with the current Makefile and scripts, be supported in our test suite system, and produce an output file which could then be reviewed and regression tested against. This is likely relatively straight-forward and involves no new build systems, coding strategies, or tools. It's familiar, and it should be relatively easy to maintain and extend.
@eclare108213, @phil-blain, @dabail10, @TillRasmussen, @rallard77, @mattdturner, I am interested to hear other ideas about how we might proceed on this front. I'm not particularly familiar with a lot of tools, but know different groups have tried different things such as
https://github.com/NCAR/cesm_unit_test_tutorial/
It's not clear to me that these things "catch on" or are ultimately maintained. There are almost certainly benefits. But there may also be drawbacks in terms of installation, having to learn new tools/languages, maintenance, and general buy-in. But I don't want to discount them out of hand. Looking for input from others. Some questions to consider,
To provide a starting point, my feeling is that we should build unit tests mainly to test low level infrastructure (time managers, communication, etc). They should be used to extend test coverage and to validate implementation where CICE can't (i.e. test things that CICE isn't calling or test 1000s of years of the time manager with different calendars). I think only a few internal developers are going to create and run the unit tests, but they should be part of our weekly testing. And I prefer to write in Fortran and leverage the current CICE build, run, and test scripts. I'd rather not have to add new tools/languages/etc until it becomes clear there is benefit. And if/when that day comes, we pay the up front cost and migrate.
Again, would be happy to go another direction if it makes sense. I will also mention #437 here.
The text was updated successfully, but these errors were encountered: