search

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
222 stars 87 forks source link

Avoid using global app in tests/test_plugins.py #1893

Open drewj-tp opened 2 days ago

drewj-tp commented 2 days ago

In tests/test_plugins.py we have these setup/teardown methods

https://github.com/terrapower/armi/blob/f0b1accb58243001b83d7f9f4c619c9dfcc84c15/armi/tests/test_plugins.py#L45-L58

They pull in the global App, copy it, and then restore it at the end of the tests. I believe it would be better for us to not tests modify or even copy the global app, because it's global and something somewhere could make changes to that app that impact the tests. More in the spirit of moving away from globals where possible.

These tests are really focused on ensuring plugin registry methods do what we expect them to do on the application that registers the plugins. Can we find a way to make an app-like-thing, or even just a straight up pluggy.PluginManager that registers the hooks and plugins?

john-science commented 1 day ago

I don't understand. Sorry.

It is not possible to run most ARMI features without the global _app instantiated.

So... we can't run unit tests without an ARMI App, right?

drewj-tp commented 1 day ago

I don't understand. Sorry.

It is not possible to run most ARMI features without the global _app instantiated.

So... we can't run unit tests without an ARMI App, right?

No you're totally right. I didn't put enough context in the ticket. I've updated the body to focus specifically on tests/test_plugins.py that do the deepcopying of the global app, then register plugins on the copy.

john-science commented 13 hours ago

So, the concern for testing plugins is you want to be able to add some good (and badly broken) plugins to test.

But you can't load a plugin to test it without an ARMI application.

And you don't want to have a dozen wildly broken plugins lying in the same base App that all the other unit tests in ARMI share. (Because, necessarily, all the ARMI unit tests SHARE one App.)

So, for testing Plugins specifically (and maybe Interfaces), you need a special, one-off way to hide load plugins into the test ARMI App, without destroying the rest of the unit tests in ARMI.

TLDR, I am... not sure what this test is doing is so bad. It's two lines of code that side step a really clear and distinct problem.

Should there be a comment here about this? Maybe.

But the only solution would be a high-level redesign of ARMI to allow multiple Apps in memory at the same time. I am working on this, but it is a combined effort of maybe a dozen big tickets I've closed and another dozen big tickets that are left. It won't happen in the next 12 months, if I'm being honest.