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 IO unit test #1136

Closed
wants to merge 6 commits into from

Conversation

edwardhartnett
Copy link
Contributor

@edwardhartnett edwardhartnett commented Dec 4, 2023

Pull Request Summary

Add a test for existing binary point output.

Description

This PR does not include any code changes to the model, only test code and configuration.

The existing GitHub action CI runs do not attempt to execute unit tests (because there are none, so that would fail). In this PR I introduce a new CI YML file that runs the tests. Once this PR is merged, I will submit a follow-up PR which causes all the CI runs to run ctest.

The new CI also runs a code coverage report and uploads it to GitHub actions. This only needs to be done by one of the CI runs.

Some small data files are necessary for this test to work, the are included in model/tests/data and are copied to the binary build directories by the model/tests/CMakeLists.txt file.

Once the unit test is merged and running, @MatthewMasarik-NOAA can make it a "Required Status Check", which means that merges will not be permitted if this test fails. (And that's how you want to treat all your unit tests.)

Once we have at least one required status check, I believe that GitHub will automatically enforce that the PR has been updated with the latest version of the develop branch, so you can stop asking that in the PR checklists. We can test this to confirm.

Issue(s) addressed

Part of #682

Commit Message

Add a test for existing binary point output.

Check list

Testing

  • How were these changes tested? On my branch.
  • Are the changes covered by regression tests? (If not, why? Do new tests need to be added?) No.
  • Have the matrix regression tests been run (if yes, please note HPC and compiler)? No, no code changes.
  • Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.) No changes expected.
  • Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

@JessicaMeixner-NOAA
Copy link
Collaborator

@edwardhartnett we do not allow binary files in the WW3 repository. This PR adds 2. Can you please close this PR so that we do not have binary file in our repository? I'm happy to set-up an offline meeting to discuss other ways to create these files without saving binary files to the repository.

@edwardhartnett edwardhartnett reopened this Dec 6, 2023
@edwardhartnett edwardhartnett marked this pull request as draft December 6, 2023 20:48
@JessicaMeixner-NOAA
Copy link
Collaborator

@edwardhartnett can we please re-close this PR and/or remove the binary files? Thanks in advance for your help keeping the WW3 repo free of binary files.

@MatthewMasarik-NOAA
Copy link
Collaborator

MatthewMasarik-NOAA commented Dec 6, 2023

@edwardhartnett I wanted to add some context regarding the binary files. @JessicaMeixner-NOAA and I discussed this shortly after we met today. If something gets in that we don't allow, and we don't notice it, it could be a lot of pain manually removing it and then rewriting history. Because if a file has already been committed to your fork, I believe even if you remove the files in new commits, it will still be a part of the repo. Based on the risk, please create a new branch with the work carried over, minus binaries, when your ready for the PR to be reviewed.

@edwardhartnett
Copy link
Contributor Author

@JessicaMeixner-NOAA I am removing the binary files but I need to leave the PR open so that the CI can run.

I have converted the PR to draft and will not mark it "ready to review" until both binary files are removed.

@JessicaMeixner-NOAA
Copy link
Collaborator

In the past, CI tests could also be run from branches in forks and did not require a PR to the main repository.

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