Closed rwest closed 8 months ago
⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 54.86%. Comparing base (
8cac4c7
) to head (47d985b
). Report is 1 commits behind head on main.:exclamation: Current head 47d985b differs from pull request most recent head 9fb6d6f. Consider uploading reports for the commit 9fb6d6f to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
We could drop the reverted commits when rebasing before merging but I'm leaving them for review so you can follow along.
Thoughts on simply removing that unit test instead, making this PR something like "remove disused test and duplicated test data"?
I advise against removing the test without removing the function that it's testing. One could argue for that if trying to throw away all unused code. But as it's currently working, and has been helpful to people in the past, I feel we might as well leave it. (Although the chances that someone who could use it it will notice it's there is arguably small.) Anyway, I think the current way (in this PR) is fine.
I rebased, removing the commits that had no net effect, and added one more that moves the test into the class where it belongs. When the tests pass, I suggest this is merged.
@rwest sounds like a plan! I will let #2643 merge, rebase this, and then enble auto-merge. (Previous docs build failure was due to network drop)
⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
Motivation or Problem
Jackson tried removing this test data directory as part of another pull request (#2598). We're now having problems in a related area, caused by this, so I'm doing it as a standalone pull request. It took a few fixes to get it working. In any case, it would be nice to remove duplication (as noted in the commit message of 3177452d8a61987e6205f3cd6a64669bcffbaa31.)
Description of Changes
We already have a
test/rmg_py/test_data/testing_database
which contains a mock database for running unit tests. This PR removesrmgpy/data/test_data/
which served a similar purpose. Because they were slightly different, the tests needed to be fixed. Also moved test_remove_group (one of the broken tests) out of TestCyclicThermo into TestThermoDatabase because it had nothing to do with cyclic thermo.The test test_remove_group tests a function (remove_group) that is not currently used in RMG, but has been used in the past for manipulating the database offline. Because it could be helpful to people in the future, and is not currently (after this PR) broken, we might as well leave it. But one could argue for deleting it.
Testing
Unit tests should pass.