xfconf: add return values and expand test coverage (#1419) (#1479)

* xfconf: add return values and expand test coverage

* fix pep8

* fix pylint

* fix returns yaml docs

* Add changelog fragemnt

* revert docts for `returned`

* Update changelogs/fragments/1419-xfconf-return-values.yaml

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/system/xfconf.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* update return values to raw for scalar/lists

* another doc tweak: None -> none

* Break newline for pep8

* Fix merge mistake

* Back to list of strings

* fix yaml syntax

* Fall back to old way, deprecate returns, add ingores for errors

* add a note about dprecating facts

* Add depracation messages and fix docstring error

* remove deprecation of return values.

* Update plugins/modules/system/xfconf.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* drop the deprecation message too

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 8e53b3df6f)

Co-authored-by: Matthew Campbell <calvinmc@gmail.com>

changelogs/fragments/1419-xfconf-return-values.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+- xfconf - add in missing return values that are specified in the documentation (https://github.com/ansible-collections/community.general/issues/1418).

plugins/modules/system/xfconf.py

@@ -95,20 +95,28 @@ RETURN = '''
     type: str
     sample: "/Xft/DPI"
   value_type:
-    description: The type of the value that was changed
+    description:
+    - The type of the value that was changed (C(none) for C(get) and C(reset)
+      state). Either a single string value or a list of strings for array
+      types.
     returned: success
-    type: str
+    type: string or list of strings
-    sample: "int"
+    sample: '"int" or ["str", "str", "str"]'
   value:
-    description: The value of the preference key after executing the module
+    description:
+    - The value of the preference key after executing the module. Either a
+      single string value or a list of strings for array types.
     returned: success
-    type: str
+    type: string or list of strings
-    sample: "192"
+    sample: '"192" or ["orange", "yellow", "violet"]'
   previous_value:
-    description: The value of the preference key before executing the module (None for "get" state).
+    description:
+    - The value of the preference key before executing the module (C(none) for
+      C(get) state). Either a single string value or a list of strings for array
+      types.
     returned: success
-    type: str
+    type: string or list of strings
-    sample: "96"
+    sample: '"96" or ["red", "blue", "green"]'
 '''
 
 from ansible_collections.community.general.plugins.module_utils.module_helper import (
@@ -176,6 +184,9 @@ class XFConfProperty(CmdMixin, StateMixin, ModuleHelper):
         self.does_not = 'Property "{0}" does not exist on channel "{1}".'.format(self.module.params['property'],
                                                                                  self.module.params['channel'])
         self.vars.previous_value = self._get()
+        self.update_xfconf_output(property=self.module.params['property'],
+                                  channel=self.module.params['channel'],
+                                  previous_value=None)
 
     def process_command_output(self, rc, out, err):
         if err.rstrip() == self.does_not:
@@ -205,10 +216,13 @@ class XFConfProperty(CmdMixin, StateMixin, ModuleHelper):
 
     def state_get(self):
         self.vars.value = self.vars.previous_value
+        self.update_xfconf_output(value=self.vars.value)
 
     def state_absent(self):
         self.vars.value = None
         self.run_command(params=('channel', 'property', 'reset'), extra_params={"reset": True})
+        self.update_xfconf_output(previous_value=self.vars.previous_value,
+                                  value=None)
 
     def state_present(self):
         # stringify all values - in the CLI they will all be happy strings anyway
@@ -249,6 +263,11 @@ class XFConfProperty(CmdMixin, StateMixin, ModuleHelper):
 
         if not self.vars.is_array:
             self.vars.value = self.vars.value[0]
+            value_type = value_type[0]
+
+        self.update_xfconf_output(previous_value=self.vars.previous_value,
+                                  value=self.vars.value,
+                                  type=value_type)
 
 
 def main():

tests/sanity/ignore-2.10.txt

@@ -552,6 +552,7 @@ plugins/modules/system/runit.py validate-modules:parameter-type-not-in-doc
 plugins/modules/system/runit.py validate-modules:undocumented-parameter
 plugins/modules/system/timezone.py pylint:blacklisted-name
 plugins/modules/system/xfconf.py validate-modules:parameter-state-invalid-choice
+plugins/modules/system/xfconf.py validate-modules:return-syntax-error
 plugins/modules/web_infrastructure/jenkins_plugin.py use-argspec-type-path
 plugins/modules/web_infrastructure/rundeck_acl_policy.py pylint:blacklisted-name
 scripts/inventory/gce.py pylint:blacklisted-name

tests/sanity/ignore-2.11.txt

@@ -552,6 +552,7 @@ plugins/modules/system/runit.py validate-modules:parameter-type-not-in-doc
 plugins/modules/system/runit.py validate-modules:undocumented-parameter
 plugins/modules/system/timezone.py pylint:blacklisted-name
 plugins/modules/system/xfconf.py validate-modules:parameter-state-invalid-choice
+plugins/modules/system/xfconf.py validate-modules:return-syntax-error
 plugins/modules/web_infrastructure/jenkins_plugin.py use-argspec-type-path
 plugins/modules/web_infrastructure/rundeck_acl_policy.py pylint:blacklisted-name
 scripts/inventory/gce.py pylint:blacklisted-name

tests/sanity/ignore-2.9.txt

@@ -480,6 +480,7 @@ plugins/modules/system/runit.py validate-modules:doc-default-does-not-match-spec
 plugins/modules/system/runit.py validate-modules:parameter-type-not-in-doc
 plugins/modules/system/runit.py validate-modules:undocumented-parameter
 plugins/modules/system/timezone.py pylint:blacklisted-name
+plugins/modules/system/xfconf.py validate-modules:return-syntax-error
 plugins/modules/web_infrastructure/jenkins_plugin.py use-argspec-type-path
 plugins/modules/web_infrastructure/nginx_status_facts.py validate-modules:deprecation-mismatch
 plugins/modules/web_infrastructure/nginx_status_facts.py validate-modules:invalid-documentation

tests/unit/plugins/modules/system/test_xfconf.py

@@ -55,7 +55,8 @@ TEST_CASES = [
                 ),
             ],
             'changed': False,
-            'previous_value': '100',
+            'previous_value': None,
+            'value_type': None,
             'value': '100',
         }
     ],
@@ -75,6 +76,7 @@ TEST_CASES = [
             ],
             'changed': False,
             'previous_value': None,
+            'value_type': None,
             'value': None,
         }
     ],
@@ -93,7 +95,8 @@ TEST_CASES = [
                 ),
             ],
             'changed': False,
-            'previous_value': ['Main', 'Work', 'Tmp'],
+            'previous_value': None,
+            'value_type': None,
             'value': ['Main', 'Work', 'Tmp'],
         },
     ],
@@ -112,7 +115,8 @@ TEST_CASES = [
                 ),
             ],
             'changed': False,
-            'previous_value': 'true',
+            'previous_value': None,
+            'value_type': None,
             'value': 'true',
         },
     ],
@@ -131,7 +135,8 @@ TEST_CASES = [
                 ),
             ],
             'changed': False,
-            'previous_value': 'false',
+            'previous_value': None,
+            'value_type': None,
             'value': 'false',
         },
     ],
@@ -166,6 +171,7 @@ TEST_CASES = [
             ],
             'changed': True,
             'previous_value': '100',
+            'value_type': 'int',
             'value': '90',
         },
     ],
@@ -200,6 +206,7 @@ TEST_CASES = [
             ],
             'changed': False,
             'previous_value': '90',
+            'value_type': 'int',
             'value': '90',
         },
     ],
@@ -235,6 +242,7 @@ TEST_CASES = [
             ],
             'changed': True,
             'previous_value': ['Main', 'Work', 'Tmp'],
+            'value_type': ['str', 'str', 'str'],
             'value': ['A', 'B', 'C'],
         },
     ],
@@ -270,6 +278,7 @@ TEST_CASES = [
             ],
             'changed': False,
             'previous_value': ['A', 'B', 'C'],
+            'value_type': ['str', 'str', 'str'],
             'value': ['A', 'B', 'C'],
         },
     ],
@@ -302,6 +311,7 @@ TEST_CASES = [
             ],
             'changed': True,
             'previous_value': ['A', 'B', 'C'],
+            'value_type': None,
             'value': None,
         },
     ],
@@ -333,14 +343,20 @@ def test_xfconf(mocker, capfd, patch_xfconf, testcase):
     results = json.loads(out)
     print("testcase =\n%s" % testcase)
     print("results =\n%s" % results)
+
     assert 'changed' in results
     assert results['changed'] == testcase['changed']
-    if 'msg' in results:
+
-        assert results.get('msg') == testcase['msg']
+    for test_result in ('channel', 'property'):
-    if 'value' in results:
+        assert test_result in results, "'{0}' not found in {1}".format(test_result, results)
-        assert results['value'] == testcase['value']
+        assert results[test_result] == results['invocation']['module_args'][test_result], \
-    if 'previous_value' in results:
+            "'{0}': '{1}' != '{2}'".format(test_result, results[test_result], results['invocation']['module_args'][test_result])
-        assert results['previous_value'] == testcase['previous_value']
+
+    for conditional_test_result in ('msg', 'value', 'previous_value'):
+        if conditional_test_result in testcase:
+            assert conditional_test_result in results, "'{0}' not found in {1}".format(conditional_test_result, results)
+            assert results[conditional_test_result] == testcase[conditional_test_result], \
+                "'{0}': '{1}' != '{2}'".format(conditional_test_result, results[conditional_test_result], testcase[conditional_test_result])
 
     assert mock_run_command.call_count == len(testcase['run_command.calls'])
     if mock_run_command.call_count: