Differential D50

don't try to create dirs under the test suite
ClosedPublic
Actions

Authored by kparal on Apr 11 2014, 8:21 AM.

Details

Reviewers
mkrizek
jskladan
Commits
rLTRN28370f12ba01: don't try to create dirs under the test suite
Summary

And also:

  • config: remove cachedir and lockdir, they are not needed ATM
  • config: simplify an error message

This is after discussion in D35#8 with @jskladan and @tflink.

Test Plan

You will no longer see warnings about missing directories when running the test suite, even if you don't have them.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
mkrizek added a comment.Apr 14 2014, 9:02 AM

Is "don't try to create dirs under the test suite" useful for something else than just so unittests don't touch the disk? If not, wouldn't it be better to just monkeypatch config values in unittests and don't create additional logic (not) to touch the disk?

kparal added a comment.Apr 14 2014, 9:15 AM

Is "don't try to create dirs under the test suite" useful for something else than just so unittests don't touch the disk? If not, wouldn't it be better to just monkeypatch config values in unittests and don't create additional logic (not) to touch the disk?

Yes, we could do that. But we would have to monkeypatch it in every single unit test (a new fixture for every test_* file). This way it's transparent for all tests and you don't need to do anything about it. I mean, that's what TestingConfig is for, isn't it? Also, direct support in TestingConfig seems cleaner to me than monkeypatching (it's basically just two returns). But maybe I'm missing some even simpler solution?

jskladan accepted this revision.Apr 15 2014, 1:51 PM

Makes sense to me, and the code looks fine.

mkrizek accepted this revision.Apr 15 2014, 1:53 PM

ok, makes sense

kparal closed this revision.Apr 15 2014, 2:06 PM

Closed by commit rLTRN28370f12ba01 (authored by @kparal).

Revision Contents

Diff 146

View Options

libtaskotron/config.py

Show First 20 Lines • Show All 70 Lines • ▼ Show 20 Line(s)69def _load():
71 71​
72 @return Config instance that was updated by external config file contents 72​ @return Config instance that was updated by external config file contents
73 @throw TaskotronConfigError if file config parsing and handling failed 73​ @throw TaskotronConfigError if file config parsing and handling failed
74 ''' 74​ '''
75 75​
76 # first load the defaults and make sure we even want to load config files 76​ # first load the defaults and make sure we even want to load config files
77 env_profile = os.getenv(PROFILE_VAR) 77​ env_profile = os.getenv(PROFILE_VAR)
78 config = _load_defaults(env_profile) 78​ config = _load_defaults(env_profile)
79 if not config.load_conf_files: 79​ if not config.touch_disk:
80 log.debug('Not loading config files from disk as requested') 80​ log.debug('Testing profile, not loading config files from disk')
81 return config 81​ return config
82 82​
83 # load config files 83​ # load config files
84 filename = _search_dirs(CONF_DIRS, CONF_FILE) 84​ filename = _search_dirs(CONF_DIRS, CONF_FILE)
85 if not filename: 85​ if not filename:
86 log.warn('No config file %s found in dirs: %s' % (CONF_FILE, CONF_DIRS)) 86​ log.warn('No config file %s found in dirs: %s' % (CONF_FILE, CONF_DIRS))
87 return config 87​ return config
88 88​
▲ Show 20 Lines • Show All 132 Lines • ▼ Show 20 Line(s)
221 221​
222def _create_dirs(config): 222​def _create_dirs(config):
223 '''Create directories in the local file system for appropriate config 223​ '''Create directories in the local file system for appropriate config
224 options - e.g. tmpdir, logdir, etc. 224​ options - e.g. tmpdir, logdir, etc.
225 225​
226 @param config Config instance 226​ @param config Config instance
227 ''' 227​ '''
228 228​
229​ if not config.touch_disk:
230​ log.debug('Testing profile, not trying to create necessary dirs')
231​ return
232​
229 for dir_ in (config.tmpdir, config.logdir): 233​ for dir_ in (config.tmpdir, config.logdir):
230 try: 234​ try:
231 file_utils.makedirs(dir_) 235​ file_utils.makedirs(dir_)
232 except OSError as e: 236​ except OSError as e:
233 # log the problem, but don't raise it. Mostly we can work even 237​ # log the problem, but don't raise it. Mostly we can work even
234 # without these directiories, probably. If not, they will receive 238​ # without these directiories, probably. If not, they will receive
235 # a crash later. (We can re-evaluate this decision then.) 239​ # a crash later. (We can re-evaluate this decision then.)
236 log.warn('Failed to create a required directory, please create it ' 240​ log.warn('Failed to create a required directory, please create it '
237 'manually: %s' % e) 241​ 'manually: %s' % e)
238 242​
View Options

libtaskotron/config_defaults.py

Show First 20 Lines • Show All 44 Lines • ▼ Show 20 Line(s)29 def __init__(self):
45 45​
46 self.tmpdir = '/var/tmp/taskotron' 46​ self.tmpdir = '/var/tmp/taskotron'
47 self.logdir = '/var/log/taskotron' 47​ self.logdir = '/var/log/taskotron'
48 self.logfile = os.path.join(self.logdir, 'taskotron.log') 48​ self.logfile = os.path.join(self.logdir, 'taskotron.log')
49 49​
50 self.fas_username = 'taskotron' 50​ self.fas_username = 'taskotron'
51 self.fas_password = '' 51​ self.fas_password = ''
52 52​
53 # Load config files from the disk? If this is False, then only internal 53​ # Perform operations accessing disk drive (loading config files,
54 # default values (this file) are used. 54​ # creating directories)? If this is False, then the disk is not accessed
55 self.load_conf_files = True 55​ # and only internal default values are used.
56​ self.touch_disk = True
56 57​
57 58​
58 def __str__(self): 59​ def __str__(self):
59 # Make this object more readable when printing 60​ # Make this object more readable when printing
60 return '<%s: %s>' % (self.__class__.__name__, 61​ return '<%s: %s>' % (self.__class__.__name__,
61 pprint.pformat(vars(self))) 62​ pprint.pformat(vars(self)))
62 63​
63 64​
Show All 14 Lines78class TestingConfig(Config):
78 '''Configuration for testing suite profile. Inherits values from @class 79​ '''Configuration for testing suite profile. Inherits values from @class
79 Config and overrides some. Read @class Config documentation.''' 80​ Config and overrides some. Read @class Config documentation.'''
80 81​
81 def __init__(self): 82​ def __init__(self):
82 '''Create new TestingConfig.''' 83​ '''Create new TestingConfig.'''
83 super(TestingConfig, self).__init__() 84​ super(TestingConfig, self).__init__()
84 85​
85 self.profile = ProfileName.TESTING 86​ self.profile = ProfileName.TESTING
86 # we don't want to load conf files, it slows down the test suite 87​ # we don't want to access the disk, it slows down the test suite
87 self.load_conf_files = False 88​ self.touch_disk = False
88 89​
89 90​
90class ProfileName(object): 91​class ProfileName(object):
91 '''Enum of available profile names. These can be specified in the config 92​ '''Enum of available profile names. These can be specified in the config
92 file or as the environment variable.''' 93​ file or as the environment variable.'''
93 94​
94 DEVELOPMENT = 'development' 95​ DEVELOPMENT = 'development'
95 PRODUCTION = 'production' 96​ PRODUCTION = 'production'
96 TESTING = 'testing' 97​ TESTING = 'testing'
View Options

testing/test_config.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2014, Red Hat, Inc. 2​# Copyright 2014, Red Hat, Inc.
3# License: GNU General Public License version 2 or later 3​# License: GNU General Public License version 2 or later
4 4​
5'''Unit tests for libtaskotron/config.py''' 5​'''Unit tests for libtaskotron/config.py'''
6 6​
7import os 7​import os
8import pytest 8​import pytest
9from StringIO import StringIO 9​from StringIO import StringIO
10 10​
11from libtaskotron import config 11​from libtaskotron import config
12from libtaskotron import exceptions as exc 12​from libtaskotron import exceptions as exc
13​from libtaskotron import file_utils
13 14​
14 15​
15class TestConfig(object): 16​class TestConfig(object):
16 17​
17 def teardown_method(self, method): 18​ def teardown_method(self, method):
18 '''Run this after every method execution ends''' 19​ '''Run this after every method execution ends'''
19 # reset the singleton instance back 20​ # reset the singleton instance back
20 config._config = None 21​ config._config = None
Show All 14 Lines
35 36​
36 def test_testing_profile(self): 37​ def test_testing_profile(self):
37 '''By default we should have a testing profile''' 38​ '''By default we should have a testing profile'''
38 assert os.getenv(config.PROFILE_VAR) == config.ProfileName.TESTING 39​ assert os.getenv(config.PROFILE_VAR) == config.ProfileName.TESTING
39 conf = config.get_config() 40​ conf = config.get_config()
40 print conf 41​ print conf
41 assert conf.profile == config.ProfileName.TESTING 42​ assert conf.profile == config.ProfileName.TESTING
42 assert isinstance(conf, config.TestingConfig) 43​ assert isinstance(conf, config.TestingConfig)
43 assert not conf.load_conf_files 44​ assert not conf.touch_disk
44 45​
45 def test_singleton_instance(self): 46​ def test_singleton_instance(self):
46 '''Test whether we really have a singleton Config instance''' 47​ '''Test whether we really have a singleton Config instance'''
47 conf = config.get_config() 48​ conf = config.get_config()
48 assert isinstance(conf, config.Config) 49​ assert isinstance(conf, config.Config)
49 assert conf is config._config 50​ assert conf is config._config
50 conf2 = config.get_config() 51​ conf2 = config.get_config()
51 assert conf2 is conf 52​ assert conf2 is conf
▲ Show 20 Lines • Show All 67 Lines • ▼ Show 20 Line(s)114 def test_merge_config(self):
119 assert hasattr(conf, 'profile') 120​ assert hasattr(conf, 'profile')
120 old_profile = conf.profile 121​ old_profile = conf.profile
121 file_config = {'tmpdir': '/a/road/to/nowhere', 122​ file_config = {'tmpdir': '/a/road/to/nowhere',
122 'profile': 'invalid'} 123​ 'profile': 'invalid'}
123 config._merge_config(conf, file_config) 124​ config._merge_config(conf, file_config)
124 assert conf.tmpdir == '/a/road/to/nowhere' 125​ assert conf.tmpdir == '/a/road/to/nowhere'
125 assert conf.profile == old_profile 126​ assert conf.profile == old_profile
126 127​
127 def test_no_load_conf_files(self, monkeypatch): 128​ def test_no_touch_disk(self, monkeypatch):
128 '''Make sure that _load() function works and doesn't load from disk''' 129​ '''We shouldn't touch disk in the testing profile'''
129 # we will do that by checking that _search_dirs() or _load_file does 130​ # we will do that by checking that appropriate disk-touching methods
130 # not get called 131​ # don't get called
131 def _search_dirs_raise(x, y): 132​ def _search_dirs_raise(x, y):
132 assert False, 'This must not be called' 133​ assert False, 'This must not be called'
133 def _load_file_raise(x): 134​ def _load_file_raise(x):
134 assert False, 'This must not be called' 135​ assert False, 'This must not be called'
136​ def _makedirs_raise(x):
137​ assert False, 'This must not be called'
135 138​
136 monkeypatch.setattr(config, '_search_dirs', _search_dirs_raise) 139​ monkeypatch.setattr(config, '_search_dirs', _search_dirs_raise)
137 monkeypatch.setattr(config, '_load_file', _load_file_raise) 140​ monkeypatch.setattr(config, '_load_file', _load_file_raise)
141​ monkeypatch.setattr(file_utils, 'makedirs', _makedirs_raise)
138 142​
139 conf = config._load() 143​ conf = config.get_config()
140 assert isinstance(conf, config.Config) 144​ assert isinstance(conf, config.Config)
141 145​
142 def test_devel_profile_no_config(self, monkeypatch): 146​ def test_devel_profile_no_config(self, monkeypatch):
143 '''When there are no config files and no envvar, the profile should be 147​ '''When there are no config files and no envvar, the profile should be
144 set to 'development' ''' 148​ set to 'development' '''
145 self.unset_profile(monkeypatch) 149​ self.unset_profile(monkeypatch)
146 self.disable_create_dirs(monkeypatch) 150​ self.disable_create_dirs(monkeypatch)
147 # make sure we don't find any config files 151​ # make sure we don't find any config files
148 monkeypatch.setattr(config, 'CONF_DIRS', []) 152​ monkeypatch.setattr(config, 'CONF_DIRS', [])
149 conf = config.get_config() 153​ conf = config.get_config()
150 assert conf.profile == config.ProfileName.DEVELOPMENT 154​ assert conf.profile == config.ProfileName.DEVELOPMENT