terrapower / armi

An open-source nuclear reactor analysis automation framework that helps design teams increase efficiency and quality
https://terrapower.github.io/armi/
Apache License 2.0
233 stars 90 forks source link

Add Guide to Docs for ARMI Testing Tools #1746

Open john-science opened 5 months ago

john-science commented 5 months ago

Based on recent questions I've gotten, I think it would be useful to add a guide to the ARMI-specific testing tools to the "dev" section of the ARMI docs. Below is a first-draft that @opotowsky wrote previously.

ARMI Testing Tools

ARMI has many useful tools to streamline tests in the plugins. Included here are some popular ones, or at least ones that should be popular.

If you're trying to write a new unit test, chances are something LIKE it has been done before and you don't need to design it from scratch. Look around ARMI and other plugins for examples of tests.

Testing with runLog

Use Case: Test code that prints to stdout

While there are some other mocking examples in ARMI, none are as heavily used as mockRunLogs. mockRunLogs.BufferLog() is used to capture the runLog output instead of printing it.

In test_comparedb3.py, there is a (simplified here) use case. A portion of the test for _diffSpecialData wants to confirm the below printout has happened, so it uses the getStdout() method to check that the expected printout exists.

getStdout Example
def test_diffSpecialData(self):
    dr = DiffResults(0.01)
    fileName = "test.txt"
    with OutputWriter(fileName) as out:
        with mockRunLogs.BufferLog() as mock:
            #... skip for clarity: create refData & srcData
            _diffSpecialData(refData, srcData, out, dr)
            self.assertEqual(dr.nDiffs(), 0)
            self.assertIn("Special formatting parameters for", mock.getStdout())

There are examples of this throughout ARMI, but sparsely (so far) in plugins. Search for BufferLog or getStdout in the code to find examples.

Self-Cleaning Directories

Use Case: Automatically cleans up tests that create files

from armi.utils.directoryChangers import TemporaryDirectoryChanger

Two main uses of this class in testing:

  1. Standalone test that calls code that creates something (test_operators.py)

https://github.com/terrapower/armi/blob/2bcb03689954ae39f3044f18a9a77c1fb7a0e63b/armi/operators/tests/test_operators.py#L237-L242

  1. Setup and teardown of a testing class, where all/most of the tests create something (test_comparedb3.py)

https://github.com/terrapower/armi/blob/2bcb03689954ae39f3044f18a9a77c1fb7a0e63b/armi/bookkeeping/db/tests/test_comparedb3.py#L36-L52

Note that sometimes it's necessary to give the temporary directory change object a non-default root path:

Include root argument
THIS_DIR = os.path.dirname(__file__)
.
.
.
def test_something():
    with TemporaryDirectoryChanger(root=THIS_DIR): 
        # test something

Load a Test Reactor

Use Case: need a full reactor. Warning: Expensive, and possibly over-used. Consider whether mocking or BYO components (below) can be used instead.

from armi.reactor.tests.test_reactors import loadTestReactor Returns an operator and a reactor:

https://github.com/terrapower/armi/blob/342d2c9ffa8ff36b190b237b465ed38be08efeae/armi/reactor/tests/test_reactors.py#L142-L146

So many interfaces and methods require an operator, a reactor, or both. Check out this example in test_fluxReconstructor.py:

And a test in that class using fuel blocks from the loaded reactor (and a FluxReconstructor method from the operator):

Sidebar: speed up tests using test reactor If using, maybe you can speed up unit tests by reducing the number of rings.

The database interface needs a reactor to load, so the setUp of a class in test_database3.py reduces the size of the reactor ahead of the tests:

Empty Reactors

from armi import tests

  1. getEmptyHexReactor()
  2. getEmptyCartesianReactor()

BYO Blocks & Assemblies

Use Case: likes speedy tests and doesn't need a full reactor

from armi.reactor.tests import test_blocks buildSimpleFuelBlock() : returns a block with no reactor/assembly lineage

https://github.com/terrapower/armi/blob/2bcb03689954ae39f3044f18a9a77c1fb7a0e63b/armi/reactor/tests/test_blocks.py#L46-L47

loadtestBlock(): returns a block in an assembly in a reactor

https://github.com/terrapower/armi/blob/2bcb03689954ae39f3044f18a9a77c1fb7a0e63b/armi/reactor/tests/test_blocks.py#L79-L80

... block.add(stuff) ...

https://github.com/terrapower/armi/blob/2bcb03689954ae39f3044f18a9a77c1fb7a0e63b/armi/reactor/tests/test_blocks.py#L238-L243

from armi.reactor.tests import test_assemblies

buildTestAssemblies()

https://github.com/terrapower/armi/blob/2bcb03689954ae39f3044f18a9a77c1fb7a0e63b/armi/reactor/tests/test_assemblies.py#L54-L61

makeTestAssembly()

https://github.com/terrapower/armi/blob/2bcb03689954ae39f3044f18a9a77c1fb7a0e63b/armi/reactor/tests/test_assemblies.py#L182-L190

Miscellaneous Miscellany

Here are some test files we felt might be useful for people to search to help with test development in plugins.

andfranklin commented 4 months ago

RE: Self-Cleaning Directories

A lot of the use cases from TemporaryDirectoryChanger could be covered by using pytest's tmp_path fixture in combination with os.chdir. I'd be willing to bet that simple workarounds could be found for the "non-default root path" use case(s).

Some possible side benefits of this alternative approach:

Aside from historical inertia, is there a strong reason why ARMI would like to continue to maintain TemporaryDirectoryChanger?

andfranklin commented 4 months ago

RE: Load a Test Reactor

I'm working on something for this. It'll be able to cache test reactors for quick reloading between test runs. It'll also be fully compatible with xdist -- NO RACE CONDITIONS! 🎉

It'll be ready for show-time in a week or two. I'm just writing-up documentation and developing realistic benchmark cases.

If it's accepted by ARMI, then loadTestReactor could be greatly simplified. I also have an idea for how to close #1636.

jakehader commented 4 months ago

Thanks for the help on this @andfranklin!

jakehader commented 4 months ago

RE: Self-Cleaning Directories

A lot of the use cases from TemporaryDirectoryChanger could be covered by using pytest's tmp_path fixture in combination with os.chdir. I'd be willing to bet that simple workarounds could be found for the "non-default root path" use case(s).

Some possible side benefits of this alternative approach:

Aside from historical inertia, is there a strong reason why ARMI would like to continue to maintain TemporaryDirectoryChanger?

Plugins often use a temporary directory changer outside of testing to run external programs that produce their own temporary files. When the temporary directory is closed after the work is done only some a subset of files are returned.

opotowsky commented 4 months ago

Aside from historical inertia, is there a strong reason why ARMI would like to continue to maintain TemporaryDirectoryChanger

It's used in the code as well as the tests

opotowsky commented 4 months ago

RE: Testing with runLog

Would pytest's caplog fixture solve this?

As far as I can tell, runLog is using python's logging module under the hood. It seems like caplog would be sufficient for testing the logs produced by the runLog. If that's true, then a lot of test code be simplified, and some of the helper functions could probably be deleted.

Maybe there's something that I'm not aware of 🤷‍♂️

I think refactoring the logging used for hundreds of tests is probably out of scope for this ticket.

opotowsky commented 4 months ago

RE: Load a Test Reactor

I'm working on something for this. It'll be able to cache test reactors for quick reloading between test runs. It'll also be fully compatible with xdist -- NO RACE CONDITIONS! 🎉

It'll be ready for show-time in a week or two. I'm just writing-up documentation and developing realistic benchmark cases.

If it's accepted by ARMI, then loadTestReactor could be greatly simplified. I also have an idea for how to close #1636.

This sounds awesome!

john-science commented 4 months ago

RE: Testing with runLog

Would pytest's caplog fixture solve this?

Sorry, what I was saying is we already HAVE a solution for this. mockRunLogs is used in hundreds of tests.

john-science commented 4 months ago

RE: Load a Test Reactor

I'm working on something for this. It'll be able to cache test reactors for quick reloading between test runs. It'll also be fully compatible with xdist -- NO RACE CONDITIONS! 🎉

It'll be ready for show-time in a week or two. I'm just writing-up documentation and developing realistic benchmark cases.

If it's accepted by ARMI, then loadTestReactor could be greatly simplified. I also have an idea for how to close #1636.

This ticket is NOT about features we don't have. If you have a feature request or idea, that would be a different ticket.

The point of this ticket is to write documentation for testing tools we already have in ARMI.

andfranklin commented 4 months ago

This ticket is NOT about features we don't have. If you have a feature request or idea, that would be a different ticket.

The point of this ticket is to write documentation for testing tools we already have in ARMI.

Understood, just trying to provide a different perspective of what could be. To me it seems like ARMI could relieve some of its maintenance burden if different practices were adopted. Maybe there's a reason why things are the way that they are for reasons that I'm not aware of 🤷‍♂️

andfranklin commented 4 months ago

I think refactoring the logging used for hundreds of tests is probably out of scope for this ticket.

That's absolutely fair 😆 I wasn't trying to suggest that.

andfranklin commented 4 months ago

This sounds awesome!

It will be! I'm excited to get feedback from y'all. We'll get to it soon.

andfranklin commented 4 months ago

Plugins often use a temporary directory changer outside of testing to run external programs that produce their own temporary files. When the temporary directory is closed after the work is done only some a subset of files are returned.

Ah that makes sense, thanks @jakehader and @opotowsky. Sounds like #1780 actually needs to be addressed. I'll see if I can help with that.

john-science commented 2 months ago

@opotowsky Do you want to take this ticket, or should I? I figure it should be one of us.

opotowsky commented 2 months ago

@opotowsky Do you want to take this ticket, or should I? I figure it should be one of us.

I can take it but I'm probably full up for work this week. So it would be later. (I'm happy to do it though)

john-science commented 2 months ago

@opotowsky Do you want to take this ticket, or should I? I figure it should be one of us.

I can take it but I'm probably full up for work this week. So it would be later. (I'm happy to do it though)

There's no rush. I wouldn't get to it this week either. If you want it, self-assign. Otherwise, just assign it to me. #shrug NBD either way.