spedas / pyspedas

Python-based Space Physics Environment Data Analysis Software
https://pyspedas.readthedocs.io/
MIT License
156 stars 60 forks source link

Import problem for the subtract_median testing #1002

Open xandrd opened 2 months ago

xandrd commented 2 months ago

I'm extending the test coverage for the subtract_average and subtract_median functions. Since subtract_median essentially wraps around subtract_average, I aim to create a test to verify if subtract_median correctly calls subtract_average with the appropriate parameters.

Here’s the intended test structure:

def test_subtract_median_parameter_passing(self):
    """Test that parameters are correctly passed to subtract_average via subtract_median."""

    with patch('pyspedas.subtract_average') as mock_subtract_average:
        pyspedas.subtract_median('test', newname='new_test', suffix='-sfx', overwrite=True)

        # Verify that subtract_average was called with the correct parameters, including median=1
        mock_subtract_average.assert_called_once_with(
            'test', newname='new_test', suffix='-sfx', overwrite=True, median=1
        )

However, this test fails due to how subtract_median imports subtract_average:

from .subtract_average import subtract_average

Here’s the problem: subtract_average.py is in the tplot_math package, which itself is part of pytplot. The tplot_math package's __init__.py file imports both functions as follows:

from .subtract_median import subtract_median
from .subtract_average import subtract_average

This enables users to import the functions like:

import pytplot.tplot_math.subtract_average

instead of:

import pytplot.tplot_math.subtract_average.subtract_average

However, when subtract_median internally calls subtract_average, it uses the full path: pytplot.tplot_math.subtract_average.subtract_average, not the simplified path pytplot.tplot_math.subtract_average.

Due to this, the test fails:

This indicates either a misunderstanding of the Python import system on my part or a potential issue with how the pytplot import structure is currently organized.

[!IMPORTANT] With the reorganization of pyspedas into separate projects, this import behavior may lead to conflicts similar to the one described above.

jameswilburlewis commented 2 months ago

Wow, this one had both me and ChatGPT stumped for a while! But I figured it out:

What you need to do here is mock subtract_average where it's used (in subtract_median) not where it's defined (in subtract_average). So your patch call would have to be:

with patch('pytplot.tplot_math.subtract_median.subtract_average') as mock_subtract_average:

But if you tried this, you would have noticed that it throws an AttributeError:

AttributeError: <function subtract_median at 0x177639e50> does not have the attribute 'subtract_average'

Why? Because when the subtract_median() function is imported, it shadows the module name subtract_median! So it's unable to look up subtract_average in the subtract_median module.

To verify this, I copied subtract_average.py and subtract_median.py to sa.py and sm.py, and changed the function names in those files to subtract_averagex and subtract_medianx. I updated the import statement in sm.py, and also added the new imports to tplot_math/__init__.py. Now the function name subtract_medianx no longer shadows the module name that contains it.

With these changes in place, this test:

    def test_subtract_median_parameter_passing(self):
        from unittest.mock import patch
        """Test that parameters are correctly passed to subtract_average via subtract_median."""
        pyspedas.themis.fgm(probe='c')
        with patch('pytplot.tplot_math.sm.subtract_averagex') as mock_subtract_average:
            subtract_medianx('thc_fgs_dsl', newname='new_test', suffix='-sfx', overwrite=True)

            # Verify that subtract_average was called with the correct parameters, including median=1
            mock_subtract_average.assert_called_once_with(
                'thc_fgs_dsl', newname='new_test', suffix='-sfx', overwrite=True, median=1
            )

works perfectly: the mock function is called as expected.

Basically, the extremely common pattern of defining 'func' within 'func.py' seems to render it difficult to successfully mock anything that func() is calling internally.

I think most users of unittest.mock are using it to mock classes or class methods, rather than free functions. With classes, the usual naming convention of defining MyClass in myclass.py avoids the shadowing problem.

There was one other thing I tried that worked: in subtract_median, I think I imported pytplot instead of subtract average, then called subtract_average with the fully qualified name pytplot.subtract_average(). (Perhaps I had tplot_math in there too?). Then I was able to successfully mock it with patch('pytplot.subtract_average') as mock_subtract_average

I don't think we need to go on a mass renaming spree in order to support mocking every possible function. But there are a few cases, with the more complicated modules like mms, themis, and erg, where module vs. function name collisions have caused issues, and I think I will do a little more refactoring to avoid those cases.

For this specific task: I don't think it's necessary to go to the lengths of trying to mock subtract_average to verify that it's being passed the correct arguments. Simply setting a breakpoint would let you check that.