From 397a9c640c872e4cc2079b147dcded65d3ed68f5 Mon Sep 17 00:00:00 2001
From: samueltwum1 <samueltwum1@gmail.com>
Date: Thu, 17 Sep 2020 18:08:03 +0200
Subject: [PATCH] SAR-149 Resolve some review comments
- Pass parameters as a keyword argument to transaction
- Fix test to reflect change to transaction module
---
src/ska/logging/transactions.py | 22 ++++++++------------
tests/test_transactions.py | 36 ++++++++++++++-------------------
2 files changed, 23 insertions(+), 35 deletions(-)
diff --git a/src/ska/logging/transactions.py b/src/ska/logging/transactions.py
index 670e53e..5a1bbd4 100644
--- a/src/ska/logging/transactions.py
+++ b/src/ska/logging/transactions.py
@@ -5,7 +5,7 @@ import json
import logging
import os
-from typing import Mapping, Text
+from typing import Text
from ska.skuid.client import SkuidClient
@@ -31,15 +31,13 @@ class Transaction:
"""
- def __init__(self, name, params, logger=None):
- if not isinstance(params, Mapping):
- raise TransactionParamsError("params must be dict-like (Mapping)")
- self.logger = logger
- if not logger:
- self.logger = logging.getLogger(__name__)
+ def __init__(self, name, **params):
+ # TODO: get the logger from the transaction logginh application without passing to
+ # context handler see https://gitlab.com/ska-telescope/ska-logging/-/merge_requests/7#note_414359589
+ self.logger = logging.getLogger(__name__)
self._name = name
self._params = params
- self.transaction_id = self._get_existing_transaction_id() or self._generate_new_id()
+ self.transaction_id = self._get_new_or_existing_transaction_id()
def __enter__(self):
params_json = json.dumps(self._params)
@@ -56,10 +54,10 @@ class Transaction:
if exc_type:
raise
- def _get_existing_transaction_id(self):
+ def _get_new_or_existing_transaction_id(self):
transaction_id = self._params.get("transaction_id")
if not self._is_valid_id(transaction_id):
- transaction_id = None
+ transaction_id = self._generate_new_id()
return transaction_id
def _is_valid_id(self, transaction_id):
@@ -87,7 +85,3 @@ class TransactionIdGenerator:
def next(self):
return self._get_id()
-
-
-class TransactionParamsError(TypeError):
- """Invalid data type for transaction parameters."""
diff --git a/tests/test_transactions.py b/tests/test_transactions.py
index 825713b..e29b5c6 100644
--- a/tests/test_transactions.py
+++ b/tests/test_transactions.py
@@ -10,7 +10,7 @@ import concurrent.futures
from unittest.mock import patch, MagicMock
from ska.logging import transaction
-from ska.logging.transactions import TransactionParamsError, TransactionIdGenerator
+from ska.logging.transactions import TransactionIdGenerator
from tests.conftest import (
get_first_record_and_log_message,
get_last_record_and_log_message,
@@ -23,62 +23,56 @@ class TestTransactionIdGeneration:
def test_existing_valid_id_is_reused(self, id_generator_stub):
parameters = {"transaction_id": "abc1234", "other": "config"}
- with transaction("name", parameters) as transaction_id:
+ with transaction("name", **parameters) as transaction_id:
assert transaction_id == "abc1234"
def test_new_id_generated_if_id_is_empty(self, id_generator_stub):
parameters = {"transaction_id": "", "other": "config"}
- with transaction("name", parameters) as transaction_id:
+ with transaction("name", **parameters) as transaction_id:
assert transaction_id == id_generator_stub.last_id
def test_new_id_generated_if_id_is_none(self, id_generator_stub):
parameters = {"transaction_id": None, "other": "config"}
- with transaction("name", parameters) as transaction_id:
+ with transaction("name", **parameters) as transaction_id:
assert transaction_id == id_generator_stub.last_id
def test_new_id_generated_if_id_is_only_white_space(self, id_generator_stub):
parameters = {"transaction_id": "\t\n \r\n", "other": "config"}
- with transaction("name", parameters) as transaction_id:
+ with transaction("name", **parameters) as transaction_id:
assert transaction_id == id_generator_stub.last_id
def test_new_id_generated_if_id_is_not_string_type(self, id_generator_stub):
parameters = {"transaction_id": 1234.5, "other": "config"}
- with transaction("name", parameters) as transaction_id:
+ with transaction("name", **parameters) as transaction_id:
assert transaction_id == id_generator_stub.last_id
def test_new_id_generated_if_id_is_not_present(self, id_generator_stub):
parameters = {"other": "config"}
- with transaction("name", parameters) as transaction_id:
+ with transaction("name", **parameters) as transaction_id:
assert transaction_id == id_generator_stub.last_id
def test_new_id_generated_if_params_empty(self, id_generator_stub):
parameters = {}
- with transaction("name", parameters) as transaction_id:
+ with transaction("name", **parameters) as transaction_id:
assert transaction_id == id_generator_stub.last_id
def test_id_provider_only_used_once_for_one_new_id(self, id_generator_stub):
parameters = {}
- with transaction("name", parameters):
+ with transaction("name", **parameters):
assert id_generator_stub.call_count == 1
def test_id_provider_not_used_for_existing_valid_id(self, id_generator_stub):
parameters = {"transaction_id": "abc1234"}
- with transaction("name", parameters):
+ with transaction("name", **parameters):
assert id_generator_stub.call_count == 0
- def test_error_if_params_type_is_not_mapping(self):
- parameters = []
- with pytest.raises(TransactionParamsError):
- with transaction("name", parameters):
- pass
-
class TestTransactionLogging:
"""Tests for :class:`~ska.logging.transaction` related to logging."""
def test_name_and_id_and_params_in_context_handler(self, id_generator_stub, recording_logger):
- params = {"other": ["config", 1, 2, 3.0]}
- with transaction("name", params) as transaction_id:
+ parameters = {"other": ["config", 1, 2, 3.0]}
+ with transaction("name", **parameters) as transaction_id:
pass
_, first_log_message = get_first_record_and_log_message(recording_logger)
_, last_log_message = get_last_record_and_log_message(recording_logger)
@@ -87,7 +81,7 @@ class TestTransactionLogging:
assert "Start transaction" in first_log_message
assert "name" in first_log_message
assert transaction_id in first_log_message
- assert json.dumps(params) in first_log_message
+ assert json.dumps(parameters) in first_log_message
# __exit__ log message
assert "Exit transaction" in last_log_message
@@ -95,9 +89,9 @@ class TestTransactionLogging:
assert transaction_id in last_log_message
def test_exception_logs_transaction_id_and_command(self, id_generator_stub, recording_logger):
- params = {"other": ["config", 1, 2, 3.0]}
+ parameters = {"other": ["config", 1, 2, 3.0]}
with pytest.raises(RuntimeError):
- with transaction("name", params) as transaction_id:
+ with transaction("name", **parameters) as transaction_id:
raise RuntimeError("Something went wrong")
record_logs = get_all_record_logs(recording_logger)
--
GitLab