Wildcard import in PTL tests

Developers
1

I noticed that we have a lot of wildcard imports in PTL tests.

from test.function import *

According to PEP8:

Wildcard imports (from import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools

This is also found in the PTL quick start guide.

Is there a reason we do wildcard imports at the beginning of a test file? If not I suggest we make the the imports explicit. If we need everything from a module we can import the module using

import module

and access its members using module.member .

Or do

import module as md

and access members using md.member to save some typing.

2

I’m not sure if there’s a reason why we do wildcard imports other than the fact that it’s more convenient for test writer to write “DshUtils()” than “pbs_dshutils.DshUtils()”, and to import * than to explicitly specify the objects that they are importing from a module.

About importing modules as aliases, it’d be great if we could come up with short, intuitive aliases for modules to import, but I have a feeling that it’s easier said than done.

So, is there enough value in giving up the convenience that comes with wildcard imports? Isn’t it the same as including a header file in C? Is it really that bad?

3

I agree that using wildcard imports is convenient. It introduces problems that, in my opinion, should be avoided at the cost of that convenience.

Opportunity for name collision

For example, if I imported some standard library modules and everything in my module using wildcard imports:

import os
import foo
import bar
from my_module import *

If my_module contains foo or bar then they will be used instead of those in the standard library.

Performance impact

Wildcard import is inefficient. The speed of loading and running a python script will be slow if it imports modules with a large number of members.

Reader confusion

from my_module import foo, bar
from my_other_module import my_function

clearly shows what we import and use from each module. With wildcard imports, it is difficult to find out where foo comes from and where my_function comes from without going through the source files of all modules we imported.

Static analysis tools confusion

Static analysis tools like pylint and flake8 have trouble doing certain types of checking (one example is checking for variables and functions that are not used) if wildcard imports exist.

Removing wildcard imports and rewriting them explicitly can help us avoid these problems. I think it is a good investment for the cost of some convenience.

Reference

  1. https://stackoverflow.com/questions/2360724/what-exactly-does-import-import
  2. https://stackoverflow.com/questions/2386714/why-is-import-bad
4

Thank a lot for elaborating further Minghui.

I had not considered performance impact, that alone is enough of a reason for me to not do wildcard imports. Also, we recently integrated Codacy which uses Pylint internally for static analysis, so that’s a pretty good reason to not use them as well.

@hirenvadalia and @kjakkali what do you guys think?

5

@minghui and @agrawalravi90 Here we are talking about PTL which is test framework means users will use that to write test cases and run them… So in that context personally I will chose convenience over performance…

And regarding “Opportunity for name collision” it is up to test writer and fw developer, how they write code… and till now we have not hit any such problem in PTL. So I am not much worried about this…

Regarding “Reader confusion” in PTL world what I have seen is user don’t care much about what it importing when you have wildcard, they care only about how easily I can write test.

But Yes, as per standards of Python it is good practice to use explicit import where performance matter, but here I think we don’t care much about performance because we are want to give convenience to test writer.

6

Test writers still need to know what members are there in a module even if they import everything in it, so I do not see how much more convenient wildcard import is than a import as statement with a short alias.

BTW can someone please explain to me why we import every other test in the same test category?
For example:

from tests.functional import *

is in every functional test. Why does a functional test file need to import all other test files?

Thanks.

7

Thanks for your reply Hiren.

I agree with you that convenience is important for test writers, but is it really that inconvenient to type a few extra characters, a handful of times in a test? or to import the specific objects from a module at the time of importing instead of the entire module? Also, as Minghui mentioned already, it might actually help test writers as they’ll not have to search through the entire repo to find which module a particular object belongs to.

About performance, if we improve performance, we will be able to add more tests to our SmokeTest suite to enable better test coverage while running Travis, or run the existing ones faster so that one gets faster turnarounds on PRs, so I disagree with you that performance is not important. Now, how much performance improvement can we actually get by not importing the entire module remains to be tested, but I do think that if the improvement is significant then that alone can justify importing each object from a module explicitly.

So, I think it might be worth investigating this more to see if the advantages trump the slight inconvenience that this might cause.

8

This is all the classes that get imported when I do “from tests.functional import *”:
[(‘BatchUtils’, <class ‘ptl.lib.pbs_testlib.BatchUtils’>), (‘CliUtils’, <class ‘ptl.utils.pbs_cliutils.CliUtils’>), (‘Comm’, <class ‘ptl.lib.pbs_testlib.Comm’>), (‘DshUtils’, <class ‘ptl.utils.pbs_dshutils.DshUtils’>), (‘Entity’, <class ‘ptl.lib.pbs_testlib.Entity’>), (‘EquivClass’, <class ‘ptl.lib.pbs_testlib.EquivClass’>), (‘ExpectAction’, <class ‘ptl.lib.pbs_testlib.ExpectAction’>), (‘ExpectActions’, <class ‘ptl.lib.pbs_testlib.ExpectActions’>), (‘FairshareNode’, <class ‘ptl.lib.pbs_testlib.FairshareNode’>), (‘FairshareTree’, <class ‘ptl.lib.pbs_testlib.FairshareTree’>), (‘FileUtils’, <class ptl.utils.pbs_fileutils.FileUtils at 0x7f9ffcb92f58>), (‘Holidays’, <class ptl.lib.pbs_testlib.Holidays at 0x7f9ffc921328>), (‘Hook’, <class ‘ptl.lib.pbs_testlib.Hook’>), (‘InteractiveJob’, <class ‘ptl.lib.pbs_testlib.InteractiveJob’>), (‘Job’, <class ‘ptl.lib.pbs_testlib.Job’>), (‘Limit’, <class ‘ptl.lib.pbs_testlib.Limit’>), (‘LooseVersion’, <class distutils.version.LooseVersion at 0x7f9ffcb92bb0>), (‘MoM’, <class ‘ptl.lib.pbs_testlib.MoM’>), (‘OrderedDict’, <class ‘collections.OrderedDict’>), (‘PBSInitServices’, <class ‘ptl.lib.pbs_testlib.PBSInitServices’>), (‘PBSLogAnalyzer’, <class ‘ptl.utils.pbs_logutils.PBSLogAnalyzer’>), (‘PBSObject’, <class ‘ptl.lib.pbs_testlib.PBSObject’>), (‘PBSService’, <class ‘ptl.lib.pbs_testlib.PBSService’>), (‘PBSServiceInstanceWrapper’, <class ‘ptl.utils.pbs_testsuite.PBSServiceInstanceWrapper’>), (‘PBSTestSuite’, <class ‘ptl.utils.pbs_testsuite.PBSTestSuite’>), (‘PbsAlterError’, <class ‘ptl.lib.pbs_testlib.PbsAlterError’>), (‘PbsAttribute’, <class ‘ptl.lib.pbs_testlib.PbsAttribute’>), (‘PbsBatchObject’, <class ‘ptl.lib.pbs_testlib.PbsBatchObject’>), (‘PbsBatchStatus’, <class ‘ptl.lib.pbs_testlib.PbsBatchStatus’>), (‘PbsConnectError’, <class ‘ptl.lib.pbs_testlib.PbsConnectError’>), (‘PbsDeleteError’, <class ‘ptl.lib.pbs_testlib.PbsDeleteError’>), (‘PbsDeljobError’, <class ‘ptl.lib.pbs_testlib.PbsDeljobError’>), (‘PbsDelresvError’, <class ‘ptl.lib.pbs_testlib.PbsDelresvError’>), (‘PbsFairshareError’, <class ‘ptl.lib.pbs_testlib.PbsFairshareError’>), (‘PbsGroup’, <class ‘ptl.lib.pbs_testlib.PbsGroup’>), (‘PbsHoldError’, <class ‘ptl.lib.pbs_testlib.PbsHoldError’>), (‘PbsInitServicesError’, <class ‘ptl.lib.pbs_testlib.PbsInitServicesError’>), (‘PbsManagerError’, <class ‘ptl.lib.pbs_testlib.PbsManagerError’>), (‘PbsMessageError’, <class ‘ptl.lib.pbs_testlib.PbsMessageError’>), (‘PbsMomConfigError’, <class ‘ptl.lib.pbs_testlib.PbsMomConfigError’>), (‘PbsMoveError’, <class ‘ptl.lib.pbs_testlib.PbsMoveError’>), (‘PbsOrderError’, <class ‘ptl.lib.pbs_testlib.PbsOrderError’>), (‘PbsQdisableError’, <class ‘ptl.lib.pbs_testlib.PbsQdisableError’>), (‘PbsQenableError’, <class ‘ptl.lib.pbs_testlib.PbsQenableError’>), (‘PbsQstartError’, <class ‘ptl.lib.pbs_testlib.PbsQstartError’>), (‘PbsQstopError’, <class ‘ptl.lib.pbs_testlib.PbsQstopError’>), (‘PbsQtermError’, <class ‘ptl.lib.pbs_testlib.PbsQtermError’>), (‘PbsReleaseError’, <class ‘ptl.lib.pbs_testlib.PbsReleaseError’>), (‘PbsRerunError’, <class ‘ptl.lib.pbs_testlib.PbsRerunError’>), (‘PbsResourceError’, <class ‘ptl.lib.pbs_testlib.PbsResourceError’>), (‘PbsRunError’, <class ‘ptl.lib.pbs_testlib.PbsRunError’>), (‘PbsSchedConfigError’, <class ‘ptl.lib.pbs_testlib.PbsSchedConfigError’>), (‘PbsSelectError’, <class ‘ptl.lib.pbs_testlib.PbsSelectError’>), (‘PbsServiceError’, <class ‘ptl.lib.pbs_testlib.PbsServiceError’>), (‘PbsSignalError’, <class ‘ptl.lib.pbs_testlib.PbsSignalError’>), (‘PbsStatusError’, <class ‘ptl.lib.pbs_testlib.PbsStatusError’>), (‘PbsSubmitError’, <class ‘ptl.lib.pbs_testlib.PbsSubmitError’>), (‘PbsTypeArray’, <class ‘ptl.lib.pbs_testlib.PbsTypeArray’>), (‘PbsTypeAttribute’, <class ‘ptl.lib.pbs_testlib.PbsTypeAttribute’>), (‘PbsTypeChunk’, <class ‘ptl.lib.pbs_testlib.PbsTypeChunk’>), (‘PbsTypeDuration’, <class ‘ptl.lib.pbs_testlib.PbsTypeDuration’>), (‘PbsTypeExecHost’, <class ‘ptl.lib.pbs_testlib.PbsTypeExecHost’>), (‘PbsTypeExecVnode’, <class ‘ptl.lib.pbs_testlib.PbsTypeExecVnode’>), (‘PbsTypeFGCLimit’, <class ‘ptl.lib.pbs_testlib.PbsTypeFGCLimit’>), (‘PbsTypeJobId’, <class ‘ptl.lib.pbs_testlib.PbsTypeJobId’>), (‘PbsTypeLicenseCount’, <class ‘ptl.lib.pbs_testlib.PbsTypeLicenseCount’>), (‘PbsTypeList’, <class ‘ptl.lib.pbs_testlib.PbsTypeList’>), (‘PbsTypeSelect’, <class ‘ptl.lib.pbs_testlib.PbsTypeSelect’>), (‘PbsTypeSize’, <class ‘ptl.lib.pbs_testlib.PbsTypeSize’>), (‘PbsTypeVariableList’, <class ‘ptl.lib.pbs_testlib.PbsTypeVariableList’>), (‘PbsUser’, <class ‘ptl.lib.pbs_testlib.PbsUser’>), (‘Policy’, <class ‘ptl.lib.pbs_testlib.Policy’>), (‘ProcMonitor’, <class ‘ptl.utils.pbs_procutils.ProcMonitor’>), (‘ProcUtils’, <class ‘ptl.utils.pbs_procutils.ProcUtils’>), (‘PtlConfig’, <class ‘ptl.lib.pbs_testlib.PtlConfig’>), (‘PtlException’, <class ‘ptl.lib.pbs_testlib.PtlException’>), (‘PtlExpectError’, <class ‘ptl.lib.pbs_testlib.PtlExpectError’>), (‘PtlFailureException’, <class ‘ptl.lib.pbs_testlib.PtlFailureException’>), (‘PtlLogMatchError’, <class ‘ptl.lib.pbs_testlib.PtlLogMatchError’>), (‘Queue’, <class ‘ptl.lib.pbs_testlib.Queue’>), (‘Reservation’, <class ‘ptl.lib.pbs_testlib.Reservation’>), (‘Resource’, <class ‘ptl.lib.pbs_testlib.Resource’>), (‘ResourceResv’, <class ‘ptl.lib.pbs_testlib.ResourceResv’>), (‘Scheduler’, <class ‘ptl.lib.pbs_testlib.Scheduler’>), (‘Server’, <class ‘ptl.lib.pbs_testlib.Server’>), (‘SkipTest’, <class ‘unittest.case.SkipTest’>), (‘TestFunctional’, <class ‘tests.functional.TestFunctional’>), (‘attrl’, <class ptl.lib.pbs_ifl_mock.attrl at 0x7f9ffcb92c18>), (‘attropl’, <class ptl.lib.pbs_ifl_mock.attropl at 0x7f9ffcb927a0>), (‘batch_status’, <class ptl.lib.pbs_ifl_mock.batch_status at 0x7f9ffcb92c80>), (‘ecl_attrerr’, <class ptl.lib.pbs_ifl_mock.ecl_attrerr at 0x7f9ffcb92ce8>), (‘ecl_attribute_errors’, <class ptl.lib.pbs_ifl_mock.ecl_attribute_errors at 0x7f9ffcb92d50>), (‘itemgetter’, <type ‘operator.itemgetter’>), (‘setUpClassError’, <class ‘ptl.utils.pbs_testsuite.setUpClassError’>), (‘tearDownClassError’, <class ‘ptl.utils.pbs_testsuite.tearDownClassError’>)]

And this is all the modules that get imported:
[(‘cPickle’, <module ‘cPickle’ from ‘/usr/lib64/python2.7/lib-dynload/cPickle.so’>), (‘calendar’, <module ‘calendar’ from ‘/usr/lib64/python2.7/calendar.pyc’>), (‘copy’, <module ‘copy’ from ‘/usr/lib64/python2.7/copy.pyc’>), (‘datetime’, <module ‘datetime’ from ‘/usr/lib64/python2.7/lib-dynload/datetime.so’>), (‘grp’, <module ‘grp’ from ‘/usr/lib64/python2.7/lib-dynload/grpmodule.so’>), (‘logging’, <module ‘logging’ from ‘/usr/lib64/python2.7/logging/init.pyc’>), (‘os’, <module ‘os’ from ‘/usr/lib64/python2.7/os.pyc’>), (‘pbs_qstat’, <module ‘tests.functional.pbs_qstat’ from ‘tests/functional/pbs_qstat.py’>), (‘platform’, <module ‘platform’ from ‘/usr/lib64/python2.7/platform.pyc’>), (‘ptl’, <module ‘ptl’ from ‘/usr/lib/python2.7/site-packages/ptl/init.pyc’>), (‘pwd’, <module ‘pwd’ (built-in)>), (‘random’, <module ‘random’ from ‘/usr/lib64/python2.7/random.pyc’>), (‘re’, <module ‘re’ from ‘/usr/lib64/python2.7/re.pyc’>), (‘socket’, <module ‘socket’ from ‘/usr/lib64/python2.7/socket.pyc’>), (‘string’, <module ‘string’ from ‘/usr/lib64/python2.7/string.pyc’>), (‘subprocess’, <module ‘subprocess’ from ‘/usr/lib64/python2.7/subprocess.pyc’>), (‘sys’, <module ‘sys’ (built-in)>), (‘tempfile’, <module ‘tempfile’ from ‘/usr/lib64/python2.7/tempfile.pyc’>), (‘threading’, <module ‘threading’ from ‘/usr/lib64/python2.7/threading.pyc’>), (‘time’, <module ‘time’ from ‘/usr/lib64/python2.7/lib-dynload/timemodule.so’>), (‘traceback’, <module ‘traceback’ from ‘/usr/lib64/python2.7/traceback.pyc’>), (‘unittest’, <module ‘unittest’ from ‘/usr/lib64/python2.7/unittest/init.pyc’>)]

So I don’t think that it imports all the other test files, I think it only imports TestFunctional, and everything inside pbs_testsuite module, and its imports.

9

@agrawalravi90 : “from my_module import foo, bar” will more readable instead of ‘from tests.functional import *’. May be we can convert pbs_smoketest.py with this change and let run it. If we get better performance then we can add this as guideline.

Sorry for replying late.

1 Like
10

Oh my goodness, that’s a lot of extra stuff!
As a test writer, I do not think it is inconvenient to list out the
specific modules I need.
And as has already been mentioned earlier, it does make it easier for
others to be able to tell where something comes from.

11

@minghui or @agrawalravi90 (as @kjakkali mentioned) I would like to see data for performance and timing of execution of test using explicit import vs specific import before we discuss this further? Can one of you please do little POC and provide that please?

12

Thank you, @agrawalravi90. So really what most tests need is just the base class

from tests.functional import TestFunctional
13

I think it is 1-3 times. The only places where TestFunctional (or other derived classes from PBSTestSuite) are called are: A) when you define your class and derive it from TestFunctional B) In setUp() C) in tearDown(). The rest of PTL is accessed via self.

It doesn’t sound like much of an inconvenience to me.

Bhroam