Unit tests are manual, not unit test?

[brandondube]

This issue is part of my portion of the JOSS review, openjournals/joss-reviews#5478

Browsing a random unit test, test_Bessel.py, as best I can tell the test does not do what it says:

# Script for testing the far-field calculations of PyPO by calculating the far-field of 
# a uniformly illuminated disk, with varying wavelength, and calculating the HPBW. 
# This is then compared to the theoretical value of a Bessel function.

The last line of the comment -- the comparison, seems to happen but not in a quantified way. It looks like the assertion just tests whether the values are truth (not zero, NaN, inf, or -inf) - https://github.com/PyPO-dev/PyPO/blob/main/tests/test_Bessel.py#L33-L39

The plotting code seems to imply this is a test that a human runs, looks at a plot, and makes a decision on? That is a manual validation test, not a unit test

I looked at a couple of other tests, e.g. fit gauss, and some of them are truly unit tests. Please make all of the unit tests unit tests, and separate out the manual tests.

The tests also have a bad code smell,

from src.PyPO.System import System

You should never see this in python code, it should be from PyPO.System import System; the src. means this can only be done with cwd at the root of the repo, and implies an improperly installed package (related to #128)

[arendMoerman]

Thanks for the comments! We will get to this ASAP. The second author @MrGafaji has a busy week this week, but we plan to work on it this weekend. We will keep you updated on this issue!

[arendMoerman]

Thanks for the wait!

We have made all the tests automated as per the JOSS checklist. Also, tests such as the test_Bessel.py now do what they were supposed to do. The correct packaging of PyPO should have gotten rid of this particular code smell.
We also removed the parts , such as plotting code, that were implying manual testing in order to avoid confusion for people wanting to inspect the tests.

The instructions for running the testsuite can be found in the docs.
Instructions for generating a coverage overview, if desired, are included in the docs as well.

[brandondube]

Thank you for the updates. A pain point, the documentation suggests: ./DevUtils.py -t. This is not normative for python (related to #150). A python programmer would expect to find the instructions, e.g., pytest . or python -m unittest ... to invoke the unit test suite. The documentation also relies on the devutils files having executable permissions for the user, and /usr/bin/python3 being the interpreter that PyPO is installed to. Please change this to at a minimum python DevUtils.py -t, so that the instructions are compatible with the full range of possible setups that pip install . can produce.

I ran the tests on my M2 macbook air for more than ten minutes, and ctrl+C'd them after waiting that long. It looks like TestBessel has a dead spin or something in it,

# duration: 13m 13.44s
python DevUtils.py -c
Warning: you need to install "coverage_plugin" extra requirements to use this plugin. e.g. `pip install nose2[coverage_plugin]`
test_addLoggingLevel (tests.test_CustomLogger.TestCustomLogger) ... ok
test_CustomFormatter (tests.test_CustomLogger.TestCustomLogger) ... ok
test_CustomGUIFormatter (tests.test_CustomLogger.TestCustomLogger) ... ok
test_CustomGUILogger (tests.test_CustomLogger.TestCustomLogger) ... ok
test_CustomLogger (tests.test_CustomLogger.TestCustomLogger) ... ok
test_GUILogger (tests.test_CustomLogger.TestCustomLogger) ... ok
test_findConnectedSubsets (tests.test_MatUtils.Test_MatUtils) ... ok
test_plotBeam2D (tests.test_Plotting.Test_SystemOps) ... ok
test_plotBeam3D (tests.test_Plotting.Test_SystemOps) ... ok
test_plotBeamCut (tests.test_Plotting.Test_SystemOps) ... ok
test_plotGroup (tests.test_Plotting.Test_SystemOps) ... ok
test_plotRTframe (tests.test_Plotting.Test_SystemOps) ... ok
test_plotSystem (tests.test_Plotting.Test_SystemOps) ... ok
test_findRotation (tests.test_RayTraceUtils.Test_RayTraceUtils) ... ok
test_FocusFind_ellipse_x (tests.test_RayTraceUtils.Test_RayTraceUtils) ... ok
test_FocusFind_ellipse_z (tests.test_RayTraceUtils.Test_RayTraceUtils) ... ok
test_FocusFind_parabola (tests.test_RayTraceUtils.Test_RayTraceUtils) ... ok
test_getAnglesFromMatrix (tests.test_RayTraceUtils.Test_RayTraceUtils) ... ok
test_setCustomBeamPath (tests.test_SystemPaths.Test_SystemPaths) ... ok
test_setSavePath (tests.test_SystemPaths.Test_SystemPaths) ... ok
test_generateGridsEllipsoid (tests.test_SystemGrids.Test_SystemGrids) ... ok
test_generateGridsHyperboloid (tests.test_SystemGrids.Test_SystemGrids) ... ok
test_generateGridsParaboloid (tests.test_SystemGrids.Test_SystemGrids) ... ok
test_generateGridsPlane (tests.test_SystemGrids.Test_SystemGrids) ... ok
test_loadRefllib (tests.test_SystemGrids.Test_SystemGrids) ... ok
test_addGroup (tests.test_SystemDictsAndAddElements.Test_SystemDictsAndAddElement) ... ok
test_addPOFields (tests.test_SystemDictsAndAddElements.Test_SystemDictsAndAddElement) ... ok
test_addPOScalarFields (tests.test_SystemDictsAndAddElements.Test_SystemDictsAndAddElement) ... ok
test_addReflector (tests.test_SystemDictsAndAddElements.Test_SystemDictsAndAddElement) ... ok
test_addRTFrame (tests.test_SystemDictsAndAddElements.Test_SystemDictsAndAddElement) ... ok
test_dictsExist (tests.test_SystemDictsAndAddElements.Test_SystemDictsAndAddElement) ... ok
test_mergeBeams_uv (tests.test_MergeBeams.Test_MergeBeams) ... ok
test_mergeBeams_xy (tests.test_MergeBeams.Test_MergeBeams) ... ok
test_fitGauss (tests.test_FitGauss.Test_FitGauss) ... ok
test_autoConverge (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_invalidRunHybridDict (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_invalidRunPODict (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_invalidRunRTDict (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_loadCPUlib (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_loadGPUlib (tests.test_SystemPO_RT.Test_SystemPO_RT) ... No GPU libraries found... Not testing GPU functionalities.
ok
test_runPO_EH (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_runPO_EHP_Hybrid (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_runPO_FF (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_runPO_JM (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_runPO_JMEH (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_runPO_Scalar (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_runRT (tests.test_SystemPO_RT.Test_SystemPO_RT) ... ok
test_rotations_ellipse (tests.test_MatTransform.Test_MatTransform) ... ok
test_rotations_hyperbola (tests.test_MatTransform.Test_MatTransform) ... ok
test_rotations_parabola (tests.test_MatTransform.Test_MatTransform) ... ok
test_rotations_plane (tests.test_MatTransform.Test_MatTransform) ... ok
test_translations_ellipse (tests.test_MatTransform.Test_MatTransform) ... ok
test_translations_hyperbola (tests.test_MatTransform.Test_MatTransform) ... ok
test_translations_parabola (tests.test_MatTransform.Test_MatTransform) ... ok
test_translations_plane (tests.test_MatTransform.Test_MatTransform) ... ok
test_Bessel (tests.test_Bessel.Test_Bessel) ... ^C
----------------------------------------------------------------------
Ran 56 tests in 787.694s

OK

Traceback (most recent call last):
  File "/Users/bdd/miniconda3/envs/tmp/bin/nose2", line 8, in <module>
    sys.exit(discover())
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/site-packages/nose2/main.py", line 339, in discover
    return main(*args, **kwargs)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/site-packages/nose2/main.py", line 87, in __init__
    super().__init__(**kw)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/main.py", line 101, in __init__
    self.runTests()
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/site-packages/nose2/main.py", line 302, in runTests
    self.result = runner.run(self.test)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/site-packages/nose2/runner.py", line 55, in run
    executor(test, result)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/site-packages/nose2/runner.py", line 43, in executor
    return suite(result)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/suite.py", line 84, in __call__
    return self.run(*args, **kwds)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/suite.py", line 122, in run
    test(result)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/suite.py", line 84, in __call__
    return self.run(*args, **kwds)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/suite.py", line 122, in run
    test(result)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/suite.py", line 84, in __call__
    return self.run(*args, **kwds)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/suite.py", line 122, in run
    test(result)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/case.py", line 650, in __call__
    return self.run(*args, **kwds)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/case.py", line 591, in run
    self._callTestMethod(testMethod)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/unittest/case.py", line 549, in _callTestMethod
    method()
  File "/Users/bdd/src/PyPO/tests/test_Bessel.py", line 24, in test_Bessel
    e, h = self._calc_ff(lam)
  File "/Users/bdd/src/PyPO/tests/test_Bessel.py", line 83, in _calc_ff
    s.runPO(source_to_ff)
  File "/Users/bdd/src/PyPO/src/PyPO/System.py", line 1174, in runPO
    out = PyPO_CPUd(source, target, _runPODict)
  File "/Users/bdd/src/PyPO/src/PyPO/BindCPU.py", line 210, in PyPO_CPUd
    mgr.new_sthread(target=lib.propagateToFarField, args=args)
  File "/Users/bdd/src/PyPO/src/PyPO/Threadmgr.py", line 20, in new_sthread
    t.join(.1)
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/threading.py", line 1100, in join
    self._wait_for_tstate_lock(timeout=max(timeout, 0))
  File "/Users/bdd/miniconda3/envs/tmp/lib/python3.10/threading.py", line 1116, in _wait_for_tstate_lock
    if lock.acquire(block, timeout):
KeyboardInterrupt

The other tests took just a few moments to run up to then. Could you check if the bessel unit tests are just some arm64 / big endian bug or something?

[arendMoerman]

Thank you for the response. We have updated the way tests are run. We have actually overhauled and streamlined the tests quite a bit. They now use the parametrisation capabilities of nose2 more fully and therefore the total amount of test code has gone down. This dependency on the arametrisation of nose2 does mean that they must be run like:

nose2

Also, the tests are not run anymore from the DevUtils.py script. We have updated the instructions in the docs accordingly.
Now, only generating documentation is done by calling a script. Running the tests is now done directly from the command line.

On a different note, the test (Bessel) that took so long was actually taking long. I had put the gridsizes of one optical element quite high, in order to test some scaling stuff with my GPU and forgot to put it back I'm afraid. It did not need to be that high.
This got us thinking and we decided that this particular test, and a few others, were really not unit tests after all but more like validations of the program. We removed these tests and replaced them with more appropriate, actual, unit tests for these functionalities.
We have put these removed tests into jupyter notebooks and are now called demonstrations. They can be found in the docs under the "Demonstrations" section. These are currently being added to and updated quite regularly, as I had a large backlog of validation tests on my computer, which I am now slowly adding into the demonstrations section.

In this way, we still have these demonstrations, which are more like (physical) validations, but have them separated from the unit tests.

Could you please check the unit tests again and confirm if they work?

[brandondube]

thanks! They all pass with a nose2 command now