Skip to content

Commit

Permalink
reduce pytest warnings (#3674)
Browse files Browse the repository at this point in the history
 * Update docs for pytest.

 * Bring pytest warnings from 423 down to 31
   * fix usage of deprecated unittest api
   * fix tests skipped because of `__init__` constructor
   * fix classes incorrectly identified as tests
   * fix deprecated usage of external modules
   * fix incorrect escape sequences in regular expressions
   * ignore unnecessary warnings from external packages
     **Note:** this could be made more flexible (e.g. narrowing down filters), since such warnings can sometimes be useful, e.g. if we are using a deprecated API of the package (say, django). Most of the time, it is the external package that is using some deprecated API.

 * add individual test timings
  • Loading branch information
ltalirz authored Dec 16, 2019
1 parent 8eeb432 commit a73e4b6
Show file tree
Hide file tree
Showing 22 changed files with 357 additions and 358 deletions.
2 changes: 2 additions & 0 deletions .github/workflows/tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ set -ev

# Make sure the folder containing the workchains is in the python path before the daemon is started
export PYTHONPATH="${PYTHONPATH}:${GITHUB_WORKSPACE}/.ci"
# show timings of tests
export PYTEST_ADDOPTS=" --durations=0"

verdi daemon start 4
verdi -p test_${AIIDA_TEST_BACKEND} run .ci/test_daemon.py
Expand Down
8 changes: 1 addition & 7 deletions aiida/backends/tests/cmdline/commands/test_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@
class TestVerdiDataExportable:
"""Test exportable data objects."""

def __init__(self):
pass

NODE_ID_STR = 'node_id'
EMPTY_GROUP_ID_STR = 'empty_group_id'
EMPTY_GROUP_NAME_STR = 'empty_group'
Expand Down Expand Up @@ -87,7 +84,7 @@ def data_export_test(self, datatype, ids, supported_formats):
# Try to export it again. It should fail because the
# file exists
res = self.cli_runner.invoke(export_cmd, options, catch_exceptions=False)
self.assertNotEquals(res.exit_code, 0, 'The command should fail because the file already exists')
self.assertNotEqual(res.exit_code, 0, 'The command should fail because the file already exists')

# Now we force the export of the file and it should overwrite
# existing files
Expand All @@ -104,9 +101,6 @@ def data_export_test(self, datatype, ids, supported_formats):
class TestVerdiDataListable:
"""Test listable data objects."""

def __init__(self):
pass

NODE_ID_STR = 'node_id'
EMPTY_GROUP_ID_STR = 'empty_group_id'
EMPTY_GROUP_NAME_STR = 'empty_group'
Expand Down
22 changes: 11 additions & 11 deletions aiida/backends/tests/common/test_extendeddicts.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@
from aiida.common import extendeddicts


class TestFFADExample(extendeddicts.FixedFieldsAttributeDict):
class FFADExample(extendeddicts.FixedFieldsAttributeDict):
"""
An example class that accepts only the 'alpha', 'beta' and 'gamma' keys/attributes.
"""
_valid_fields = ('alpha', 'beta', 'gamma')


class TestDFADExample(extendeddicts.DefaultFieldsAttributeDict):
class DFADExample(extendeddicts.DefaultFieldsAttributeDict):
"""
An example class that has 'alpha', 'beta' and 'gamma' as default keys.
"""
Expand Down Expand Up @@ -270,7 +270,7 @@ class TestFFAD(unittest.TestCase):

def test_insertion(self):
"""Test insertion."""
dictionary = TestFFADExample()
dictionary = FFADExample()
dictionary['alpha'] = 1
dictionary.beta = 2
# Not a valid key.
Expand All @@ -281,14 +281,14 @@ def test_insertion(self):

def test_insertion_on_init(self):
"""Test insertion in constructor."""
TestFFADExample({'alpha': 1, 'beta': 2})
FFADExample({'alpha': 1, 'beta': 2})
with self.assertRaises(KeyError):
# 'delta' is not a valid key
TestFFADExample({'alpha': 1, 'delta': 2})
FFADExample({'alpha': 1, 'delta': 2})

def test_pickle(self):
"""Note: pickle works here because self._valid_fields is defined at the class level!"""
dictionary_01 = TestFFADExample({'alpha': 1, 'beta': 2})
dictionary_01 = FFADExample({'alpha': 1, 'beta': 2})
dictionary_02 = pickle.loads(pickle.dumps(dictionary_01))
dictionary_02.gamma = 3
with self.assertRaises(KeyError):
Expand All @@ -299,15 +299,15 @@ def test_class_attribute(self):
I test that the get_valid_fields() is working as a class method,
so I don't need to instantiate the class to get the list.
"""
self.assertEqual(set(TestFFADExample.get_valid_fields()), set(['alpha', 'beta', 'gamma']))
self.assertEqual(set(FFADExample.get_valid_fields()), set(['alpha', 'beta', 'gamma']))


class TestDFAD(unittest.TestCase):
"""Test for the default fields attribute dictionary."""

def test_insertion_and_retrieval(self):
"""Test insertion and retrieval."""
dictionary = TestDFADExample()
dictionary = DFADExample()
dictionary['alpha'] = 1
dictionary.beta = 2
dictionary['delta'] = 3
Expand All @@ -319,7 +319,7 @@ def test_insertion_and_retrieval(self):

def test_keylist_method(self):
"""Test keylist retrieval."""
dictionary = TestDFADExample()
dictionary = DFADExample()
dictionary['alpha'] = 1
dictionary.beta = 2
dictionary['delta'] = 3
Expand All @@ -336,11 +336,11 @@ def test_class_attribute(self):
I test that the get_default_fields() is working as a class method,
so I don't need to instantiate the class to get the list.
"""
self.assertEqual(set(TestDFADExample.get_default_fields()), set(['alpha', 'beta', 'gamma']))
self.assertEqual(set(DFADExample.get_default_fields()), set(['alpha', 'beta', 'gamma']))

def test_validation(self):
"""Test validation."""
dictionary = TestDFADExample()
dictionary = DFADExample()

# Should be ok to have an empty 'alpha' attribute
dictionary.validate()
Expand Down
30 changes: 15 additions & 15 deletions aiida/backends/tests/engine/test_process_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
###########################################################################
"""Module to test process builder."""
# pylint: disable=no-member,protected-access,no-name-in-module
from collections import Mapping, MutableMapping
from collections.abc import Mapping, MutableMapping

from aiida import orm
from aiida.backends.testbase import AiidaTestCase
Expand All @@ -20,7 +20,7 @@
DEFAULT_INT = 256


class TestWorkChain(WorkChain):
class ExampleWorkChain(WorkChain):
"""Defining test work chain."""

@classmethod
Expand All @@ -34,7 +34,7 @@ def define(cls, spec):
spec.input('default', valid_type=orm.Int, default=orm.Int(DEFAULT_INT).store())


class TestLazyProcessNamespace(Process):
class LazyProcessNamespace(Process):
"""Process with basic nested namespaces to test "pruning" of empty nested namespaces from the builder."""

@classmethod
Expand Down Expand Up @@ -74,7 +74,7 @@ def setUp(self):
self.assertIsNone(Process.current())
self.process_class = CalculationFactory('templatereplacer')
self.builder = self.process_class.get_builder()
self.builder_workchain = TestWorkChain.get_builder()
self.builder_workchain = ExampleWorkChain.get_builder()
self.inputs = {
'dynamic': {
'namespace': {
Expand All @@ -95,34 +95,34 @@ def tearDown(self):

def test_builder_inputs(self):
"""Test the `ProcessBuilder._inputs` method to get the inputs with and without `prune` set to True."""
builder = TestLazyProcessNamespace.get_builder()
builder = LazyProcessNamespace.get_builder()

# When no inputs are specified specifically, `prune=True` should get rid of completely empty namespaces
self.assertEqual(builder._inputs(prune=False), {'namespace': {'nested': {}}, 'metadata': {}})
self.assertEqual(builder._inputs(prune=True), {})

# With a specific input in `namespace` the case of `prune=True` should now only remove `metadata`
integer = orm.Int(DEFAULT_INT)
builder = TestLazyProcessNamespace.get_builder()
builder = LazyProcessNamespace.get_builder()
builder.namespace.a = integer
self.assertEqual(builder._inputs(prune=False), {'namespace': {'a': integer, 'nested': {}}, 'metadata': {}})
self.assertEqual(builder._inputs(prune=True), {'namespace': {'a': integer}})

# A value that is a `Node` instance but also happens to be an "empty mapping" should not be pruned
empty_node = MappingData()
builder = TestLazyProcessNamespace.get_builder()
builder = LazyProcessNamespace.get_builder()
builder.namespace.a = empty_node
self.assertEqual(builder._inputs(prune=False), {'namespace': {'a': empty_node, 'nested': {}}, 'metadata': {}})
self.assertEqual(builder._inputs(prune=True), {'namespace': {'a': empty_node}})

# Verify that empty lists are considered as a "value" and are not pruned
builder = TestLazyProcessNamespace.get_builder()
builder = LazyProcessNamespace.get_builder()
builder.namespace.c = list()
self.assertEqual(builder._inputs(prune=False), {'namespace': {'c': [], 'nested': {}}, 'metadata': {}})
self.assertEqual(builder._inputs(prune=True), {'namespace': {'c': []}})

# Verify that empty lists, even in doubly nested namespace are considered as a "value" and are not pruned
builder = TestLazyProcessNamespace.get_builder()
builder = LazyProcessNamespace.get_builder()
builder.namespace.nested.bird = list()
self.assertEqual(builder._inputs(prune=False), {'namespace': {'nested': {'bird': []}}, 'metadata': {}})
self.assertEqual(builder._inputs(prune=True), {'namespace': {'nested': {'bird': []}}})
Expand Down Expand Up @@ -172,15 +172,15 @@ def test_dynamic_getters_value(self):

def test_dynamic_getters_doc_string(self):
"""Verify that getters have the correct docstring."""
builder = TestWorkChain.get_builder()
self.assertEqual(builder.__class__.name_spaced.__doc__, str(TestWorkChain.spec().inputs['name_spaced']))
self.assertEqual(builder.__class__.boolean.__doc__, str(TestWorkChain.spec().inputs['boolean']))
builder = ExampleWorkChain.get_builder()
self.assertEqual(builder.__class__.name_spaced.__doc__, str(ExampleWorkChain.spec().inputs['name_spaced']))
self.assertEqual(builder.__class__.boolean.__doc__, str(ExampleWorkChain.spec().inputs['boolean']))

def test_builder_restart_work_chain(self):
"""Verify that nested namespaces imploded into flat link labels can be reconstructed into nested namespaces."""
caller = orm.WorkChainNode().store()

node = orm.WorkChainNode(process_type=TestWorkChain.build_process_type())
node = orm.WorkChainNode(process_type=ExampleWorkChain.build_process_type())
node.add_incoming(self.inputs['dynamic']['namespace']['alp'], LinkType.INPUT_WORK, 'dynamic__namespace__alp')
node.add_incoming(self.inputs['name']['spaced'], LinkType.INPUT_WORK, 'name__spaced')
node.add_incoming(self.inputs['name_spaced'], LinkType.INPUT_WORK, 'name_spaced')
Expand Down Expand Up @@ -211,7 +211,7 @@ def test_port_names_overlapping_mutable_mapping_methods(self): # pylint: disabl
as attributes, they can overlap with some of the mappings builtin methods, e.g. `values()`, `items()` etc.
The port names should take precendence in this case and if one wants to access the mapping methods one needs to
cast the builder to a dictionary first."""
builder = TestWorkChain.get_builder()
builder = ExampleWorkChain.get_builder()

# The `values` method is obscured by a port that also happens to be called `values`, so calling it should raise
with self.assertRaises(TypeError):
Expand All @@ -232,7 +232,7 @@ def test_port_names_overlapping_mutable_mapping_methods(self): # pylint: disabl
self.assertNotIn(method, dir(builder))

# On the other hand, all the port names *should* be present
for port_name in TestWorkChain.spec().inputs.keys():
for port_name in ExampleWorkChain.spec().inputs.keys():
self.assertIn(port_name, dir(builder))

# The `update` method is implemented, but prefixed with an underscore to not block the name for a port
Expand Down
Loading

0 comments on commit a73e4b6

Please sign in to comment.