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

Improve test for TADA_PairForCriteriaCalc #525

Closed
hillarymarler opened this issue Sep 13, 2024 · 4 comments · Fixed by #522
Closed

Improve test for TADA_PairForCriteriaCalc #525

hillarymarler opened this issue Sep 13, 2024 · 4 comments · Fixed by #522
Assignees
Labels
bug Good First Issue Good issue for first time contributors MVP

Comments

@hillarymarler
Copy link
Collaborator

In test-CriteriaComparison.R, test occasionally fails when the random test df does not contain any of the characteristics (pH, temp, salinity, chloride) to be paired. Test needs to be updated to re-run TADA_RandomTestingData until a data set containing one or more of the paired characteristics is found.

@hillarymarler hillarymarler added Good First Issue Good issue for first time contributors bug MVP labels Sep 13, 2024
@wokenny13 wokenny13 self-assigned this Sep 16, 2024
@wokenny13
Copy link
Collaborator

wokenny13 commented Sep 16, 2024

@hillarymarler @cristinamullin

Would it make sense for test_that() in test-CriteriaComparison.R to contain any paired parameter argument (with TADA_DataRetrieval() ) options rather than run TADA_RandomTestingData() for testdat1?

Or would we want to continually re-run the TADA_RandomTestingData for a particular reason when running a test function?

@wokenny13
Copy link
Collaborator

I included tryCatch() and try() base functions to handle rerunning TADA_RandomTestingData() for testdat1 if they do not contain any paired parameters needed to run TADA_PairForCriteriaCalc. These changes have been made within the WQXunitRef pull request.

The test_that will attempt to run up to 10 times in a for loop (can make this iteration larger, but if the dataset does not include the paired parameter after a certain amount of times, this may be an external/internal error that may be outside of the CriteriaCalc functions and will slow down the check process if the loop runs for too long).

Reviewing and testing of this logic should be done to ensure it works as intended.

@hillarymarler
Copy link
Collaborator Author

That sounds like a great solution! Is PR #522 ready for review or are there still additional changes you would like to make first?

@wokenny13
Copy link
Collaborator

That sounds like a great solution! Is PR #522 ready for review or are there still additional changes you would like to make first?

#522 is ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Good First Issue Good issue for first time contributors MVP
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants