Rewrite IPython extension tests

[antonymilne]

Note. Somewhat related and probably best done in parallel or by the same person (@noklam? 😀): #1840.

test_ipython.py is a bit of a mess. There's way too much mocking going on and it causes quite a few problems. I strongly suspect that it's not testing what we think it tests either, e.g. in test_load_kedro_objects if you alter the paths from kedro_path / ... to something else then the test will still pass.

I think we should rewrite the whole file. This isn't necessarily a huge task in terms of writing code, because I suspect that some of the tests there are basically pointless even if they worked. But it will take some careful thought.

First stage of this is to ask what do we actually want to test here?

• If it's the behaviour of underlying functions (like reload_kedro function) then which parts of that are actually valuable to test? Are those best tested through unit tests or e2e behave test instead?
• If it's ipython-specific behaviour (like line magics) then there should be ipython-specific ways to test that without mocking so much. e.g. https://medium.com/@davide.sarra/how-to-test-magics-in-ipython-extensions-86d99e5d6802 and Google how to test ipython line magics, extensions, etc. @noklam recently introduced IPython.testing.globalipapp.get_ipython to the tests, which feels like a much better solution than all the mocking

Second stage is to write the new tests! Don't be afraid to delete old tests if they are no longer valuable. A good example of tests that will no longer be valuable is those that test behaviour of default_project_path. The flow for this should be way simpler and easier to test (maybe not even need tests at all depending on how it works) after #1840.

Warning. Much of this file is currently marked as pragma: no cover so that test coverage doesn't break CI due to the skipped tests. Fixing tests should increase coverage and then these no covers should be removed.

[antonymilne]

In #1837 we have opted to skip some IPython tests. They pass individually but not as part of the whole test suite due to some interference with other tests.

@noklam and I suspect this is related to #1787. I saw the same kind of non-deterministic test behaviour working on something else before which led to me adding the bit of code given there in kedro/tests/framework/cli/conftest.py.

Depending on how the rewrite of this file works, #1787 may or may not be required to complete this.

Also note that the IPython extension contains a function _remove_cached_modules that does some similar del sys.modules stuff. Possibly that's interfering with things?

[noklam]

Brain dumps here:
something funky is interfering in kedro/project/__init__.py, the error is caused by this function as it tries to load a module that doesn't exist anymore (deleted by del sys.modules)

    def _get_pipelines_registry_callable(pipelines_module: str):
        module_obj = importlib.import_module(pipelines_module)

Note that I try to run in debug mode in both situations

1. tests/ipython/test_ipython.py tests/framework/project/test_logging.py --no-cov - Pass
2. tests/framework/project/test_logging.py tests/ipython/test_ipython.py --no-cov - Fail
   At the time reload_kedro get executed, the sys.modules are identical, so it's cached even earlier than this, maybe it's some closure that captured it? Not sure. The Mock actually cached something about sys.modules['fake_package_name'] which was deleted later.

Some thoughts from Antony:

[noklam]

I am now more certain this is related to import. If I do mock_ipython().push.call_args_list I will get the same error, because one of the call args is calling the fake_package_name, removing the pipeline as below and updating the tests will bypass the error.

    get_ipython().push(
        variables={
            "context": context,
            "catalog": catalog,
            "session": session,
            # "pipelines": pipelines,
        }
    )