Cleanup test suite #161

yardasol · 2022-07-28T03:01:37Z

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)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Required for Merging

Associated Issues and PRs

Issue: Feature: Clean up test suite and make use of pytest.fixture and pytest.raises in the Test suite #155

Associated Developers

Dev: @munkm

Checklist for Reviewers

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

…code-specific directories; rename integration tests

pep8speaks · 2022-07-28T03:01:42Z

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! 🍻

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

yardasol · 2022-08-29T19:12:27Z

@munkm bump

munkm

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.

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.

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.

munkm · 2022-09-12T17:03:54Z

doc/installation.rst


+from the root directory of SaltProc.


If one wants to run just the unit tests and not the integration tests, how would they do that here?

You can specify the path to the test(s) you want when calling pytest. Alternatively, you can use the --ignore flag followed by the path(s) to the test(s) you don't want to test.

Ok, I think that's worth explicitly stating here for people who may want to run unit tests locally when doing development. Can you please add it back in with the updated paths?

doc/releasenotes/v0.5.0.rst

pytest.ini

munkm · 2022-09-13T04:10:42Z

saltproc/input_schema.json

@@ -102,7 +98,7 @@
                                        "pattern": "^\\.\\/(.*)\\.xml$"}
                                },

-                                "required": ["settings", "geometry", "materials", "chain_file"]
+                                "required": ["settings", "materials", "chain_file"]


so a geometry file isn't required any more? Won't that cause some issues?

A geometry file is required, but we have an entirely separate object for that, geo_files, so we don't need to duplicate that.

I see. Ok, so I like the new geo_files object, but do you think it might be less intuitive that the geometry settings file path is not included in this input schema? All of the other paths are defined here. Would it potentially make it more difficult to read?

It may be confusing at first, but that's just how SaltProc is structured. We could try to change that in the future, but in the context of SaltProc's current structure, I think it would be more confusing for users to have two different geometry file specification parameters, because then which one are they supposed to use?

Ok, that makes sense!

tests/conftest.py

tests/unit_tests/test_materialflow.py

tests/integration_tests/basic_reprocessing/test.py

tests/integration_tests/run_constant_reprocessing/test.py

munkm · 2022-09-13T04:57:43Z

tests/integration_tests/run_constant_reprocessing/test.py

+    np.testing.assert_allclose(test_param, ref_param, rtol=tol)
+
+
+@pytest.mark.slow


Maybe say what this decorator is doing so future devs can use it appropriately if they add tests.

I'm actually not sure what it does. It was just in the previous version of the test suite.

It tells pytest to run this "slow test" after all others. I.e. priority = last

tests/integration_tests/run_no_reprocessing/test.py

Co-authored-by: Madicken Munk <madicken.munk@gmail.com>

yardasol · 2022-09-13T21:58:06Z

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.

munkm

Thanks for your responses @yardasol ! I tried to clarify some of my earlier questions since they weren't super obvious.

Which files are you talking about?

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 don't know. I've never used wheel.

I think that moving them here means they won't be distributed if we publish the package on pypi or conda-forge. That's ok (there are packages that choose to not do that, after all), but I think it's worth thinking about. Also, apparently the former convention seems to be a remnant from earlier rules around packaging . Anyways, we can always change it back if we encounter issues with this new structure.

tests/integration_tests/file_interface_openmc/test.py

munkm · 2022-09-13T21:59:56Z

doc/installation.rst


+from the root directory of SaltProc.


Ok, I think that's worth explicitly stating here for people who may want to run unit tests locally when doing development. Can you please add it back in with the updated paths?

doc/releasenotes/v0.5.0.rst

pytest.ini

tests/conftest.py

tests/integration_tests/run_no_reprocessing/test.py

tests/unit_tests/test_depcode_serpent.py

munkm · 2022-09-27T17:25:17Z

saltproc/input_schema.json

@@ -102,7 +98,7 @@
                                        "pattern": "^\\.\\/(.*)\\.xml$"}
                                },

-                                "required": ["settings", "geometry", "materials", "chain_file"]
+                                "required": ["settings", "materials", "chain_file"]


I see. Ok, so I like the new geo_files object, but do you think it might be less intuitive that the geometry settings file path is not included in this input schema? All of the other paths are defined here. Would it potentially make it more difficult to read?

yardasol · 2022-09-27T21:29:45Z

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 · 2022-10-10T18:51:23Z

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

munkm · 2022-10-11T21:12:04Z

tools/ci/build-openmc.sh

+#cp -r /usr/local/share/openmc openmc_src/share/.
+#cp -r /usr/local/share/man openmc_src/share/.


We can make a good first issue for somebody to remove these. I think if they're unused, we should just delete them and not comment them out.

…tests Cleanup test suite 71b6e08

…nup-tests Cleanup test suite 71b6e08

yardasol and others added 18 commits February 21, 2022 14:21

split up the read_main_input function

315849e

rename the helper function for read_main_input

f13237e

split up app.run()

9407d2f

split up app.run()

02cc71b

Merge branch 'master' into cleaning-exercise

ea39880

add back in missing content from master merge

a96216c

move tests directory outside of the saltproc module

fc0b0ae

move unit tests to their own directory; move input and data files to …

22da911

…code-specific directories; rename integration tests

split test_app into unit test and integration test components

cb5688e

move run to top of file

eb1adce

remove global variables; rename variables

d8594a4

add arguments to some of the app functions

7ff3ee4

Merge branch 'cleaning-exercise' into cleanup-tests

7c1536e

reorder functions

d3d56ec

replace os.path with pathlib.Path; fix import statements

019eed6

get test_app unit test working

2679fbf

move pytest.ini to repo base

c804f8c

remove tests packages from saltproc package

ff31b67

add missing h5 files

3188df9

yardasol mentioned this pull request Jul 28, 2022

Cleaning exercise #137

Closed

16 tasks

yardasol changed the title ~~Cleanup tests suite~~ Cleanup test suite Jul 28, 2022

yardasol added 3 commits July 27, 2022 22:13

add openmc option to _create_depcode_object

3749f09

fix _create_simulation_object function signature

327118b

cleanup run_no_reproc

7eb9e1d

yardasol force-pushed the cleanup-tests branch 2 times, most recently from 56aabaa to c42be3c Compare July 28, 2022 23:51

yardasol and others added 3 commits July 28, 2022 18:52

cleanup run_no_reproc

56aabaa

Merge remote-tracking branch 'origin/cleanup-tests' into cleanup-tests

9225563

cleanup run_no_reproc

c42be3c

fix new syntax inconsistencies

ba49bca

yardasol requested a review from LukeSeifert September 8, 2022 18:54

munkm reviewed Sep 13, 2022

View reviewed changes

yardasol and others added 6 commits September 13, 2022 16:35

remove cruft terminal output

49be51f

process_input_file -> process_file; path_input_file -> dot_file

c0bfe49

Add descriptions for object fixtures

4d849ca

remove old comments

94e749d

Remov cruft pytest marker

bba890f

Co-authored-by: Madicken Munk <madicken.munk@gmail.com>

Merge remote-tracking branch 'origin/cleanup-tests' into cleanup-tests

9b06262

yardasol requested a review from munkm September 13, 2022 21:58

reorder function definitions to be consitent w style of software

a1315fc

munkm reviewed Sep 27, 2022

View reviewed changes

expand testing instructions

3a6135a

Merge remote-tracking branch 'origin/cleanup-tests' into cleanup-tests

c0bcedd

yardasol requested a review from munkm October 4, 2022 16:31

fix test input file names

8e2192b

yardasol force-pushed the cleanup-tests branch from 5eed7cc to 8e2192b Compare October 4, 2022 16:47

remove paths to share dir is tools/ci scripts

51e7a8d

munkm approved these changes Oct 11, 2022

View reviewed changes

munkm merged commit 71b6e08 into arfc:master Oct 11, 2022

github-actions bot pushed a commit that referenced this pull request Oct 11, 2022

update docs at commit: Merge pull request #161 from yardasol/cleanup-…

1d1d577

…tests Cleanup test suite 71b6e08

github-actions bot pushed a commit to khurrumsaleem/saltproc that referenced this pull request Oct 12, 2022

update docs at commit: Merge pull request arfc#161 from yardasol/clea…

8c57e8c

…nup-tests Cleanup test suite 71b6e08

github-actions bot pushed a commit to yardasol/saltproc that referenced this pull request Nov 3, 2022

update docs at commit: Merge pull request arfc#161 from yardasol/clea…

b7d135d

…nup-tests Cleanup test suite 71b6e08

yardasol mentioned this pull request Jan 9, 2023

Feature: Clean up test suite and make use of pytest.fixture and pytest.raises in the Test suite #155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup test suite #161

Cleanup test suite #161

yardasol commented Jul 28, 2022 •

edited

Loading

pep8speaks commented Jul 28, 2022 •

edited

Loading

yardasol commented Aug 29, 2022

munkm left a comment

munkm Sep 12, 2022

yardasol Sep 13, 2022

munkm Sep 13, 2022

munkm Sep 13, 2022

yardasol Sep 13, 2022 •

edited

Loading

munkm Sep 27, 2022

yardasol Sep 27, 2022 •

edited

Loading

munkm Sep 28, 2022

munkm Sep 13, 2022

yardasol Sep 13, 2022

samgdotson Sep 28, 2022

yardasol commented Sep 13, 2022

munkm left a comment •

edited

Loading

munkm Sep 13, 2022

munkm Sep 27, 2022

yardasol commented Sep 27, 2022

yardasol commented Oct 10, 2022

munkm Oct 11, 2022

		np.testing.assert_allclose(test_param, ref_param, rtol=tol)


		@pytest.mark.slow

		#cp -r /usr/local/share/openmc openmc_src/share/.
		#cp -r /usr/local/share/man openmc_src/share/.

Cleanup test suite #161

Cleanup test suite #161

Conversation

yardasol commented Jul 28, 2022 • edited Loading

Summary of changes

Types of changes

Required for Merging

Associated Issues and PRs

Associated Developers

Checklist for Reviewers

pep8speaks commented Jul 28, 2022 • edited Loading

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

yardasol commented Aug 29, 2022

munkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol commented Sep 13, 2022

munkm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardasol commented Sep 27, 2022

yardasol commented Oct 10, 2022

Choose a reason for hiding this comment

yardasol commented Jul 28, 2022 •

edited

Loading

pep8speaks commented Jul 28, 2022 •

edited

Loading

yardasol Sep 13, 2022 •

edited

Loading

yardasol Sep 27, 2022 •

edited

Loading

munkm left a comment •

edited

Loading