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

Add ExtData Testing Framework #1556

Merged
merged 17 commits into from
Jun 15, 2022
Merged

Conversation

bena-nasa
Copy link
Collaborator

@bena-nasa bena-nasa commented Jun 10, 2022

I spent some time cleaning up the ExtData testing framework I maintain on my own personal git repo that I use to make sure I have not broken ExtData (and to some extent History as it requires History to function with the most basic functionality). I'm making this PR here, more for feedback.

The idea is that I have a set of test cases that exercise various ExtData data cases by having History generate the input for ExtData. Each case consists of input files only, no data as that is generated in each case on the fly.

I then have a set of python scripts to run each case, by moving the RC files for the specified cases to a temp directory, running ExtDataDriver.x, checking that the program finishes successfully, and then deleting the temp directory

I thought eventually we would want this in MAPL, but not sure how it fits into any other tests (its not PFUNIT!) or we would make use (is it something user has to explicitly run, that cmake can run or that CI can run like we run the model for example). Maybe it doesn't belong here yet but I figured since I took the time to clean things up for my own sake I would see if there are thoughts about this and how we might incorporate it, if that is something that anyone besides me would have interest in.

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)

Checklist:

  • I have tested this change with a run of GEOSgcm (if non-trivial)
  • I have added one of the required labels (0 diff, 0 diff trivial, 0 diff structural, non 0-diff)
  • I have updated the CHANGELOG.md accordingly following the style of Keep a Changelog

@bena-nasa bena-nasa added 🎁 New Feature This is a new feature 0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs labels Jun 10, 2022
@bena-nasa bena-nasa requested review from a team as code owners June 10, 2022 18:30
@bena-nasa bena-nasa requested a review from tclune June 10, 2022 18:30
tclune
tclune previously approved these changes Jun 10, 2022
@tclune
Copy link
Collaborator

tclune commented Jun 10, 2022

CMake can add tests that for things that are not pFunit. Just need to define a command that should return 0 (success), and then a cmake add_test() command. (Have not done it in a while, so I'm rusty on the details.) Then the existing CI will exercise this as well.

The challenge usually is to get relevant input data into the CMake test environment. If the test generates its own inputs, all the better.

@bena-nasa
Copy link
Collaborator Author

CMake can add tests that for things that are not pFunit. Just need to define a command that should return 0 (success), and then a cmake add_test() command. (Have not done it in a while, so I'm rusty on the details.) Then the existing CI will exercise this as well.

The challenge usually is to get relevant input data into the CMake test environment. If the test generates its own inputs, all the better.

Thanks, I'll take a look around and see how it is done elsewhere in MAPL see how this could fit in with cmake

@bena-nasa bena-nasa requested a review from a team as a code owner June 14, 2022 17:33
@bena-nasa bena-nasa removed the 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs label Jun 14, 2022
@bena-nasa bena-nasa requested a review from tclune June 14, 2022 20:20
Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

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

Added inline comments/questions.

@bena-nasa
Copy link
Collaborator Author

It appears that the issue is "large" test I have that must be beyond the scope of the CI can handle based on the resources avaialable. For now I will remove that from the cases.txt file that so that a generic make tests command does not run it. If I or someone else wants to test it ourselves for the time being easy enough for me to add the line back to the file

@mathomp4
Copy link
Member

It appears that the issue is "large" test I have that must be beyond the scope of the CI can handle based on the resources avaialable. For now I will remove that from the cases.txt file that so that a generic make tests command does not run it. If I or someone else wants to test it ourselves for the time being easy enough for me to add the line back to the file

I suppose we could also have a label for that test? Maybe it could be a "VERYLARGE" test and we could exclude them from CI, but still have it as a case in general?

@tclune
Copy link
Collaborator

tclune commented Jun 15, 2022

If @bena-nasa finds the test useful, I would not want to lose it. It could still be exercised by the nightly scripts.

@bena-nasa
Copy link
Collaborator Author

If @bena-nasa finds the test useful, I would not want to lose it. It could still be exercised by the nightly scripts.

I'm not deleting it, we updated the ci to skip that test but certainly be hand run or if I could figure out a way to test this on fewer cores might be the best bet

@bena-nasa bena-nasa requested a review from tclune June 15, 2022 15:05
@mathomp4
Copy link
Member

Woooo! All pass! 🎉

@tclune tclune merged commit 79eb1be into develop Jun 15, 2022
@tclune tclune deleted the feature/bmauer/extdata_test_cases branch June 15, 2022 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff Trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) 🎁 New Feature This is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants