slurp - improve error code and test coverage (#75038)

* Improve the error handling code

Rather than multiple return paths, have a single return and set the message based
on the type of failure.

* Add another test for non-specific failures

* Reorganize tests so failure tests are in one tasks file

* Remove os.stat() call and add changelog

changelogs/fragments/slurp-improve-error-handling-readability.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - slurp - improve the logic in the error handling and remove ``os.stat()`` call (https://github.com/ansible/ansible/pull/75038)

lib/ansible/modules/slurp.py

@@ -94,18 +94,19 @@ def main():
     source = module.params['src']
 
     try:
-        os.stat(source)
         with open(source, 'rb') as source_fh:
             source_content = source_fh.read()
     except (IOError, OSError) as e:
         if e.errno == errno.ENOENT:
-            module.fail_json(msg="file not found: %s" % source)
+            msg = "file not found: %s" % source
         elif e.errno == errno.EACCES:
-            module.fail_json(msg="file is not readable: %s" % source)
+            msg = "file is not readable: %s" % source
         elif e.errno == errno.EISDIR:
-            module.fail_json(msg="source is a directory and must be a file: %s" % source)
+            msg = "source is a directory and must be a file: %s" % source
         else:
-            module.fail_json(msg="unable to slurp file: %s" % to_native(e, errors='surrogate_then_replace'))
+            msg = "unable to slurp file: %s" % to_native(e, errors='surrogate_then_replace')
+
+        module.fail_json(msg)
 
     data = base64.b64encode(source_content)
 

test/integration/targets/slurp/tasks/main.yml

@@ -54,37 +54,6 @@
       - "slurp_binary is not changed"
       - "slurp_binary is not failed"
 
-- name: test slurping a non-existent file
-  slurp:
-    src: '{{ output_dir }}/i_do_not_exist'
-  register: slurp_missing
-  ignore_errors: true
-
-- name: check slurp missing result
-  assert:
-    that:
-      - "slurp_missing is failed"
-      - "slurp_missing.msg"
-      - "slurp_missing is not changed"
-
-- name: Create a directory to test with
-  file:
-    path: '{{ output_dir }}/baz/'
-    state: directory
-
-- name: test slurping a directory
-  slurp:
-    src: '{{ output_dir }}/baz'
-  register: slurp_dir
-  ignore_errors: true
-
-- name: check slurp directory result
-  assert:
-    that:
-      - slurp_dir is failed
-      - slurp_dir.msg is search('source is a directory and must be a file')
-      - slurp_dir is not changed
-
 - name: test slurp with missing argument
   action: slurp
   register: slurp_no_args
@@ -97,6 +66,4 @@
       - "slurp_no_args.msg"
       - "slurp_no_args is not changed"
 
-# Ensure unreadable file and directory handling and error messages
-# https://github.com/ansible/ansible/issues/67340
-- import_tasks: 'test_unreadable.yml'
+- import_tasks: test_unreadable.yml

test/integration/targets/slurp/tasks/test_unreadable.yml

@@ -1,3 +1,22 @@
+- name: test slurping a non-existent file
+  slurp:
+    src: '{{ output_dir }}/i_do_not_exist'
+  register: slurp_missing
+  ignore_errors: yes
+
+- name: Create a directory to test with
+  file:
+    path: '{{ output_dir }}/baz/'
+    state: directory
+
+- name: test slurping a directory
+  slurp:
+    src: '{{ output_dir }}/baz'
+  register: slurp_dir
+  ignore_errors: yes
+
+# Ensure unreadable file and directory handling and error messages
+# https://github.com/ansible/ansible/issues/67340
 - name: create test user
   user:
     name: "{{ become_test_user }}"
@@ -16,37 +35,47 @@
   slurp:
     src: "{{ output_dir }}/qux.txt"
   register: slurp_unreadable_file
-  become: true
+  become: yes
   become_user: "{{ become_test_user }}"
   become_method: su
-  ignore_errors: true
-
-- name: check slurp unreadable file result
-  assert:
-    that:
-      - "slurp_unreadable_file is failed"
-      - "slurp_unreadable_file.msg is regex('^file is not readable:')"
-      - "slurp_unreadable_file is not changed"
+  ignore_errors: yes
 
 - name: create unreadable directory
   file:
     path: "{{ output_dir }}/test_data"
     state: directory
-    mode: 0700
+    mode: '0700'
     owner: root
 
 - name: test slurp unreadable directory
   slurp:
     src: "{{ output_dir }}/test_data"
   register: slurp_unreadable_dir
-  become: true
+  become: yes
   become_user: "{{ become_test_user }}"
   become_method: su
-  ignore_errors: true
+  ignore_errors: yes
 
-- name: check slurp unreadable directory result
+- name: Try to access file as directory
+  slurp:
+    src: "{{ output_dir }}/qux.txt/somefile"
+  ignore_errors: yes
+  register: slurp_path_file_as_dir
+
+- name: check slurp failures
   assert:
     that:
-      - "slurp_unreadable_dir is failed"
-      - "slurp_unreadable_dir.msg is regex('^file is not readable:')"
-      - "slurp_unreadable_dir is not changed"
+      - slurp_missing is failed
+      - slurp_missing.msg is search('file not found')
+      - slurp_missing is not changed
+      - slurp_unreadable_file is failed
+      - slurp_unreadable_file.msg is regex('^file is not readable:')
+      - slurp_unreadable_file is not changed
+      - slurp_unreadable_dir is failed
+      - slurp_unreadable_dir.msg is regex('^file is not readable:')
+      - slurp_unreadable_dir is not changed
+      - slurp_path_file_as_dir is failed
+      - slurp_path_file_as_dir is search('unable to slurp file')
+      - slurp_dir is failed
+      - slurp_dir.msg is search('source is a directory and must be a file')
+      - slurp_dir is not changed