[PBCKP-314] rewrite fio_mkdir to use pio#563
Conversation
fukanchik
force-pushed
the
REL_2_6-PBCKP-314
branch
from
8c2a823 to
9272dc0
Compare
fukanchik
force-pushed
the
REL_2_6-PBCKP-314
branch
from
9272dc0 to
b8d6ab0
Compare
|
|
||
| module_name = 'init' | ||
|
|
||
| DIR_PERMISSION = 0o700 |
There was a problem hiding this comment.
На винде обломается. Там возвращает 0o777.
| self.assertEqual( | ||
| dir_files(backup_dir), | ||
| ['backups', 'wal'] | ||
| CATALOG_DIRS |
There was a problem hiding this comment.
можно было не выделять в константу:
- сохранить dir_files(backup_dir) в переменную
- сравнить с
['backups', 'wal'] - использовать эту переменную ниже в цикле.
Но не настаиваю. Если хочешь, пусть будет CATALOG_DIRS.
There was a problem hiding this comment.
Да, явный перечень директорий лучше. Если что-то когда-то изменится легче будет локализовать изменения.
|
|
||
| for subdir in CATALOG_DIRS: | ||
| dirname = os.path.join(backup_dir, subdir) | ||
| self.assertEqual(DIR_PERMISSION, stat.S_IMODE(os.stat(dirname).st_mode)) |
There was a problem hiding this comment.
имхо, лишний чек.
Но если хочешь, сделай его тогда работающим на Винде: DIR_PERMISSION = 0o777
|
|
||
| try: | ||
| self.init_pb(backup_dir, cleanup=False) | ||
| self.fail("This should have failed due to non empty catalog dir.") |
There was a problem hiding this comment.
А почему такого self.fail нет в других тестах? Разве они не каждый раз должны ошибку выбрасывать?
There was a problem hiding this comment.
Может использовать self.assertRaisesRegex ?
Он, конечно, только для 3го питона... Ну когда докатится до тестировщиков, либо они согласятся третий скомпилить, либо скопипастим реализацию.
There was a problem hiding this comment.
А почему такого self.fail нет в других тестах? Разве они не каждый раз должны ошибку выбрасывать?
отсутствие такого self.fail это ошибка. если в сишном коде ожидается elog(ERROR) а его нет -- это тоже нужно проверять. Это стоит добавить в другое места тоже.
добавлено: нет всё же там есть. вот как это описано например в test_add_instance_idempotence():
self.assertEqual(1, 0)
There was a problem hiding this comment.
Может использовать self.assertRaisesRegex ?
Да, это более явное выражение мысли. Я переделаю.
К сожалению не ясно на какую версию питона расчитывать. Тут оказывается в гитхабе мы используем не поддерживаемую версию.
DEPRECATION: Python 3.5 reached the end of its life on September 13th, 2020. Please upgrade your Python as Python 3.5 is no longer maintained. pip 21.0 will drop support for Python 3.5 in January 2021. pip 21.0 will remove support for this functionality.
| os.chmod(no_access_dir, stat.S_IREAD) | ||
|
|
||
| try: | ||
| self.init_pb(backup_dir, cleanup=False) |
There was a problem hiding this comment.
А вот тут cleanup=False разве нужен? Кажется, что нет.
| os.chmod(no_access_dir, stat.S_IREAD|stat.S_IEXEC) | ||
|
|
||
| try: | ||
| self.init_pb(backup_dir, cleanup=False) |
There was a problem hiding this comment.
Зачистка в init_pb существует только потому что в tearDown() не вызывается self.del_test_dir. Я бы её оттуда вообще убрал, а del_test_dir перенёс в tearDown.
Во всех этих трёх тестах del_test_dir вызывается в блоке finally так что зачистка в init_pb им не нужна.
…ss and test_init_backup_catalog_no_write.
3 participants
No description provided.