From 8e53b3df6f7047f724d9bb8f85b9e0a9733c7887 Mon Sep 17 00:00:00 2001 From: Matthew Campbell Date: Sat, 12 Dec 2020 03:07:30 -0500 Subject: [PATCH] xfconf: add return values and expand test coverage (#1419) * 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 * Update plugins/modules/system/xfconf.py Co-authored-by: Felix Fontein * 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 * drop the deprecation message too Co-authored-by: Felix Fontein --- .../fragments/1419-xfconf-return-values.yaml | 2 + plugins/modules/system/xfconf.py | 37 ++++++++++++++----- tests/sanity/ignore-2.10.txt | 1 + tests/sanity/ignore-2.11.txt | 1 + tests/sanity/ignore-2.9.txt | 1 + .../plugins/modules/system/test_xfconf.py | 36 +++++++++++++----- 6 files changed, 59 insertions(+), 19 deletions(-) create mode 100644 changelogs/fragments/1419-xfconf-return-values.yaml diff --git a/changelogs/fragments/1419-xfconf-return-values.yaml b/changelogs/fragments/1419-xfconf-return-values.yaml new file mode 100644 index 0000000000..6d3b4eb67b --- /dev/null +++ b/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). diff --git a/plugins/modules/system/xfconf.py b/plugins/modules/system/xfconf.py index 77c877fb7b..8d0700ae11 100644 --- a/plugins/modules/system/xfconf.py +++ b/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 - sample: "int" + type: string or list of strings + 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 - sample: "192" + type: string or list of strings + 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 - sample: "96" + type: string or list of strings + 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(): diff --git a/tests/sanity/ignore-2.10.txt b/tests/sanity/ignore-2.10.txt index 69bae9d3f7..78db9c2d12 100644 --- a/tests/sanity/ignore-2.10.txt +++ b/tests/sanity/ignore-2.10.txt @@ -500,6 +500,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 diff --git a/tests/sanity/ignore-2.11.txt b/tests/sanity/ignore-2.11.txt index 69bae9d3f7..78db9c2d12 100644 --- a/tests/sanity/ignore-2.11.txt +++ b/tests/sanity/ignore-2.11.txt @@ -500,6 +500,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 diff --git a/tests/sanity/ignore-2.9.txt b/tests/sanity/ignore-2.9.txt index f86bcc67ef..6907ac709b 100644 --- a/tests/sanity/ignore-2.9.txt +++ b/tests/sanity/ignore-2.9.txt @@ -400,6 +400,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 diff --git a/tests/unit/plugins/modules/system/test_xfconf.py b/tests/unit/plugins/modules/system/test_xfconf.py index d29e61e76b..1002952ce3 100644 --- a/tests/unit/plugins/modules/system/test_xfconf.py +++ b/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'] - if 'value' in results: - assert results['value'] == testcase['value'] - if 'previous_value' in results: - assert results['previous_value'] == testcase['previous_value'] + + for test_result in ('channel', 'property'): + assert test_result in results, "'{0}' not found in {1}".format(test_result, results) + assert results[test_result] == results['invocation']['module_args'][test_result], \ + "'{0}': '{1}' != '{2}'".format(test_result, results[test_result], results['invocation']['module_args'][test_result]) + + 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: