From 5a007dce33861873b7b30452edf83c40f26ede2b Mon Sep 17 00:00:00 2001
From: Eddy Mwiti <eddmwiti@amazon.com>
Date: Thu, 7 Jul 2022 12:30:27 +0100
Subject: [PATCH] [SlurmPlugin] Log exceptions raised when reading json

While reading json files such as `run_instances_overrides.json`, the read operation may fail due to use of non-standard characters. This is difficult to debug since at the moment we use a default value when there is an error and furthermore the error is not logged.
This commit logs the exception in situations where we fallback to a default value.
---
 src/slurm_plugin/common.py                       |  2 +-
 tests/slurm_plugin/test_common.py                | 16 +++++++++++++++-
 .../test_common/test_read_json/faulty.json       |  5 +++++
 .../test_common/test_read_json/standard.json     |  5 +++++
 4 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100644 tests/slurm_plugin/test_common/test_read_json/faulty.json
 create mode 100644 tests/slurm_plugin/test_common/test_read_json/standard.json

diff --git a/src/slurm_plugin/common.py b/src/slurm_plugin/common.py
index 4e70a5d98..3afb0e294 100644
--- a/src/slurm_plugin/common.py
+++ b/src/slurm_plugin/common.py
@@ -73,7 +73,7 @@ def read_json(file_path, default=None):
             )
             raise
         else:
-            logger.info("Unable to read file '%s'. Using default: %s", file_path, default)
+            logger.info("Unable to read file '%s' due to an exception: %s. Using default: %s", file_path, e, default)
             return default
 
 
diff --git a/tests/slurm_plugin/test_common.py b/tests/slurm_plugin/test_common.py
index 2774dc199..67e02fc86 100644
--- a/tests/slurm_plugin/test_common.py
+++ b/tests/slurm_plugin/test_common.py
@@ -15,7 +15,7 @@
 import pytest
 from assertpy import assert_that
 from common.utils import time_is_up
-from slurm_plugin.common import TIMESTAMP_FORMAT, get_clustermgtd_heartbeat
+from slurm_plugin.common import TIMESTAMP_FORMAT, get_clustermgtd_heartbeat, read_json
 
 
 @pytest.mark.parametrize(
@@ -76,3 +76,17 @@ def test_get_clustermgtd_heartbeat(time, expected_parsed_time, mocker):
         return_value=f"some_random_stdout\n{time.strftime(TIMESTAMP_FORMAT)}",
     )
     assert_that(get_clustermgtd_heartbeat("some file path")).is_equal_to(expected_parsed_time)
+
+
+@pytest.mark.parametrize(
+    "json_file, log_assertion",
+    [
+        ("test_read_json/faulty.json", lambda logs: "Failed with exception:" in logs),
+        ("test_read_json/standard.json", lambda logs: "Failed with exception:" not in logs),
+    ],
+)
+def test_read_json(test_datadir, caplog, json_file, log_assertion):
+    json_file_path = str(test_datadir.joinpath(json_file))
+    with pytest.raises(Exception):
+        read_json(json_file_path)
+    assert log_assertion
diff --git a/tests/slurm_plugin/test_common/test_read_json/faulty.json b/tests/slurm_plugin/test_common/test_read_json/faulty.json
new file mode 100644
index 000000000..b96f02d9f
--- /dev/null
+++ b/tests/slurm_plugin/test_common/test_read_json/faulty.json
@@ -0,0 +1,5 @@
+{
+    "test_property_1": {
+                    "test_property_2": "test_value"
+                }
+}
\ No newline at end of file
diff --git a/tests/slurm_plugin/test_common/test_read_json/standard.json b/tests/slurm_plugin/test_common/test_read_json/standard.json
new file mode 100644
index 000000000..f3eb8fa6e
--- /dev/null
+++ b/tests/slurm_plugin/test_common/test_read_json/standard.json
@@ -0,0 +1,5 @@
+{
+    "test_property_1": {
+      "test_property_2": "test_value"
+    }
+}
\ No newline at end of file