Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import problem for the subtract_median testing #1002

Open
xandrd opened this issue Aug 30, 2024 · 1 comment
Open

Import problem for the subtract_median testing #1002

xandrd opened this issue Aug 30, 2024 · 1 comment
Assignees
Labels
python Issues involving Python and Python-related tools outside of pyspedas pytplot Issues involving the pytplot package QA/Testing

Comments

@xandrd
Copy link
Collaborator

xandrd commented Aug 30, 2024

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:

  • Using patch('pytplot.tplot_math.subtract_average.subtract_average') fails because the first subtract_average is already identified as a function due to the __init__.py structure.
  • Using patch('pytplot.tplot_math.subtract_average') does not work either because within subtract_median, the function is actually referenced as pytplot.tplot_math.subtract_average.subtract_average.

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
Copy link
Contributor

jameswilburlewis commented Aug 31, 2024

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.

@jameswilburlewis jameswilburlewis added QA/Testing pytplot Issues involving the pytplot package python Issues involving Python and Python-related tools outside of pyspedas labels Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Issues involving Python and Python-related tools outside of pyspedas pytplot Issues involving the pytplot package QA/Testing
Projects
None yet
Development

No branches or pull requests

2 participants