Treat correctly relative imports in 'import_module_from_file' #305
Conversation
* Use the absolute path of the filename if the normalized path is not a directory.
|
Maybe this needs a little bit of redesign. Should we allow loading of modules that don't exist? |
| if os.path.isdir(filename): | ||
| filename = os.path.join(filename, '__init__.py') | ||
| else: | ||
| filename = os.path.abspath(filename) |
There was a problem hiding this comment.
I don't think this is correct, because you always trigger the _do_import_module_from_file. This is not exactly the same as using import_module since it does not treat correctly namespace packages.
I see that we already normalise the filename argument, this meaning that it won't contain any .. components inside it, except possibly at the beginning. So, we just need to make sure that we call correctly the importlib.import_module in order to do a relative import.
unittests/test_utility.py
Outdated
| self.assertIs(module, sys.modules.get('reframe')) | ||
|
|
||
| def test_load_file_starts_with_ddot(self): | ||
| parent_dir = os.path.basename(os.path.abspath(os.pardir)) |
There was a problem hiding this comment.
Better use .. instead of os.pardir to be clearer and consistent with other usages.
There was a problem hiding this comment.
Also, I wouldn't write the test this way. I'd instead change temporarily to a directory and do a relative import from there. This is the faulty scenario in any case. You can use the utility.os_ext.change_dir context manager for doing the directory change safely.
vkarak
changed the title
| filename = os.path.join(filename, '__init__.py') | ||
|
|
||
| if filename.startswith('..'): | ||
| filename = os.path.abspath(filename) |
There was a problem hiding this comment.
I think that this is the only solution. It seems we cannot use the importlib.import_module(), because we don't which package to attach to. Am I right?
There was a problem hiding this comment.
yes, that is correct.
There was a problem hiding this comment.
I tested and it now works both with settings and checks defined with relative paths.
unittests/test_utility.py
Outdated
|
|
||
| def test_load_dir_starts_with_ddot(self): | ||
| with os_ext.change_dir('reframe'): | ||
| filename = '..%s/reframe' % os.getcwd() |
There was a problem hiding this comment.
I don't think that this does the intended thing, because os.getcwd() returns an absolute path. I don't think it's needed. In any case the change_dir() assumes that we are running the unit tests from top-level directory.
path is not a directory.
Fixes #302