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

Remove test data from fims deterministic folder #481

Conversation

Bai-Li-NOAA
Copy link
Contributor

What is the feature?

  • Remove the tests/integration/inputs/FIMS-deterministic folder and its file contents
  • Enable the automatic execution of the model comparison project to generate test data when running FIMS tests

How have you implemented the solution?

  • Delete the tests/integration/inputs/FIMS-deterministic folder and include the test data folder in the .gitignore file.
  • Modify the setup_gtest() function to rename test data folder.
  • Add code into CMakeLists.txt to execute setup_gtest() so that developers can have the test data readily available for C++ integration tests before running GoogleTest locally.
  • Add GITHUB_PAT into run-googletest.yml to make sure the gha passes.

Does the PR impact any other area of the project?

NA

How to test this change

The GitHub Actions workflows that failed in this pull request are expected to function correctly once all changes are merged into the main branch (see GHA workflow that has passed during the testing of the workflow using a specific FIMS installation). This will include running run-googletest.yml after updating FIMS to the latest version with the revised setup_gtest() function.

Developer pre-PR checklist

  • I relied on GitHub actions to 🧪 things for me while I sat on the 🛋️.

@Bai-Li-NOAA Bai-Li-NOAA linked an issue Oct 18, 2023 that may be closed by this pull request
1 task
@github-actions
Copy link
Contributor

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete!
  • For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
  • Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
  • Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
  • PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
  • Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

  • The PR is requested to be merged into the appropriate branch (typically main)
  • The code is well-designed.
  • The functionality is good for the users of the code.
  • Any User Interface changes are sensible and look good.
  • The code isn’t more complex than it needs to be.
  • Code coverage remains high, indicating the new code is tested.
  • The developer used clear names for everything.
  • Comments are clear and useful, and mostly explain why instead of what.
  • Code is appropriately documented (doxygen and roxygen).

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files Coverage Δ
R/setup_gtest.R 0.00% <0.00%> (ø)

... and 13 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@k-doering-NOAA
Copy link
Member

@Bai-Li-NOAA I see some of the GitHub actions are not passing - are there still changes to be made?

@Bai-Li-NOAA
Copy link
Contributor Author

@k-doering-NOAA, there are no changes required. The run-googletest.yml needs to be executed after updating FIMS to the latest version, which includes the revised setup_gtest() function. The GitHub Actions workflows are expected to pass once all the changes are merged into the main branch. I have tested the GitHub Actions runs on a feature branch, as shown here.

Maybe I can submit two separate pull requests: one to update the setup_gtest() function, and another to update run-googletest.yml. This approach should ensure that all GitHub Actions workflows pass for both pull requests.

@k-doering-NOAA
Copy link
Member

Thanks for explaining! I think it's ok as-is, but we may need to bypass the branch protection rules to get this in, since run-googletest is a required workflow...

@ChristineStawitz-NOAA, is it ok to temporarily bypass branch protection in this case?

Also, somehow my group was assigned this issue, but you already did it for us, Bai! I think we can review it today to help finish it up, so I assigned us as reviewers.

@Bai-Li-NOAA
Copy link
Contributor Author

Oops, I might need some new glasses because I thought I got this task last week, so I jumped into fixing it. Thanks for the group review!

@k-doering-NOAA
Copy link
Member

It was definitely assigned to you on the issue - and no worries, we weren't confident we would be able to fix it, so we are glad you jumped in! :)

Copy link
Contributor

@MOshima-PIFSC MOshima-PIFSC left a comment

Choose a reason for hiding this comment

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

We had one comment in the tests/gtest/integration_test_fixture.cpp file that should be addressed before merging. Other than that everything looks good. Some github actions will not pass until merged into main.

@github-actions
Copy link
Contributor

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete!
  • For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
  • Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
  • Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
  • PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
  • Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

  • The PR is requested to be merged into the appropriate branch (typically main)
  • The code is well-designed.
  • The functionality is good for the users of the code.
  • Any User Interface changes are sensible and look good.
  • The code isn’t more complex than it needs to be.
  • Code coverage remains high, indicating the new code is tested.
  • The developer used clear names for everything.
  • Comments are clear and useful, and mostly explain why instead of what.
  • Code is appropriately documented (doxygen and roxygen).

@Bai-Li-NOAA Bai-Li-NOAA force-pushed the 317-bug-remove-test-data-from-fims-deterministic-folder branch from 66f1322 to 29ead81 Compare October 21, 2023 14:54
@github-actions
Copy link
Contributor

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete!
  • For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
  • Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
  • Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
  • PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
  • Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

  • The PR is requested to be merged into the appropriate branch (typically main)
  • The code is well-designed.
  • The functionality is good for the users of the code.
  • Any User Interface changes are sensible and look good.
  • The code isn’t more complex than it needs to be.
  • Code coverage remains high, indicating the new code is tested.
  • The developer used clear names for everything.
  • Comments are clear and useful, and mostly explain why instead of what.
  • Code is appropriately documented (doxygen and roxygen).

@ChristineStawitz-NOAA ChristineStawitz-NOAA merged commit e18f15f into main Oct 24, 2023
10 of 12 checks passed
@ChristineStawitz-NOAA ChristineStawitz-NOAA deleted the 317-bug-remove-test-data-from-fims-deterministic-folder branch October 24, 2023 12:57
@Bai-Li-NOAA
Copy link
Contributor Author

Bai-Li-NOAA commented Oct 24, 2023

@k-doering-NOAA and @ChristineStawitz-NOAA, The main branch's GitHub Actions checks have failed, and I suspect it's due to GHA not installing the latest version of FIMS. You can find more information about this issue here. Maybe we can replace remotes::install_local(upgrade = "always") with remotes::install_github("NOAA-FIMS/FIMS", upgrade = "always", force = TRUE)?

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.

[Bug]: remove test data from FIMS-deterministic folder
4 participants