Heron unit testing #20
Conversation
…E into heron-unit-testing
There was a problem hiding this comment.
Great work, VERY thorough addition to the FORCE testing suite! I have added a couple of comments and suggestions to improve the code so far, and had just a couple of questions.
I think overall, would try to consolidate some repeated lines of code wherever possible.
tests/unit_tests/test_heron.py
Outdated
| self.assertIn('type', cashflows[0].keys()) | ||
| self.assertIn('taxable', cashflows[0].keys()) | ||
| self.assertIn('inflation', cashflows[0].keys()) | ||
| self.assertIn('mult_target', cashflows[0].keys()) |
There was a problem hiding this comment.
this is actually a deprecated attribute, so it should be ignored
tests/unit_tests/test_heron.py
Outdated
| self.assertIn('mult_target', cashflows[0].keys()) | ||
|
|
||
| # Verify reference price | ||
| ref_price_value = cashflows[0].find('./reference_price/fixed_value') |
There was a problem hiding this comment.
should this check first that reference_price exists? same for reference_driver a couple of lines down
There was a problem hiding this comment.
This should be addressed by the new methods being added. In either case, find() returns None if no matches are found; thus, the error is caught, albeit without a great deal of clarity as to the cause of the issue in this version.
tests/unit_tests/test_heron.py
Outdated
| result_tree = create_componentsets_in_HERON("/fake/folder", "/fake/heron_input.xml") | ||
|
|
||
| # Verify Case node was transferred | ||
| with (self.subTest("Case node has been corrupted")): |
There was a problem hiding this comment.
is there a reason why the self.subTest( ... ) call is wrapped in extra parentheses? seems like this is not the case later in the file
There was a problem hiding this comment.
No, there is no reason for this. The extra parentheses have been removed
tests/unit_tests/test_heron.py
Outdated
| self.assertEqual(len(cf_contents), 4) | ||
|
|
||
| # Reference driver | ||
| ref_driver = cashflow[0].findall('./reference_driver') |
There was a problem hiding this comment.
I'm seeing some repetition of testing here, I wonder if it would be better to have some sort of method written which does these checks (maybe with the text value as an input) at the top of this file?
There was a problem hiding this comment.
I think that could be really helpful for simplifying the tests and also improving the clarity of the results. I'll set up some methods/functions for grouping these frequently used parse-and-assert combinations.
tests/unit_tests/test_heron.py
Outdated
|
|
||
| # Verify ProjectTime was not corrupted | ||
| with self.subTest("Non-cashflow child node of economics has been corrupted"): | ||
| proj_time = economics[0].findall('./ProjectTime') |
There was a problem hiding this comment.
this should also be lifetime
| <reference_price> | ||
| <ROM rom_stuff="stuff">more_rom_stuff</ROM> | ||
| </reference_price> | ||
| <scaling_factor_x> |
There was a problem hiding this comment.
I think it's good to check these mixed-ValuedParam cases. From what I'm seeing here, every child node here gets replaced with a value from the mock_open callable below. Now this might be a question of ignorance, but: would it be possible to leave one of these parameters unchanged? By that I mean, for example, that the end goal for Component 2 is to replace the reference_driver, the reference_price, and the driver with values read in from Line 246-248, BUT we wanted to keep the scaling_factor_x as an uncertainty. Is that possible to test for/a current capability of the method?
There was a problem hiding this comment.
The reason I chose not to modify the set of ValuedParams is that, in the current functionality of the src/heron.py script, all the ValuedParam inputs from ASPEN HYSYS and APEA are required. The src/heron.py script currently trusts that the ASPEN HYSYS and APEA inputs have been provided, so it does not reliably throw an error when one is missing. Thus, neither a functional nor an erroneous response to the omission of some of the ValuedParams can be tested.
| mock_listdir.return_value = [] | ||
|
|
||
| # Call the function | ||
| result_tree = create_componentsets_in_HERON("/fake/folder", "/fake/heron_input.xml") |
There was a problem hiding this comment.
this might be out of the scope of this PR, but should this scenario cause an error or at least a warning?
There was a problem hiding this comment.
This is a question whose answer could depend on some broader strategic goals for how this function will be used. A warning does sound like a safe middle-of-the-road option though. No matter the solution, I think it would be more appropriately addressed in a separate issue and PR. If functionality changes significantly, this test can also be updated then.
tests/unit_tests/test_heron.py
Outdated
| # Verify open function was called on correct files | ||
| for file in files_list: | ||
| # if file should have been opened | ||
| if file in ['componentSet.json', 'componentSetStuff.txt']: |
There was a problem hiding this comment.
might be good to have a list before the for loop which has the acceptable filenames (e.g., acceptable_files) then refer to it in this line. That way it is easier to modify should we ever need to
| class TestMinimalInput(unittest.TestCase): | ||
| class HERONTestCase(unittest.TestCase): | ||
|
|
||
| def check_reference_price(self, cashflow, correct_value, correct_content_length=1): |
There was a problem hiding this comment.
would you be able to add docstrings to these new methods? For reference: https://github.com/idaholab/raven/wiki/RAVEN-Code-Standards#function-and-class-documentation-strings
There was a problem hiding this comment.
No problem.
| with self.subTest("Non-capex CashFlow node was corrupted"): | ||
| cashflow_non_capex = economics.findall('./CashFlow[@name="other"]') | ||
| self.assertEqual(len(cashflow_non_capex), 1) | ||
| cf_noncap_contents = [e for e in cashflow_non_capex[0].findall('./') |
There was a problem hiding this comment.
@caleb-sitton-inl seems like it's best to keep as is, but also it might be a good idea to start an issue for Feature Request regarding switching to dynamic TreeStructure elements rather than ET
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Not Applicable
What are the significant changes in functionality due to this change request?
This implements a complete set of nine HERON unit tests. Specifically, the tests focus on the
src/heron/create_componentsets_in_HERON()function. Thetests/unit_tests/test_heron.pyfile contains the tests, which use theunittestmodule to run. This file can be run directly and all unit tests will run. This set of tests is designed to cover the vast majority of potential errors in the function without the length that would be required for a strictly comprehensive test suite.The
tests/unit_tests/testsfile contains the necessary content for the unit tests to be run through rook by runningFORCE/run_tests. Rook does not have a significant role in the testing itself; each test is run as a "generic executable" using theunittestmodule, which reports back a failure, success, or error. Output is handled by, and thus formatted by, rook. An advantage to this system is that rook will run both unit and integration tests withrun_tests.The
tests/unit_tests/README.mdfile contains a description of what each individual unit test is intended to check.For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.