Skip to content

Commit

Permalink
tests: avoid list() and dict() as default parameter value
Browse files Browse the repository at this point in the history
"Default parameter values are evaluated from left to right when the
function definition is executed." which means that the list or dict
is created only once and then the same reference is passed to the
function with each invokation, so the function calls can be affected
by the previous one.

See:
https://docs.python.org/3/reference/compound_stmts.html#function-definitions

Reviewed-by: Jakub Vávra <[email protected]>
  • Loading branch information
pbrezina committed Apr 4, 2023
1 parent a5efc5e commit 3d0fcca
Show file tree
Hide file tree
Showing 9 changed files with 116 additions and 64 deletions.
9 changes: 6 additions & 3 deletions src/tests/system/lib/sssd/misc/ssh.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def __init__(
argv: list[Any],
*,
cwd: str | None = None,
env: dict[str, Any] = dict(),
env: dict[str, Any] | None = None,
input: str | None = None,
read_timeout: float = 2,
log_level: SSHLog = SSHLog.Full,
Expand All @@ -31,15 +31,18 @@ def __init__(
:type argv: list[Any]
:param cwd: Working directory, defaults to None (= do not change)
:type cwd: str | None, optional
:param env: Additional environment variables, defaults to dict()
:type env: dict[str, Any], optional
:param env: Additional environment variables, defaults to None
:type env: dict[str, Any] | None, optional
:param input: Content of standard input, defaults to None
:type input: str | None, optional
:param read_timeout: Timeout in seconds, how long should the client wait for output, defaults to 30 seconds
:type read_timeout: float, optional
:param log_level: Log level, defaults to SSHLog.Full
:type log_level: SSHLog, optional
"""
if env is None:
env = {}

argv = to_list_of_strings(argv)
command = shlex.join(argv)
pidfile = "/tmp/.mh.sshkillableprocess.pid"
Expand Down
9 changes: 6 additions & 3 deletions src/tests/system/lib/sssd/roles/ad.py
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ def _path(self, basedn: ADObject | str | None = None) -> str:

return f"{basedn},{self.role.host.naming_context}"

def _exec(self, op: str, args: list[str] | str = list(), **kwargs) -> SSHProcessResult:
def _exec(self, op: str, args: list[str] | str | None = None, **kwargs) -> SSHProcessResult:
"""
Execute AD command.
Expand All @@ -380,11 +380,14 @@ def _exec(self, op: str, args: list[str] | str = list(), **kwargs) -> SSHProcess
:param op: Command group operation (usually New, Set, Remove, Get)
:type op: str
:param args: List of additional command arguments, defaults to list()
:type args: list[str], optional
:param args: List of additional command arguments, defaults to None
:type args: list[str] | None, optional
:return: SSH process result.
:rtype: SSHProcessResult
"""
if args is None:
args = []

if isinstance(args, list):
args = " ".join(args)
elif args is None:
Expand Down
36 changes: 24 additions & 12 deletions src/tests/system/lib/sssd/roles/ipa.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ def __init__(self, role: IPA, name: str, command_group: str) -> None:
self.name: str = name
"""Object name."""

def _exec(self, op: str, args: list[str] = list(), **kwargs) -> SSHProcessResult:
def _exec(self, op: str, args: list[str] | None = None, **kwargs) -> SSHProcessResult:
"""
Execute IPA command.
Expand All @@ -234,22 +234,28 @@ def _exec(self, op: str, args: list[str] = list(), **kwargs) -> SSHProcessResult
:param op: Command group operation (usually add, mod, del, show)
:type op: str
:param args: List of additional command arguments, defaults to list()
:type args: list[str], optional
:param args: List of additional command arguments, defaults to None
:type args: list[str] | None, optional
:return: SSH process result.
:rtype: SSHProcessResult
"""
if args is None:
args = []

return self.role.host.ssh.exec(["ipa", f"{self.command_group}-{op}", self.name, *args], **kwargs)

def _add(self, attrs: CLIBuilderArgs = dict(), input: str | None = None):
def _add(self, attrs: CLIBuilderArgs | None = None, input: str | None = None):
"""
Add IPA object.
:param attrs: Object attributes in :class:`pytest_mh.cli.CLIBuilder` format, defaults to dict()
:type attrs: pytest_mh.cli.CLIBuilderArgs, optional
:param attrs: Object attributes in :class:`pytest_mh.cli.CLIBuilder` format, defaults to None
:type attrs: pytest_mh.cli.CLIBuilderArgs | None, optional
:param input: Contents of standard input given to the executed command, defaults to None
:type input: str | None, optional
"""
if attrs is None:
attrs = {}

self._exec("add", self.cli.args(attrs), input=input)

def _modify(self, attrs: CLIBuilderArgs, input: str | None = None):
Expand Down Expand Up @@ -924,7 +930,7 @@ def __get_location(self, location: IPAAutomountLocation | str) -> IPAAutomountLo
else:
raise ValueError(f"Unexepected location type: {type(location)}")

def _exec(self, op: str, args: list[str] = list(), **kwargs) -> SSHProcessResult:
def _exec(self, op: str, args: list[str] | None = None, **kwargs) -> SSHProcessResult:
"""
Execute automoutmap IPA command.
Expand All @@ -935,11 +941,14 @@ def _exec(self, op: str, args: list[str] = list(), **kwargs) -> SSHProcessResult
:param op: Command group operation (usually add, mod, del, show)
:type op: str
:param args: List of additional command arguments, defaults to list()
:type args: list[str], optional
:param args: List of additional command arguments, defaults to None
:type args: list[str] | None, optional
:return: SSH process result.
:rtype: SSHProcessResult
"""
if args is None:
args = []

defargs = self.cli.args(
{
"location": (self.cli.option.POSITIONAL, self.location.name),
Expand Down Expand Up @@ -995,7 +1004,7 @@ def __init__(
self.map: IPAAutomountMap = map
self.info: str | None = None

def _exec(self, op: str, args: list[str] = list(), **kwargs) -> SSHProcessResult:
def _exec(self, op: str, args: list[str] | None = None, **kwargs) -> SSHProcessResult:
"""
Execute automoutkey IPA command.
Expand All @@ -1006,11 +1015,14 @@ def _exec(self, op: str, args: list[str] = list(), **kwargs) -> SSHProcessResult
:param op: Command group operation (usually add, mod, del, show)
:type op: str
:param args: List of additional command arguments, defaults to list()
:type args: list[str], optional
:param args: List of additional command arguments, defaults to None
:type args: list[str] | None, optional
:return: SSH process result.
:rtype: SSHProcessResult
"""
if args is None:
args = []

defargs = self.cli.args(
{
"location": (self.cli.option.POSITIONAL, self.map.location.name),
Expand Down
18 changes: 9 additions & 9 deletions src/tests/system/lib/sssd/roles/ldap.py
Original file line number Diff line number Diff line change
Expand Up @@ -412,19 +412,19 @@ def _add(self, attrs: LDAPRecordAttributes) -> None:
def _modify(
self,
*,
add: LDAPRecordAttributes = dict(),
replace: LDAPRecordAttributes = dict(),
delete: LDAPRecordAttributes = dict(),
add: LDAPRecordAttributes | None = None,
replace: LDAPRecordAttributes | None = None,
delete: LDAPRecordAttributes | None = None,
) -> None:
"""
Modify LDAP record.
:param add: Attributes and values to add, defaults to dict()
:type add: LDAPRecordAttributes, optional
:param replace: Attributes and values to replace, defaults to dict()
:type replace: LDAPRecordAttributes, optional
:param delete: Attributes and values to delete, defaults to dict()
:type delete: LDAPRecordAttributes, optional
:param add: Attributes and values to add, defaults to None
:type add: LDAPRecordAttributes | None, optional
:param replace: Attributes and values to replace, defaults to None
:type replace: LDAPRecordAttributes | None, optional
:param delete: Attributes and values to delete, defaults to None
:type delete: LDAPRecordAttributes | None, optional
"""
self.role.ldap.modify(self.dn, add=add, replace=replace, delete=delete)

Expand Down
9 changes: 6 additions & 3 deletions src/tests/system/lib/sssd/roles/samba.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def __init__(self, role: Samba, command: str, name: str) -> None:
self.name: str = name
"""Object name."""

def _exec(self, op: str, args: list[str] = list(), **kwargs) -> SSHProcessResult:
def _exec(self, op: str, args: list[str] | None = None, **kwargs) -> SSHProcessResult:
"""
Execute samba-tool command.
Expand All @@ -261,11 +261,14 @@ def _exec(self, op: str, args: list[str] = list(), **kwargs) -> SSHProcessResult
:param op: Command group operation (usually add, delete, show)
:type op: str
:param args: List of additional command arguments, defaults to list()
:type args: list[str], optional
:param args: List of additional command arguments, defaults to None
:type args: list[str] | None, optional
:return: SSH process result.
:rtype: SSHProcessResult
"""
if args is None:
args = []

return self.role.host.ssh.exec(["samba-tool", self.command, op, self.name, *args], **kwargs)

def _add(self, attrs: CLIBuilderArgs) -> None:
Expand Down
12 changes: 8 additions & 4 deletions src/tests/system/lib/sssd/topology.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,25 @@ def test_fixture_name(fixture1: BaseRole, fixture2: BaseRole, ...):
"""

def __init__(
self, name: str, topology: Topology, fixtures: dict[str, str] = dict(), domains: dict[str, str] = dict()
self,
name: str,
topology: Topology,
fixtures: dict[str, str] | None = None,
domains: dict[str, str] | None = None,
) -> None:
"""
:param name: Topology name used in pytest output.
:type name: str
:param topology: Topology required to run the test.
:type topology: Topology
:param fixtures: Dynamically created fixtures available during the test run.
:type fixtures: dict[str, str], optional
:type fixtures: dict[str, str] | None, optional
:param domains: Automatically created SSSD domains on client host
:type domains: dict[str, str], optional
:type domains: dict[str, str] | None, optional
"""
super().__init__(name, topology, fixtures)

self.domains: dict[str, str] = domains
self.domains: dict[str, str] = domains if domains is not None else {}
"""Map hosts to SSSD domains."""

def export(self) -> dict:
Expand Down
33 changes: 21 additions & 12 deletions src/tests/system/lib/sssd/utils/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ def __init__(self, host: MultihostHost, ssh: SSHClient | None = None) -> None:
"""SSH client for the target user."""

def kinit(
self, principal: str, *, password: str, realm: str | None = None, args: list[str] = list()
self, principal: str, *, password: str, realm: str | None = None, args: list[str] | None = None
) -> SSHProcessResult:
"""
Run ``kinit`` command.
Expand All @@ -487,17 +487,20 @@ def kinit(
:type password: str
:param realm: Kerberos realm that is appended to the principal (``$principal@$realm``), defaults to None
:type realm: str | None, optional
:param args: Additional parameters to ``klist``, defaults to list()
:type args: list[str], optional
:param args: Additional parameters to ``klist``, defaults to None
:type args: list[str] | None, optional
:return: Command result.
:rtype: SSHProcessResult
"""
if args is None:
args = []

if realm is not None:
principal = f"{principal}@{realm}"

return self.ssh.exec(["kinit", *args, principal], input=password)

def kvno(self, principal: str, *, realm: str | None = None, args: list[str] = list()) -> SSHProcessResult:
def kvno(self, principal: str, *, realm: str | None = None, args: list[str] | None = None) -> SSHProcessResult:
"""
Run ``kvno`` command.
Expand All @@ -511,25 +514,31 @@ def kvno(self, principal: str, *, realm: str | None = None, args: list[str] = li
:type principal: str
:param realm: Kerberos realm that is appended to the principal (``$principal@$realm``), defaults to None
:type realm: str | None, optional
:param args: Additional parameters to ``klist``, defaults to list()
:type args: list[str], optional
:param args: Additional parameters to ``klist``, defaults to None
:type args: list[str] | None, optional
:return: Command result.
:rtype: SSHProcessResult
"""
if args is None:
args = []

if realm is not None:
principal = f"{principal}@{realm}"

return self.ssh.exec(["kvno", *args, principal])

def klist(self, *, args: list[str] = list()) -> SSHProcessResult:
def klist(self, *, args: list[str] | None = None) -> SSHProcessResult:
"""
Run ``klist`` command.
:param args: Additional parameters to ``klist``, defaults to list()
:type args: list[str], optional
:param args: Additional parameters to ``klist``, defaults to None
:type args: list[str] | None, optional
:return: Command result.
:rtype: SSHProcessResult
"""
if args is None:
args = []

return self.ssh.exec(["klist", *args])

def kswitch(self, principal: str, realm: str) -> SSHProcessResult:
Expand Down Expand Up @@ -673,12 +682,12 @@ def cache_count(self) -> int:

return len(result.stdout_lines) - 2

def list_principals(self, env: dict[str, Any] = dict()) -> dict[str, list[str]]:
def list_principals(self, env: dict[str, Any] | None = None) -> dict[str, list[str]]:
"""
List all principals that have existing credential cache.
:param env: Additional environment variables passed to ``klist -A`` command, defaults to dict()
:type env: dict[str, Any], optional
:param env: Additional environment variables passed to ``klist -A`` command, defaults to None
:type env: dict[str, Any] | None, optional
:return: Dictionary with principal as the key and list of available tickets as value.
:rtype: dict[str, list[str]]
"""
Expand Down
27 changes: 18 additions & 9 deletions src/tests/system/lib/sssd/utils/ldap.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,24 +110,33 @@ def modify(
self,
dn: str,
*,
add: LDAPRecordAttributes = dict(),
replace: LDAPRecordAttributes = dict(),
delete: LDAPRecordAttributes = dict(),
add: LDAPRecordAttributes | None = None,
replace: LDAPRecordAttributes | None = None,
delete: LDAPRecordAttributes | None = None,
) -> None:
"""
Modify LDAP entry.
:param dn: Distinguished name.
:type dn: str
:param add: Attributes to add, defaults to dict()
:type add: LDAPRecordAttributes, optional
:param replace: Attributes to replace, defaults to dict()
:type replace: LDAPRecordAttributes, optional
:param delete: Attributes to delete, defaults to dict()
:type delete: LDAPRecordAttributes, optional
:param add: Attributes to add, defaults to None
:type add: LDAPRecordAttributes | None, optional
:param replace: Attributes to replace, defaults to None
:type replace: LDAPRecordAttributes | None, optional
:param delete: Attributes to delete, defaults to None
:type delete: LDAPRecordAttributes | None, optional
"""
modlist = []

if add is None:
add = {}

if replace is None:
replace = {}

if delete is None:
delete = {}

for attr, values in add.items():
modlist.append((ldap.MOD_ADD, attr, self.__values_to_bytes(values)))

Expand Down
Loading

0 comments on commit 3d0fcca

Please sign in to comment.