From a0b254efe5013ec2f60ec6f04a5ac8db304aba6d Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 7 Apr 2020 15:55:17 +0100 Subject: Cleanup warnings Zuul has taken to including warnings in the locations that they're raised in. We have a few of these in recent jobs so go ahead and clean them up. Change-Id: Ifcce20159d872ffd1447ca10f126ae2f2162f956 Signed-off-by: Stephen Finucane --- oslo_policy/generator.py | 17 +++++++++++------ oslo_policy/tests/test_generator.py | 6 ++++-- oslo_policy/tests/test_parser.py | 9 +++++++-- oslo_policy/tests/test_policy.py | 27 ++++++++++++++++++++++----- oslo_policy/tests/test_shell.py | 4 ++-- 5 files changed, 46 insertions(+), 17 deletions(-) diff --git a/oslo_policy/generator.py b/oslo_policy/generator.py index fd9f159..48a9972 100644 --- a/oslo_policy/generator.py +++ b/oslo_policy/generator.py @@ -277,6 +277,9 @@ def _generate_sample(namespaces, output_file=None, output_format='yaml', ',\n '.join(sections_text), '\n}\n')) + if output_file != sys.stdout: + output_file.close() + def _generate_policy(namespace, output_file=None): """Generate a policy file showing what will be used. @@ -304,6 +307,9 @@ def _generate_policy(namespace, output_file=None): for section in _sort_and_format_by_section(policies, include_help=False): output_file.write(section) + if output_file != sys.stdout: + output_file.close() + def _list_redundant(namespace): """Generate a list of configured policies which match defaults. @@ -396,12 +402,11 @@ def upgrade_policy(args=None, conf=None): _upgrade_policies(policies, default_policies) if conf.output_file: - if conf.format == 'yaml': - yaml.safe_dump(policies, open(conf.output_file, 'w'), - default_flow_style=False) - elif conf.format == 'json': - jsonutils.dump(policies, open(conf.output_file, 'w'), - indent=4) + with open(conf.output_file, 'w') as fh: + if conf.format == 'yaml': + yaml.safe_dump(policies, fh, default_flow_style=False) + elif conf.format == 'json': + jsonutils.dump(policies, fh, indent=4) else: if conf.format == 'yaml': sys.stdout.write(yaml.safe_dump(policies, diff --git a/oslo_policy/tests/test_generator.py b/oslo_policy/tests/test_generator.py index 33d420e..86003fe 100644 --- a/oslo_policy/tests/test_generator.py +++ b/oslo_policy/tests/test_generator.py @@ -674,7 +674,8 @@ class UpgradePolicyTestCase(base.PolicyBaseTestCase): with mock.patch('sys.argv', testargs): generator.upgrade_policy(conf=self.local_conf) new_file = self.get_config_file_fullname('new_policy.json') - new_policy = jsonutils.loads(open(new_file, 'r').read()) + with open(new_file, 'r') as fh: + new_policy = jsonutils.loads(fh.read()) self.assertIsNotNone(new_policy.get('new_policy_name')) self.assertIsNone(new_policy.get('deprecated_name')) @@ -693,7 +694,8 @@ class UpgradePolicyTestCase(base.PolicyBaseTestCase): with mock.patch('sys.argv', testargs): generator.upgrade_policy(conf=self.local_conf) new_file = self.get_config_file_fullname('new_policy.yaml') - new_policy = yaml.safe_load(open(new_file, 'r')) + with open(new_file, 'r') as fh: + new_policy = yaml.safe_load(fh) self.assertIsNotNone(new_policy.get('new_policy_name')) self.assertIsNone(new_policy.get('deprecated_name')) diff --git a/oslo_policy/tests/test_parser.py b/oslo_policy/tests/test_parser.py index edfd475..f1ac36b 100644 --- a/oslo_policy/tests/test_parser.py +++ b/oslo_policy/tests/test_parser.py @@ -34,16 +34,20 @@ class ParseCheckTestCase(test_base.BaseTestCase): self.assertIsInstance(result, _checks.TrueCheck) - def test_bad_rule(self): + @mock.patch.object(_parser, 'LOG') + def test_bad_rule(self, mock_log): result = _parser._parse_check('foobar') self.assertIsInstance(result, _checks.FalseCheck) + mock_log.exception.assert_called_once() @mock.patch.object(_checks, 'registered_checks', {}) - def test_no_handler(self): + @mock.patch.object(_parser, 'LOG') + def test_no_handler(self, mock_log): result = _parser._parse_check('no:handler') self.assertIsInstance(result, _checks.FalseCheck) + mock_log.error.assert_called() @mock.patch.object(_checks, 'registered_checks', { 'spam': mock.Mock(return_value='spam_check'), @@ -375,6 +379,7 @@ class ParseTextRuleTestCase(test_base.BaseTestCase): mock_shift.assert_has_calls( [mock.call('tok1', 'val1'), mock.call('tok2', 'val2')]) + @mock.patch.object(_parser, 'LOG', new=mock.Mock()) @mock.patch.object(_parser, '_parse_tokenize', return_value=[]) def test_fail(self, mock_parse_tokenize): result = _parser._parse_text_rule('test rule') diff --git a/oslo_policy/tests/test_policy.py b/oslo_policy/tests/test_policy.py index f3f75b0..0b0164e 100644 --- a/oslo_policy/tests/test_policy.py +++ b/oslo_policy/tests/test_policy.py @@ -797,6 +797,7 @@ class EnforcerTest(base.PolicyBaseTestCase): for k, v in expected_creds.items(): self.assertEqual(expected_creds[k], creds[k]) + @mock.patch('warnings.warn', new=mock.Mock()) def test_map_context_attributes_populated_system(self): request_context = context.RequestContext(system_scope='all') expected_creds = request_context.to_policy_values() @@ -1161,6 +1162,7 @@ class RuleDefaultTestCase(base.PolicyBaseTestCase): class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase): + @mock.patch('warnings.warn', new=mock.Mock()) def test_deprecate_a_policy_check_string(self): deprecated_rule = policy.DeprecatedRule( name='foo:create_bar', @@ -1200,6 +1202,7 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase): enforcer.enforce('foo:create_bar', {}, {'roles': ['baz']}) ) + @mock.patch('warnings.warn', new=mock.Mock()) def test_deprecate_an_empty_policy_check_string(self): deprecated_rule = policy.DeprecatedRule( name='foo:create_bar', @@ -1227,6 +1230,7 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase): enforcer.enforce('foo:create_bar', {}, {'roles': ['fizz']}, do_raise=True) + @mock.patch('warnings.warn', new=mock.Mock()) def test_deprecate_replace_with_empty_policy_check_string(self): deprecated_rule = policy.DeprecatedRule( name='foo:create_bar', @@ -1468,6 +1472,7 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase): deprecated_since='N' ) + @mock.patch('warnings.warn', new=mock.Mock()) def test_override_deprecated_policy_with_old_name(self): # Simulate an operator overriding a policy rules = jsonutils.dumps({'foo:bar': 'role:bazz'}) @@ -1536,6 +1541,7 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase): self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bazz']}) ) + @mock.patch('warnings.warn', new=mock.Mock()) def test_override_both_new_and_old_policy(self): # Simulate an operator overriding a policy using both the the new and # old policy names. The following doesn't make a whole lot of sense @@ -1586,6 +1592,7 @@ class DocumentedRuleDefaultDeprecationTestCase(base.PolicyBaseTestCase): self.enforcer.enforce('foo:create_bar', {}, {'roles': ['bazz']}) ) + @mock.patch('warnings.warn', new=mock.Mock()) def test_override_deprecated_policy_with_new_rule(self): # Simulate an operator overriding a deprecated policy with a reference # to the new policy, as done by the sample policy generator. @@ -1712,37 +1719,46 @@ class EnforcerCheckRulesTest(base.PolicyBaseTestCase): self.enforcer.load_rules(True) self.assertTrue(self.enforcer.check_rules(raise_on_violation=True)) - def test_undefined_rule(self): + @mock.patch.object(policy, 'LOG') + def test_undefined_rule(self, mock_log): rules = jsonutils.dumps({'foo': 'rule:bar'}) self.create_config_file('policy.json', rules) self.enforcer.load_rules(True) self.assertFalse(self.enforcer.check_rules()) + mock_log.warning.assert_called() - def test_undefined_rule_raises(self): + @mock.patch.object(policy, 'LOG') + def test_undefined_rule_raises(self, mock_log): rules = jsonutils.dumps({'foo': 'rule:bar'}) self.create_config_file('policy.json', rules) self.enforcer.load_rules(True) self.assertRaises(policy.InvalidDefinitionError, self.enforcer.check_rules, raise_on_violation=True) + mock_log.warning.assert_called() - def test_cyclical_rules(self): + @mock.patch.object(policy, 'LOG') + def test_cyclical_rules(self, mock_log): rules = jsonutils.dumps({'foo': 'rule:bar', 'bar': 'rule:foo'}) self.create_config_file('policy.json', rules) self.enforcer.load_rules(True) self.assertFalse(self.enforcer.check_rules()) + mock_log.warning.assert_called() - def test_cyclical_rules_raises(self): + @mock.patch.object(policy, 'LOG') + def test_cyclical_rules_raises(self, mock_log): rules = jsonutils.dumps({'foo': 'rule:bar', 'bar': 'rule:foo'}) self.create_config_file('policy.json', rules) self.enforcer.load_rules(True) self.assertRaises(policy.InvalidDefinitionError, self.enforcer.check_rules, raise_on_violation=True) + mock_log.warning.assert_called() - def test_complex_cyclical_rules_false(self): + @mock.patch.object(policy, 'LOG') + def test_complex_cyclical_rules_false(self, mock_log): rules = jsonutils.dumps({'foo': 'rule:bar', 'bar': 'rule:baz and role:admin', 'baz': 'rule:foo or role:user'}) @@ -1750,6 +1766,7 @@ class EnforcerCheckRulesTest(base.PolicyBaseTestCase): self.enforcer.load_rules(True) self.assertFalse(self.enforcer.check_rules()) + mock_log.warning.assert_called() def test_complex_cyclical_rules_true(self): rules = jsonutils.dumps({'foo': 'rule:bar or rule:baz', diff --git a/oslo_policy/tests/test_shell.py b/oslo_policy/tests/test_shell.py index 0eb6aa6..b27347c 100644 --- a/oslo_policy/tests/test_shell.py +++ b/oslo_policy/tests/test_shell.py @@ -229,7 +229,7 @@ passed: sampleservice:sample_rule2 self.create_config_file( "target.json", jsonutils.dumps(target)) - target_file = open(self.get_config_file_fullname('target.json'), 'r') - target_from_file = target_file.read() + with open(self.get_config_file_fullname('target.json'), 'r') as fh: + target_from_file = fh.read() result = shell.flatten(jsonutils.loads(target_from_file)) self.assertEqual(result, {"target.secret.project_id": "1234"}) -- cgit v1.2.1