Cleanup test suite

[yardasol]

Summary of changes

This PR cleans up and overhauls the test suite to increase readability and future extensibility.

Specifically, this PR:

• eliminates the saltproc.tests package and moves the tests directory to the base directory of the repo

• moves pytest.ini to the base directory of the repo

• adds a conftest.py file to tests/

• Makes the following changes to the source files:

  • in app.py:
  • Eliminates global variables and adds parameters and return variables to functions previously missing them to facilitate proper unit testing
    • read_main_input()
    • _print_simulation_input_info()
    • _create_depcode_object()
    • _create_simulation_object()
    • _create_reactor_object()
    • _process_main_input_reactor_params()
    • reprocess_materials()
    • get_paths()
    • refill_materials()
  • in depcode.py:
  • Remove materials.xml from default value for template_input_file_path

• store depletion-code specific inputs data in new directories:

  • serpent_data
  • openmc_data

• Changes to integration tests

  • runsim_no_reproc -> run_no_reprocessing: make datafile naming consistent, rely on a saltproc input file instead of module-level variables, internal variable name adjusments.
  • cons_reproc -> run_constant_reprocessing: make datafile naming consistent, internal variable name adjustments, use previously implemented but unused functions for testing reference results and test results.
  • (new) -> simple_reprocessing: reprocessing-related tests from test_app.py
  • (new) -> database_storage: database related tests from test-simulation.py
  • (new) -> file_interface_openmc: OpenMC file-related tests from test_depcode.py
  • (new) -> file_interface_serpent: Serpent file-related tests from test_depcode.py

• Changes to specific unit tests:

  • test_app.py: Move testing of refill and reprocessing to an integration test, simple_reprocessing
  • test_depcode.py: Move testing of file writing to integration tests, file_interface_openmc and file_interface_serpent; split up into serpent and openmc tests.
  • test_ materialflow.py: Simplify file, syntax updates
  • test_process.py: Simplify file, syntax updates
  • test_separator.py: Simplify file, syntax updates
  • test_simulation.py: Move testing of database accuracy to an integration test, database_storage
  • test_sparger.py: Simplify file, syntax updates

• updates references to changed function names in the docpages with their new names.

  Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)

• [x] New feature (non-breaking change which adds functionality)

• [x] Breaking change (fix or feature that would cause existing functionality to change)

Required for Merging

• [x] I have read the CONTRIBUTING document.

• [x] My code follows the code style of this project.

• [x] My change requires a change to the documentation.

  • [x] I have updated the documentation accordingly.

• [x] My change is a source code change

  • [x] I have added/modified tests to cover my changes
  • [ ] I have explained why my change does not require new/modified tests

• [x] My change is a user-facing change

  • [x] I have recorded my changes in the changelog for the upcoming release

• [ ] My change is exclusively related to the repository (e.g. GitHub actions workflows, PR/issue templates, etc.)

  • [ ] I have verified or justified that my change will work as intended.

• [x] All new and existing tests passed.

  • [ ] CI tests pass
  • [x] Local tests pass (including Serpent2 integration tests)

    Associated Issues and PRs

• Issue: #155

Associated Developers

• Dev: @munkm

Checklist for Reviewers

Reviewers should use this link to get to the Review Checklist before they begin their review.

[pep8speaks]

Hello @yardasol! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2022-08-29 19:11:54 UTC

[samgdotson]

"68 files changed" 👁️ 👄 👁️

[yardasol]

This all looks fine to me. I made a take-it-or-leave-it suggestion in part of the documentation. Otherwise, my only comment is if you're moving files to use git mv, which I think would reduce the total number of files in the PR.

That's a great suggestion about git mv! I hadn't really thought about the implications of not using that. Will do this in the future.

I'd like at least one more review from @munkm or @smpark7 since this is a pretty major overhaul of the test suite.

[yardasol]

@munkm bump

[yardasol]

Ok, I think I'm done reviewing now! Thank you so much for these changes @yardasol !! I think there are lots of great improvements to the test suite here.

Thanks!

I am a little worried about calling the files used to and in the serpent/openmc runs test_, as it sort of implies that's a file that's doing the test, not used by the test. Does that make sense? Maybe we could call them sample_ or something? I'm trying to come up with an alternative that would make them more differentiable from the actual test files.

Which files are you talking about?

Do you think there could be any issues by moving the tests folder out of the saltproc dir? I think this means it could be harder for a wheel build install to run the tests (which we don't use yet so it's not an issue) but I haven't double checked that so I could be wrong.

I don't know. I've never used wheel.

[yardasol]

Thanks for the review @munkm

I'm talking about the .ini and .json files that start with test_. Since they are specific designs and configurations, maybe we could name them with the functionality first, then say they're for tests. That way it doesn't seem like those files alone are test files. e.g. something like msr_input_test .

I somewhat agree with you, and somewhat don't. I think it's perfectly valid to prefix the input files with test_ because that elucidates their purpose (the intended purpose is that they are associated with tests). I'm not convinced that renaming the files to correspond to a specific design/configuration is particularly useful because we only have one such design/configuration right now for our unit tests. If down the road we are working with more than one configuration/design, then I think it makes sense to add more detail to those file names.

[yardasol]

@munkm if you don't have any further comments, can this be merged?