From 211cf3983c79edc0e4305b743b41feddaa461cfa Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Fri, 3 May 2019 17:50:15 +0900 Subject: [PATCH] Require logger passed to broker in unit tests for account-reaper This patch removes default value for logger passed to faked BrokerClass in some unit tests for account reaper, to make sure that a logger instance is properly passed when generation BrokerClass instance. This patch also removes unnecessory parameter, which is added to confirm logger passed to broker, but is not useful in fact. NOTE: This patch is a follow-up of my previous commit[1] [1] https://review.opendev.org/#/c/295875/ Change-Id: Ica10e3ac42375c2dc141478e647408df90028ae9 --- test/unit/account/test_reaper.py | 56 ++++++++------------------------ test/unit/container/test_sync.py | 33 +++++++------------ 2 files changed, 26 insertions(+), 63 deletions(-) diff --git a/test/unit/account/test_reaper.py b/test/unit/account/test_reaper.py index fdd1165ae1..ebb5407f7f 100644 --- a/test/unit/account/test_reaper.py +++ b/test/unit/account/test_reaper.py @@ -20,7 +20,6 @@ import shutil import tempfile import unittest -from logging import DEBUG from mock import patch, call, DEFAULT import eventlet @@ -33,39 +32,6 @@ from test import unit from swift.common.storage_policy import StoragePolicy, POLICIES -class FakeLogger(object): - def __init__(self, *args, **kwargs): - self.inc = {'return_codes.4': 0, - 'return_codes.2': 0, - 'objects_failures': 0, - 'objects_deleted': 0, - 'objects_remaining': 0, - 'objects_possibly_remaining': 0, - 'containers_failures': 0, - 'containers_deleted': 0, - 'containers_remaining': 0, - 'containers_possibly_remaining': 0} - self.exp = [] - - def info(self, msg, *args): - self.msg = msg - - def error(self, msg, *args): - self.msg = msg - - def timing_since(*args, **kwargs): - pass - - def getEffectiveLevel(self): - return DEBUG - - def exception(self, *args): - self.exp.append(args) - - def increment(self, key): - self.inc[key] += 1 - - class FakeBroker(object): def __init__(self): self.info = {} @@ -75,7 +41,7 @@ class FakeBroker(object): class FakeAccountBroker(object): - def __init__(self, containers, logger=None): + def __init__(self, containers, logger): self.containers = containers self.containers_yielded = [] @@ -593,7 +559,7 @@ class TestReaper(unittest.TestCase): def test_reap_account(self): containers = ('c1', 'c2', 'c3', 'c4') - broker = FakeAccountBroker(containers) + broker = FakeAccountBroker(containers, unit.FakeLogger()) self.called_amount = 0 self.r = r = self.init_reaper({}, fakelogger=True) r.start_time = time.time() @@ -629,7 +595,7 @@ class TestReaper(unittest.TestCase): self.assertEqual(len(self.r.account_ring.devs), 3) def test_reap_account_no_container(self): - broker = FakeAccountBroker(tuple()) + broker = FakeAccountBroker(tuple(), unit.FakeLogger()) self.r = r = self.init_reaper({}, fakelogger=True) self.called_amount = 0 r.start_time = time.time() @@ -761,6 +727,7 @@ class TestReaper(unittest.TestCase): container_reaped[0] += 1 fake_ring = FakeRing() + fake_logger = unit.FakeLogger() with patch('swift.account.reaper.AccountBroker', FakeAccountBroker), \ patch( @@ -769,27 +736,32 @@ class TestReaper(unittest.TestCase): patch('swift.account.reaper.AccountReaper.reap_container', fake_reap_container): - fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) + fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'], + fake_logger) r.reap_account(fake_broker, 10, fake_ring.nodes, 0) self.assertEqual(container_reaped[0], 0) - fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) + fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'], + fake_logger) container_reaped[0] = 0 r.reap_account(fake_broker, 10, fake_ring.nodes, 1) self.assertEqual(container_reaped[0], 1) container_reaped[0] = 0 - fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) + fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'], + fake_logger) r.reap_account(fake_broker, 10, fake_ring.nodes, 2) self.assertEqual(container_reaped[0], 0) container_reaped[0] = 0 - fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) + fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'], + fake_logger) r.reap_account(fake_broker, 10, fake_ring.nodes, 3) self.assertEqual(container_reaped[0], 3) container_reaped[0] = 0 - fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) + fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'], + fake_logger) r.reap_account(fake_broker, 10, fake_ring.nodes, 4) self.assertEqual(container_reaped[0], 1) diff --git a/test/unit/container/test_sync.py b/test/unit/container/test_sync.py index 88fc304ff7..359e7ec0ae 100644 --- a/test/unit/container/test_sync.py +++ b/test/unit/container/test_sync.py @@ -48,7 +48,7 @@ class FakeRing(object): class FakeContainerBroker(object): def __init__(self, path, metadata=None, info=None, deleted=False, - items_since=None, logger=None): + items_since=None): self.db_file = path self.db_dir = os.path.dirname(path) self.metadata = metadata if metadata else {} @@ -191,7 +191,7 @@ class TestContainerSync(unittest.TestCase): mock.patch('swift.container.sync.ContainerBroker', lambda p, logger: FakeContainerBroker(p, info={ 'account': 'a', 'container': 'c', - 'storage_policy_index': 0}, logger=logger)): + 'storage_policy_index': 0})): fake_generator.side_effect = [iter(['container.db']), iter(['container.db'])] cs = sync.ContainerSync({}, container_ring=FakeRing()) @@ -237,7 +237,7 @@ class TestContainerSync(unittest.TestCase): mock.patch('swift.container.sync.ContainerBroker', lambda p, logger: FakeContainerBroker(p, info={ 'account': 'a', 'container': 'c', - 'storage_policy_index': 0}, logger=logger)): + 'storage_policy_index': 0})): fake_generator.side_effect = [iter(['container.db']), iter(['container.db'])] cs = sync.ContainerSync({}, container_ring=FakeRing()) @@ -339,8 +339,7 @@ class TestContainerSync(unittest.TestCase): try: sync.ContainerBroker = lambda p, logger: FakeContainerBroker( p, info={'account': 'a', 'container': 'c', - 'storage_policy_index': 0}, - logger=logger) + 'storage_policy_index': 0}) cs._myips = ['127.0.0.1'] # No match cs._myport = 1 # No match cs.container_sync('isa.db') @@ -373,8 +372,7 @@ class TestContainerSync(unittest.TestCase): try: sync.ContainerBroker = lambda p, logger: FakeContainerBroker( p, info={'account': 'a', 'container': 'c', - 'storage_policy_index': 0}, deleted=False, - logger=logger) + 'storage_policy_index': 0}, deleted=False) cs._myips = ['10.0.0.0'] # Match cs._myport = 1000 # Match # This complete match will cause the 1 container failure since the @@ -384,8 +382,7 @@ class TestContainerSync(unittest.TestCase): sync.ContainerBroker = lambda p, logger: FakeContainerBroker( p, info={'account': 'a', 'container': 'c', - 'storage_policy_index': 0}, deleted=True, - logger=logger) + 'storage_policy_index': 0}, deleted=True) # This complete match will not cause any more container failures # since the broker indicates deletion cs.container_sync('isa.db') @@ -403,8 +400,7 @@ class TestContainerSync(unittest.TestCase): p, info={'account': 'a', 'container': 'c', 'storage_policy_index': 0, 'x_container_sync_point1': -1, - 'x_container_sync_point2': -1}, - logger=logger) + 'x_container_sync_point2': -1}) cs._myips = ['10.0.0.0'] # Match cs._myport = 1000 # Match # This complete match will be skipped since the broker's metadata @@ -418,8 +414,7 @@ class TestContainerSync(unittest.TestCase): 'storage_policy_index': 0, 'x_container_sync_point1': -1, 'x_container_sync_point2': -1}, - metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1)}, - logger=logger) + metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1)}) cs._myips = ['10.0.0.0'] # Match cs._myport = 1000 # Match # This complete match will be skipped since the broker's metadata @@ -433,8 +428,7 @@ class TestContainerSync(unittest.TestCase): 'storage_policy_index': 0, 'x_container_sync_point1': -1, 'x_container_sync_point2': -1}, - metadata={'x-container-sync-key': ('key', 1)}, - logger=logger) + metadata={'x-container-sync-key': ('key', 1)}) cs._myips = ['10.0.0.0'] # Match cs._myport = 1000 # Match # This complete match will be skipped since the broker's metadata @@ -449,8 +443,7 @@ class TestContainerSync(unittest.TestCase): 'x_container_sync_point1': -1, 'x_container_sync_point2': -1}, metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1), - 'x-container-sync-key': ('key', 1)}, - logger=logger) + 'x-container-sync-key': ('key', 1)}) cs._myips = ['10.0.0.0'] # Match cs._myport = 1000 # Match cs.allowed_sync_hosts = [] @@ -466,8 +459,7 @@ class TestContainerSync(unittest.TestCase): 'x_container_sync_point1': -1, 'x_container_sync_point2': -1}, metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1), - 'x-container-sync-key': ('key', 1)}, - logger=logger) + 'x-container-sync-key': ('key', 1)}) cs._myips = ['10.0.0.0'] # Match cs._myport = 1000 # Match cs.allowed_sync_hosts = ['127.0.0.1'] @@ -493,8 +485,7 @@ class TestContainerSync(unittest.TestCase): 'x_container_sync_point2': -1}, metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1), 'x-container-sync-key': ('key', 1)}, - items_since=['erroneous data'], - logger=logger) + items_since=['erroneous data']) cs._myips = ['10.0.0.0'] # Match cs._myport = 1000 # Match cs.allowed_sync_hosts = ['127.0.0.1']