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

Make the R tests more modular #606

Closed
Bai-Li-NOAA opened this issue May 29, 2024 · 4 comments · Fixed by #651
Closed

Make the R tests more modular #606

Bai-Li-NOAA opened this issue May 29, 2024 · 4 comments · Fixed by #651
Assignees
Labels
kind: test case Testing of the software P3 low priority task theme: code cleanup
Milestone

Comments

@Bai-Li-NOAA
Copy link
Contributor

The one question that this PR #591 raises for me is whether there's a way to make the tests more modular. That is, the new "agecomp in proportion works" test added to test-integration-fims-estimation.R has a lot of redundancy with the existing "estimation test of fims". We want to make sure proportions still pass all the same checks but the duplicate code would be harder to maintain than if there's some way to wrap it in a function without losing the ability to get detailed output from each element of the test. Having said that, I don't think it makes sense to spend more time on this now, rather to think about the issue for future revisions to the tests.

Originally posted by @iantaylor-NOAA in #591 (review)

@Bai-Li-NOAA Bai-Li-NOAA added the status: triage_needed This is not approved for this milestone, do not work on it yet label May 29, 2024
@Bai-Li-NOAA
Copy link
Contributor Author

Thanks, @iantaylor-NOAA. That's a great suggestion, and it is possible to make the tests more modular. I have been testing this in a different branch by

The changes have resulted in much shorter test file (e.g., test-integration-fleet-log-obs-error-input.R). I can refactor all integration tests after merging in the test-obs-error-input branch.

@kellijohnson-NOAA
Copy link
Contributor

@Bai-Li-NOAA thanks for working on this. Do you think that the test data should probably be added to the repository as a data object where your script to generate the data could be placed in data-raw and usethis::use_data() could be used to place the data object in the correct location.

@Bai-Li-NOAA
Copy link
Contributor Author

Bai-Li-NOAA commented May 29, 2024

@kellijohnson-NOAA Good question! It depends on whether we want to distribute the test data to users. If a data object is generated from an R script in the data-raw/ folder, it will be accessible to users after installing the package. For test data, I think users don't need access. Therefore, we can use test fixtures within the {testthat} framework. See the file structure figure in the R Packages (2e) book here.

@kellijohnson-NOAA
Copy link
Contributor

Looks great, thanks for the link 👍!

@Bai-Li-NOAA Bai-Li-NOAA self-assigned this May 29, 2024
@kellijohnson-NOAA kellijohnson-NOAA added kind: test case Testing of the software P3 low priority task and removed status: triage_needed This is not approved for this milestone, do not work on it yet labels Jun 3, 2024
@kellijohnson-NOAA kellijohnson-NOAA added this to the 2 milestone Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: test case Testing of the software P3 low priority task theme: code cleanup
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants