launch_testing test file import can cause confusing module import issues

[pbaughman]

Bug report

Required Info:

• Operating System:
  • Ubuntu 18.04
• Installation type:
  • source
• Version or commit hash:
  • Dashing
• DDS implementation:
  • Doesn't matter, this is a pure-python thing
• Client library (if applicable):
  • N/A

Steps to reproduce issue

We have a package called lidar_integration. This package installs a python module also called lidar_integration and it has some tests that run with ros2 test. The test file is called foo_test.py

When we run the test with colcon test or with ros2 test foo_test.py --package_name lidar_integration we get a bizarre import error that was difficult to debug, but easy to understand once we figured out what's going on. The test tries to do

from lidar_integration import useful_tool

But then fails with someting like (sorry, error message is from memory)

useful_tool not a member of lidar_integration

Because of the way the test file was imported by launch_testing here, when you import lidar_integration from the test, instead of getting the lidar_integration module which contains useful_tool, you get test file which was named 'lidar_integration' when it was imported because lidar_integration is the package name and that was used to name the module when the SourceFileLoader loaded it

Expected behavior

I'm not sure, exactly. I wanted to get @hidmic 's opinion on this. If you run the test with ros2 test foo_test.py, there's no problem becaues when we import the test file we name it foo_test. foo_test doesn't collide with any modules names so there's no problem.

I suspect we use the package name as the module name because this information makes it into the junit XML generated by launch_testing (I'm speculating based on the git blame history of this line). I have not yet investigated what happens to the junit XML if I change the name of the imported test file.

Additional information

I want your take on how to solve this problem. My ideas thus far are

• Always use the launch_test_file_basename when calling _load_python_file_as_module
  • Pros: Solves this case
  • Cons: Could still collide in other cases (like if the test was named lidar_integration.py). Maybe messes up the junit XML results (I haven't checked yet)
• Move this test into another package called test_lidar_integration
  • Pros: Solves the problem. Not totally inconsistent with packages like test_launch_testing
  • Cons: This solves our problem, but it doesn't help anybody else whose confused by the same issue
• Maybe when we're importing the module, try to import that name and issue a warning if the import succeeds
  • Pros: Would make it easier to understand what's going on
  • Cons: Does not actually solve the problem. You'd still need to do one of the other things above

[hidmic]

I can't recall why didn't we simply do args.package_name + '.' + launch_test_file_basename. We should check aggregated results on Jenkins.

[hidmic]

@pbaughman are you planning to send a patch to address this issue? It seems straightforward.

[pbaughman]

@hidmic Yeah, let me see if I can find a moment to do it. I need to be a little careful to do this in a way that doesn't change the resulting XML files because we have a whole test-history infrastructure built up where the test name is the unique key.

People are going to be very upset at me if a quarter of our tests suddenly have different names and now it's hard to see the history

[pbaughman]

@hidmic I need to think about this some more, or I'm going to cause all our tests to get renamed.

The module name we choose here gets included as part of the classname in one of the junit XML elements here

If I change it (for example, to foo), then a test with a name like

demo_nodes_cpp.test_tutorial_talker_listener__rmw_fastrtps.demo_nodes_cpp.TestExecutablesTutorial.test_processes_output

becomes

demo_nodes_cpp.test_tutorial_talker_listener__rmw_fastrtps.foo.TestExecutablesTutorial.test_processes_output

[pbaughman]

I wonder if there's a way to do _load_python_file_as_module but then not have that module cached when you do a later import. . .

[pbaughman]

@hidmic I can preserve the existing test names and fix the issue by doing:

def _load_python_file_as_module(test_module_name, python_file_path):     
    """Load a given Python launch file (by path) as a Python module."""  
    # Taken from launch_testing to not introduce a weird dependency thing
    try:                                                                 
        # If there happens to be a module with the same name as test_module_name, import it
        # here before we load the test as a module.  That way, any subsequent 'import' statements
        # in the test that try to import a module with this name get the 'real' one instead of the test
        # file because the result of import_module will be cached in sys.modules
        importlib.import_module(test_module_name)                        
    except:                                                              
        pass                                                             
    loader = SourceFileLoader(test_module_name, python_file_path)        
    return loader.load_module()                                          

This tries to import any 'real' modules that have the same name as test_module_name, so any subsequent 'import' statements get the real one instead of the test-file-as-a-module one.

On a scale of good to awful, where do you think this lands? I wonder if we should worry about side-effects from importing

[hidmic]

On a scale of good to awful, where do you think this lands?

It's not pretty, but I don't see a lot of other options. You could tinker with sys.modules, but that's not pretty either.

I wonder if we should worry about side-effects from importing

Yeah, there's that. Ideally, loading a test module shouldn't have any side effects.

[pbaughman]

@hidmic I tried to mess about with sys.modules, but there was no good place to do it. When this errors out, loader.load_module() from launch_test.py is in the stack. We'd have to do major surgery on the SourceFileLoader to fix it that way, I think. . .

Anyway, I'll open a PR with the 'load the real one early' fix