190 lines
4.9 KiB
ReStructuredText
190 lines
4.9 KiB
ReStructuredText
..
|
|
This work is licensed under a Creative Commons Attribution 3.0 Unported
|
|
License.
|
|
|
|
http://creativecommons.org/licenses/by/3.0/legalcode
|
|
|
|
=====================================================
|
|
Remove the use of resource autodeletion in unit tests
|
|
=====================================================
|
|
|
|
Launchpad blueprint:
|
|
|
|
https://blueprints.launchpad.net/neutron/+spec/remove-unit-test-autodeletion
|
|
|
|
This blueprint describes the rational for removing resource
|
|
autodeletion in the unit tests.
|
|
|
|
|
|
Problem description
|
|
===================
|
|
|
|
The Neutron unit tests currently decorate resource creation methods
|
|
with contextlib.contextmanager to allow resources to be automatically
|
|
deleted when they are no longer needed. This strategy is a legacy of
|
|
a time when this explicit cleanup was required to ensure isolation
|
|
between tests, but is now unnecessary since db state is automatically
|
|
thrown away after each test. The strategy has the following negative
|
|
impacts on much of Neutron's unit test suite:
|
|
|
|
- an execution penalty, since deletion of every created resource is
|
|
always done and is performed through the wsgi layer
|
|
- a debugging penalty, since contextmanagers are generators with the
|
|
consequent loss of stack context
|
|
- a readability penalty, since the test code is riddled with
|
|
unnecessary 'with' blocks around resource creation
|
|
- a coverage penalty, since implicit deletion testing is likely to
|
|
hide gaps in testing of resource deletion
|
|
|
|
|
|
Proposed change
|
|
===============
|
|
|
|
Resource creation methods should be modified to not perform
|
|
autodeletion, as follows:
|
|
|
|
- remove the contextlib.contextmanager decorator
|
|
- change 'yield' statements to 'return'
|
|
- remove the do_delete or no_delete parameter
|
|
- remove the conditional block that deletes the resource(s)
|
|
|
|
In addition, all clients of the resource creation methods should be
|
|
updated to no longer pass a do_delete/no_delete parameter and perform
|
|
a regular call (e.g. create_resource()) rather than creating a with
|
|
block (e.g. with create_resource():). It also may make sense to
|
|
update the names of resource creation methods, since they are often of
|
|
the form 'resourcename' rather than 'create_resourcename'.
|
|
|
|
Care will have to be taken to ensure that explicit tests for deletion
|
|
are added if they do not already exist for a given resource type.
|
|
Autodeletion ensured that any resource whose creation was tested also
|
|
had its deletion validated to some extent, but this validation will
|
|
have to be explicit from now on. This is arguably desirable, since
|
|
relying on the 'magic' of autodeletion to provide test coverage can
|
|
potentially lull developers into a false sense of security.
|
|
|
|
There may also be tests that create multiple resources in a way that
|
|
depended on autodeltion to prevent uniqueness constraint violation.
|
|
It will be necessary to perform explicit deletion in those cases where
|
|
removal of autodeletion provokes test failures.
|
|
|
|
|
|
Alternatives
|
|
------------
|
|
|
|
The alternative to not removing autodeletion is continuing to live
|
|
with the penalties it imposes.
|
|
|
|
|
|
Data model impact
|
|
-----------------
|
|
|
|
None
|
|
|
|
|
|
REST API impact
|
|
---------------
|
|
|
|
None
|
|
|
|
Security impact
|
|
---------------
|
|
|
|
None
|
|
|
|
|
|
Notifications impact
|
|
--------------------
|
|
|
|
None
|
|
|
|
|
|
Other end user impact
|
|
---------------------
|
|
|
|
None
|
|
|
|
|
|
Performance Impact
|
|
------------------
|
|
|
|
Removal of autodeletion should have a positive impact on the execution
|
|
time of the unit test suite.
|
|
|
|
|
|
Other deployer impact
|
|
---------------------
|
|
|
|
None
|
|
|
|
|
|
Developer impact
|
|
----------------
|
|
|
|
Developers and reviewers familiar with the current
|
|
contextmanager-based model of resource creation in unit tests will
|
|
have to be educated as to the move to simpler creation methods and the
|
|
need for explicit testing of deletion.
|
|
|
|
Once concern voiced at the removal of the contextmanager decorator was
|
|
that it would disallow the convenience of managing the lifecycle of
|
|
multiple resources via contextlib.nested. If necessary, this
|
|
capability can be replicated by creating a helper method that accepts
|
|
a list of creation functions and ensures that the resources created
|
|
are cleaned up.
|
|
|
|
|
|
Implementation
|
|
==============
|
|
|
|
Assignee(s)
|
|
-----------
|
|
|
|
Primary assignee:
|
|
kevinbenton
|
|
|
|
Other contributors:
|
|
marun
|
|
|
|
Work Items
|
|
----------
|
|
|
|
1. Add explicit tests for deletion for resource types that have
|
|
previously relied on autodeletion to perform implicit testing of
|
|
deletion.
|
|
|
|
2. Disable autodeletion in each resource creation method
|
|
(e.g. NeutronDbPluginV2TestCase.network) in the unit test suite
|
|
and fix any test failures that result.
|
|
|
|
3. Remove support for autodeletion (parameters, deletion calls) from
|
|
the resource creation methods and update their callers.
|
|
|
|
4. Remove the contextmanager decorator from resource creation
|
|
methods, rename the creation methods (e.g. network ->
|
|
create_network), and update their clients.
|
|
|
|
|
|
Dependencies
|
|
============
|
|
|
|
None
|
|
|
|
|
|
Testing
|
|
=======
|
|
|
|
None
|
|
|
|
|
|
Documentation Impact
|
|
====================
|
|
|
|
None
|
|
|
|
|
|
References
|
|
==========
|
|
|
|
None
|