Cleaning up UserPlugins (#798)

Fixing some issues with UserPlugins:

- In armi/apps.py there was a Windows Pathing bug.
- In docs/ there were some typos to do with UserPlugins.
- In armi/tests/ there were some typos in the UserPlugin unit tests.

Because these changes removed code, the code coverage went down. So some unrelated code coverage tests were added to HistoryTracker and pathTools, just so the build would pass.

armi/apps.py

@@ -31,7 +31,6 @@
 from typing import Dict, Optional, Tuple, List
 import collections
 import importlib
-import os
 import sys
 
 from armi import context, plugins, pluginManager, meta, settings
@@ -263,7 +262,7 @@ def registerUserPlugins(self, pluginPaths):
         self.__initNewPlugins()
 
         for pluginPath in pluginPaths:
-            if os.sep in pluginPath:
+            if ".py:" in pluginPath:
                 # The path is of the form: /path/to/why.py:MyPlugin
                 self.__registerUserPluginsAbsPath(pluginPath)
             else:
@@ -274,19 +273,11 @@ def __registerUserPluginsAbsPath(self, pluginPath):
         """Helper method to register a single UserPlugin where
         the given path is of the form: /path/to/why.py:MyPlugin
         """
-        # determine if this is a Windows system
-        isWindows = False
-        if os.name == "nt":
-            isWindows = True
-
-        # handle the minor variations on Windows file pathing
-        if isWindows:
-            assert pluginPath.count(":") == 2, f"Invalid plugin path: {pluginPath}"
-            drive, filePath, className = pluginPath.split(":")
-            filePath = drive + ":" + filePath
-        else:
-            assert pluginPath.count(":") == 1, f"Invalid plugin path: {pluginPath}"
-            filePath, className = pluginPath.split(":")
+        assert pluginPath.count(".py:") == 1, f"Invalid plugin path: {pluginPath}"
+
+        # split the settings string into file path and class name
+        filePath, className = pluginPath.split(".py:")
+        filePath += ".py"
 
         spec = importlib.util.spec_from_file_location(className, filePath)
         mod = importlib.util.module_from_spec(spec)

armi/bookkeeping/tests/test_historyTracker.py

@@ -19,12 +19,12 @@
 a valid HDF5 file to load from as a test fixtures. Thus they take a little longer
 than usual.
 """
+# pylint: disable=missing-function-docstring,missing-class-docstring,protected-access
 import os
 import pathlib
 import shutil
 import unittest
 
-from armi import init as armi_init
 from armi import settings
 from armi import utils
 from armi.bookkeeping import historyTracker
@@ -35,14 +35,15 @@
 from armi.reactor import grids
 from armi.reactor.flags import Flags
 from armi.tests import ArmiTestHelper
-from armi.utils import directoryChangers
+from armi.utils.directoryChangers import TemporaryDirectoryChanger
 
 CASE_TITLE = "anl-afci-177"
 THIS_DIR = os.path.dirname(__file__)  # b/c tests don't run in this folder
 TUTORIAL_DIR = os.path.join(ROOT, "tests", "tutorials")
 
 
 def runTutorialNotebook():
+    # pylint: disable=import-outside-toplevel
     import nbformat
     from nbconvert.preprocessors import ExecutePreprocessor
 
@@ -57,35 +58,14 @@ class TestHistoryTracker(ArmiTestHelper):
 
     @classmethod
     def setUpClass(cls):
-        # Not using a directory changer since it isn't important that we go back in the
-        # first place, and we don't want to get tangled with the directory change below.
-        # We need to be in the TUTORIAL_DIR in the first place so that for `filesToMove`
-        # to work right.
+        # We need to be in the TUTORIAL_DIR so that for `filesToMove` to work right.
         os.chdir(TUTORIAL_DIR)
 
-        # Make sure to do this work in a temporary directory to avoid race conditions
-        # when running tests in parallel with xdist.
-        cls.dirChanger = directoryChangers.TemporaryDirectoryChanger(
-            filesToMove=TUTORIAL_FILES
-        )
+        # Do this work in a temp dir, to avoid race conditions.
+        cls.dirChanger = TemporaryDirectoryChanger(filesToMove=TUTORIAL_FILES)
         cls.dirChanger.__enter__()
         runTutorialNotebook()
 
-        reloadCs = settings.Settings(f"{CASE_TITLE}.yaml")
-
-        newSettings = {}
-        newSettings["db"] = True
-        newSettings["reloadDBName"] = pathlib.Path(f"{CASE_TITLE}.h5").absolute()
-        newSettings["runType"] = "Snapshots"
-        newSettings["loadStyle"] = "fromDB"
-        newSettings["detailAssemLocationsBOL"] = ["001-001"]
-
-        reloadCs = reloadCs.modified(newSettings=newSettings)
-        reloadCs.caseTitle = "armiRun"
-
-        o = armi_init(cs=reloadCs)
-        cls.o = o
-
     @classmethod
     def tearDownClass(cls):
         cls.dirChanger.__exit__(None, None, None)
@@ -94,13 +74,13 @@ def setUp(self):
         cs = settings.Settings(f"{CASE_TITLE}.yaml")
         newSettings = {}
         newSettings["db"] = True
-        newSettings["reloadDBName"] = pathlib.Path(f"{CASE_TITLE}.h5").absolute()
-        newSettings["loadStyle"] = "fromDB"
         newSettings["detailAssemLocationsBOL"] = ["001-001"]
+        newSettings["loadStyle"] = "fromDB"
+        newSettings["reloadDBName"] = pathlib.Path(f"{CASE_TITLE}.h5").absolute()
         newSettings["startNode"] = 1
         cs = cs.modified(newSettings=newSettings)
 
-        self.td = directoryChangers.TemporaryDirectoryChanger()
+        self.td = TemporaryDirectoryChanger()
         self.td.__enter__()
 
         c = case.Case(cs)

armi/operators/tests/test_operators.py

@@ -88,7 +88,7 @@ def test_interfaceIsActive(self):
         self.assertTrue(o.interfaceIsActive("main"))
         self.assertFalse(o.interfaceIsActive("Fake-o"))
 
-    def test_loadState(self):
+    def test_loadStateError(self):
         """The loadTestReactor() test tool does not have any history in the DB to load from"""
         o, _r = test_reactors.loadTestReactor()
 

armi/tests/test_plugins.py

@@ -53,11 +53,12 @@ def test_exposeInterfaces(self):
         """Make sure that the exposeInterfaces hook is properly implemented"""
         if self.plugin is None:
             return
-        if not hasattr(self.plugin, "exposeInterfaces"):
-            return
 
         cs = settings.getMasterCs()
         results = self.plugin.exposeInterfaces(cs)
+        if results is None or not results:
+            return
+
         # each plugin should return a list
         self.assertIsInstance(results, list)
         for result in results:

armi/tests/test_user_plugins.py

@@ -34,26 +34,29 @@
 from armi.utils import directoryChangers
 
 
-HOOKSPEC = pluggy.HookspecMarker("armi")
-
-
 class UserPluginFlags(plugins.UserPlugin):
     """Simple UserPlugin that defines a single, new flag."""
 
+    @staticmethod
+    @plugins.HOOKIMPL
     def defineFlags():
         return {"SPECIAL": utils.flags.auto()}
 
 
 class UserPluginFlags2(plugins.UserPlugin):
     """Simple UserPlugin that defines a single, new flag."""
 
+    @staticmethod
+    @plugins.HOOKIMPL
     def defineFlags():
         return {"FLAG2": utils.flags.auto()}
 
 
 class UserPluginFlags3(plugins.UserPlugin):
     """Simple UserPlugin that defines a single, new flag."""
 
+    @staticmethod
+    @plugins.HOOKIMPL
     def defineFlags():
         return {"FLAG3": utils.flags.auto()}
 
@@ -64,6 +67,8 @@ def defineFlags():
 from armi import utils
 
 class UserPluginFlags4(plugins.UserPlugin):
+    @staticmethod
+    @plugins.HOOKIMPL
     def defineFlags():
         return {"FLAG4": utils.flags.auto()}
 """
@@ -72,13 +77,17 @@ def defineFlags():
 class UserPluginBadDefinesSettings(plugins.UserPlugin):
     """This is invalid/bad because it implements defineSettings()"""
 
+    @staticmethod
+    @plugins.HOOKIMPL
     def defineSettings():
         return [1, 2, 3]
 
 
 class UserPluginBadDefineParameterRenames(plugins.UserPlugin):
     """This is invalid/bad because it implements defineParameterRenames()"""
 
+    @staticmethod
+    @plugins.HOOKIMPL
     def defineParameterRenames():
         return {"oldType": "type"}
 
@@ -91,7 +100,7 @@ class UserPluginOnProcessCoreLoading(plugins.UserPlugin):
     """
 
     @staticmethod
-    @HOOKSPEC
+    @plugins.HOOKIMPL
     def onProcessCoreLoading(core, cs):
         blocks = core.getBlocks(Flags.FUEL)
         for b in blocks:

armi/utils/pathTools.py

@@ -100,20 +100,7 @@ def isAccessible(path):
     path : str
         a directory or file
     """
-    if os.path.exists(path):
-        # This can potentially return a false positive in Python 2 if the path
-        # exists but the user does not have access. As a workaround, we attempt
-        # to list the contents of the containing directory, which will throw an
-        # OSError if the user doesn't have access.
-        try:
-            if not os.path.isdir(path):
-                path = os.path.dirname(path)
-            os.listdir(path)
-            return True
-        except OSError:
-            return False
-    else:
-        return False
+    return os.path.exists(path)
 
 
 def separateModuleAndAttribute(pathAttr):

armi/utils/tests/test_pathTools.py

@@ -17,6 +17,7 @@
 """
 # pylint: disable=missing-function-docstring,missing-class-docstring,abstract-method,protected-access,no-member,disallowed-name,invalid-name
 import os
+import time
 import types
 import unittest
 
@@ -120,6 +121,29 @@ def test_cleanPathNoMpi(self):
             context.waitAll()
             self.assertFalse(os.path.exists(dir3))
 
+    def test_isFilePathNewer(self):
+        with TemporaryDirectoryChanger():
+            path1 = "test_isFilePathNewer1.txt"
+            with open(path1, "w") as f1:
+                f1.write("test1")
+
+            time.sleep(1)
+
+            path2 = "test_isFilePathNewer2.txt"
+            with open(path2, "w") as f2:
+                f2.write("test2")
+
+            self.assertFalse(pathTools.isFilePathNewer(path1, path2))
+            self.assertTrue(pathTools.isFilePathNewer(path2, path1))
+
+    def test_isAccessible(self):
+        with TemporaryDirectoryChanger():
+            path1 = "test_isAccessible.txt"
+            with open(path1, "w") as f1:
+                f1.write("test")
+
+            self.assertTrue(pathTools.isAccessible(path1))
+
 
 if __name__ == "__main__":
     unittest.main()

doc/tutorials/making_your_first_app.rst

@@ -392,17 +392,15 @@ This can be done by sublassing :py:class:`armi.plugins.UserPlugin`:
 
     class UserPluginExample(plugins.UserPlugin):
         """
-        This plugin flex-tests the onProcessCoreLoading() hook,
-        and arbitrarily adds "1" to the height of every block,
-        after the DB is loaded.
+        This plugin flex-tests the onProcessCoreLoading() hook, and
+        arbitrarily adds "1" to the power ever each fuel block.
         """
 
         @staticmethod
-        @HOOKSPEC
+        @plugins.HOOKIMPL
         def onProcessCoreLoading(core, cs):
-            blocks = core.getBlocks(Flags.FUEL)
-            for b in blocks:
-                b.p.height += 1.0
+        for b in core.getBlocks(Flags.FUEL):
+            b.p.power += 1.0
 
 In most ways, ``UserPluginExample`` above is just a normal
 :py:class:`ArmiPlugin <armi.plugins.ArmiPlugin>`. You can implement any of the normal
@@ -424,7 +422,7 @@ file:
 
 .. code-block::
 
-  userPlugins::
+  userPlugins:
     - armi.tests.test_user_plugins.UserPlugin0
     - //path/to/my/pluginz.py:UserPlugin1
     - C:\\path\to\my\pluginZ.py:UserPlugin2