diff --git a/.travis.yml b/.travis.yml index 4bb4149a..5f0f4943 100644 --- a/.travis.yml +++ b/.travis.yml @@ -139,7 +139,7 @@ env: - PIP_NO_WARN_SCRIPT_LOCATION=1 - CFLAGS="-pipe -std=gnu++11" - CXXFLAGS="-pipe -std=gnu++11" - - RS_TEST_CMD="-m zope.testrunner --test-path=src --auto-color --auto-progress -vvv --slow-test=3" + - RS_TEST_CMD="-m zope.testrunner --test-path=src --auto-color --auto-progress -v --slow-test=3" # Uploading built wheels for releases. # TWINE_PASSWORD is encrypted and stored directly in the # travis repo settings. diff --git a/.travis/zope_testrunner_gevent.py b/.travis/zope_testrunner_gevent.py index c26e0cc7..7134a370 100644 --- a/.travis/zope_testrunner_gevent.py +++ b/.travis/zope_testrunner_gevent.py @@ -23,7 +23,7 @@ sys.argv[:] = [ 'zope-testrunner', '--test-path=src', - '-vvv', + '-v', '--color', ] + sys.argv[1:] print(sys.argv) diff --git a/src/relstorage/storage/copy.py b/src/relstorage/storage/copy.py index bcab6dad..b3912b8e 100644 --- a/src/relstorage/storage/copy.py +++ b/src/relstorage/storage/copy.py @@ -291,7 +291,17 @@ def __next__(self): if self.cookie is self: # First time in. self.cookie = None - oid, tid, state, self.cookie = self.storage.record_iternext(self.cookie) + try: + result = self.storage.record_iternext(self.cookie) + except ValueError: + # FileStorage can raise this if the underlying storage + # is completely empty. + # See https://github.com/zopefoundation/ZODB/issues/330 + if self.cookie is None: + raise StopIteration + raise # pragma: no cover + + oid, tid, state, self.cookie = result return oid, tid, state next = __next__ # Py2 diff --git a/src/relstorage/tests/reltestbase.py b/src/relstorage/tests/reltestbase.py index 769ede81..dd8013cc 100644 --- a/src/relstorage/tests/reltestbase.py +++ b/src/relstorage/tests/reltestbase.py @@ -27,6 +27,7 @@ import time import threading import unittest +from textwrap import dedent import transaction from persistent import Persistent @@ -65,7 +66,7 @@ from . import skipIfNoConcurrentWriters from .persistentcache import PersistentCacheStorageTests from .locking import TestLocking -from .test_zodbconvert import FSZODBConvertTests +from .test_zodbconvert import ZlibWrappedFSZODBConvertTests class RelStorageTestBase(StorageCreatingMixin, TestCase, @@ -1465,7 +1466,7 @@ def check_record_iternext_too_large_oid(self): class AbstractRSZodbConvertTests(StorageCreatingMixin, - FSZODBConvertTests, + ZlibWrappedFSZODBConvertTests, # This one isn't cooperative in # setUp(), so it needs to be last. ZODB.tests.util.TestCase): @@ -1480,42 +1481,51 @@ class AbstractRSZodbConvertTests(StorageCreatingMixin, def setUp(self): super(AbstractRSZodbConvertTests, self).setUp() - - cfg = """ - %%import relstorage - %%import zc.zlibstorage - - - path %s - - - - - %s - cache-prefix %s - cache-local-dir %s - - - """ % ( - self.filestorage_name, - self.filestorage_file, - self.relstorage_name, - self.get_adapter_zconfig(), - self.relstorage_name, - os.path.abspath('.'), - ) - self._write_cfg(cfg) - + # Zap the storage self.make_storage(zap=True).close() - def _wrap_storage(self, storage): - return self._closing(ZlibStorage(storage)) + def make_storage(self, zap=True, **kw): + if kw: + raise TypeError("kwargs not supported") + if self.relstorage_name == 'source': + meth = self._create_src_storage + else: + meth = self._create_dest_storage + + storage = meth() + if zap: + storage.zap_all(slow=self.zap_slow) + return storage - def _create_dest_storage(self): - return self._wrap_storage(super(AbstractRSZodbConvertTests, self)._create_dest_storage()) - def _create_src_storage(self): - return self._wrap_storage(super(AbstractRSZodbConvertTests, self)._create_src_storage()) + def _cfg_header(self): + return '%import relstorage\n' + super(AbstractRSZodbConvertTests, self)._cfg_header() + + def _cfg_relstorage(self, name, _path, blob_dir): + cfg = dedent(""" + + %(rs_config)s + keep-history %(rs_keep_history)s + blob-dir %(rs_blobs)s + cache-prefix %(rs_name)s + cache-local-dir %(rs_cache_path)s + + """ % { + 'rs_name': name, + 'rs_keep_history': 'true' if self.keep_history else 'false', + 'rs_blobs': blob_dir, + 'rs_config': self.get_adapter_zconfig(), + 'rs_cache_path': os.path.abspath('.'), + }) + return cfg + + def _cfg_one(self, name, path, blob_dir): + if name == self.filestorage_name: + meth = self._cfg_filestorage + else: + assert name == self.relstorage_name + meth = self._cfg_relstorage + return meth(name, path, blob_dir) def test_new_instance_still_zlib(self): storage = self._closing(self.make_storage()) @@ -1529,19 +1539,26 @@ def test_new_instance_still_zlib(self): self.assertIn('_crs_untransform_record_data', new_storage.base.__dict__) self.assertIn('_crs_transform_record_data', new_storage.base.__dict__) -class AbstractRSDestZodbConvertTests(AbstractRSZodbConvertTests): +class AbstractRSDestHPZodbConvertTests(AbstractRSZodbConvertTests): + keep_history = True + zap_supported_by_dest = True + dest_db_needs_closed_before_zodbconvert = False + @property + def filestorage_file(self): + return self.srcfile +class AbstractRSDestHFZodbConvertTests(AbstractRSZodbConvertTests): + keep_history = False zap_supported_by_dest = True + dest_db_needs_closed_before_zodbconvert = False @property def filestorage_file(self): return self.srcfile - def _create_dest_storage(self): - return self._closing(self.make_storage(cache_prefix=self.relstorage_name, zap=False)) class AbstractRSSrcZodbConvertTests(AbstractRSZodbConvertTests): - + src_db_needs_closed_before_zodbconvert = False filestorage_name = 'destination' relstorage_name = 'source' @@ -1549,9 +1566,6 @@ class AbstractRSSrcZodbConvertTests(AbstractRSZodbConvertTests): def filestorage_file(self): return self.destfile - def _create_src_storage(self): - return self._closing(self.make_storage(cache_prefix=self.relstorage_name, zap=False)) - class AbstractIDBOptionsTest(unittest.TestCase): db_options = None diff --git a/src/relstorage/tests/test_zodbconvert.py b/src/relstorage/tests/test_zodbconvert.py index 35c7c39a..6a92b2fe 100644 --- a/src/relstorage/tests/test_zodbconvert.py +++ b/src/relstorage/tests/test_zodbconvert.py @@ -20,11 +20,15 @@ import tempfile import unittest from contextlib import contextmanager +from textwrap import dedent import transaction -from zc.zlibstorage import ZlibStorage + +from zope.testing.setupstack import rmtree from ZODB.DB import DB from ZODB.FileStorage import FileStorage +from ZODB.blob import Blob +from ZODB.tests.util import TestCase from relstorage.zodbconvert import main @@ -37,7 +41,7 @@ def test(self): func(self) return test -class AbstractZODBConvertBase(unittest.TestCase): +class AbstractZODBConvertBase(TestCase): cfgfile = None # Set to True in a subclass if the destination can be zapped @@ -46,8 +50,15 @@ class AbstractZODBConvertBase(unittest.TestCase): def setUp(self): super(AbstractZODBConvertBase, self).setUp() self._to_close = [] + self.__src_db = None + self.__dest_db = None + # zodb.tests.util.TestCase takes us to a temporary directory + # as our working directory, and also sets it to tempfile.tempdir; + # but don't require subclasses to know that. + self.parent_temp_directory = tempfile.tempdir def tearDown(self): + self.flush_changes_before_zodbconvert() for i in self._to_close: i.close() self._to_close = [] @@ -69,6 +80,22 @@ def tearDown(self): gc.collect() super(AbstractZODBConvertBase, self).tearDown() + src_db_needs_closed_before_zodbconvert = True + dest_db_needs_closed_before_zodbconvert = True + + def flush_changes_before_zodbconvert(self): + for db_name in 'src_db', 'dest_db': + db_attr = '_' + AbstractZODBConvertBase.__name__ + '__' + db_name + db = getattr(self, db_attr) + needs_closed = getattr(self, db_name + '_needs_closed_before_zodbconvert') + + if needs_closed and db is not None: + db.close() + setattr(self, db_attr, None) + if db in self._to_close: + self._to_close.remove(db) + + def _closing(self, thing): self._to_close.append(thing) return thing @@ -80,10 +107,14 @@ def _create_dest_storage(self): raise NotImplementedError() def _create_src_db(self): - return self._closing(DB(self._closing(self._create_src_storage()))) + if self.__src_db is None: + self.__src_db = self._closing(DB(self._create_src_storage())) + return self.__src_db def _create_dest_db(self): - return self._closing(DB(self._closing(self._create_dest_storage()))) + if self.__dest_db is None: + self.__dest_db = self._closing(DB(self._create_dest_storage())) + return self.__dest_db @contextmanager def __conn(self, name): @@ -93,7 +124,6 @@ def __conn(self, name): yield conn finally: conn.close() - db.close() def _src_conn(self): return self.__conn('src') @@ -106,6 +136,16 @@ def __write_value_for_key_in_db(self, val, key, db_conn_func): conn.root()[key] = val transaction.commit() + def __check_value_of_key_in_db(self, val, key, db_conn_func): + with db_conn_func() as conn2: + db_val = conn2.root().get(key) + if isinstance(val, bytes): + self.assertIsInstance(db_val, Blob) + with db_val.open('r') as f: + db_val = f.read() + + self.assertEqual(db_val, val) + def _write_value_for_key_in_src(self, x, key='x'): self.__write_value_for_key_in_db(x, key, self._src_conn) @@ -113,40 +153,68 @@ def _write_value_for_key_in_dest(self, x, key='x'): self.__write_value_for_key_in_db(x, key, self._dest_conn) def _check_value_of_key_in_dest(self, x, key='x'): - with self._dest_conn() as conn2: - db_x = conn2.root().get(key) - self.assertEqual(db_x, x) + self.__check_value_of_key_in_db(x, key, self._dest_conn) + + def _check_value_of_key_in_src(self, x, key='x'): + self.__check_value_of_key_in_db(x, key, self._src_conn) + + def run_zodbconvert(self, *args): + self.flush_changes_before_zodbconvert() + return main(*args) def test_convert(self): self._write_value_for_key_in_src(10) - main(['', self.cfgfile]) + self.run_zodbconvert(['', self.cfgfile]) self._check_value_of_key_in_dest(10) - def test_dry_run(self): self._write_value_for_key_in_src(10) - main(['', '--dry-run', self.cfgfile]) + self.run_zodbconvert(['', '--dry-run', self.cfgfile]) self._check_value_of_key_in_dest(None) - def test_incremental(self): + def test_incremental_after_full_copy(self, first_convert_args=('',)): + from persistent.mapping import PersistentMapping + # Make sure to write new OIDs as well as new TIDs. + for i in range(9): + # Make sure to write new OIDs as well as new TIDs. + self._write_value_for_key_in_src(PersistentMapping(i=i), key=i) + self._write_value_for_key_in_src(Blob(b'abc'), 'the_blob') + self._write_value_for_key_in_src(Blob(b'def'), 'the_blob') self._write_value_for_key_in_src(10) - main(['', self.cfgfile]) - self._check_value_of_key_in_dest(10) + self._check_value_of_key_in_src(PersistentMapping(i=2), 2) + self.run_zodbconvert(first_convert_args + (self.cfgfile,)) + self._check_value_of_key_in_dest(10) + self._check_value_of_key_in_dest(PersistentMapping(i=2), 2) + self._check_value_of_key_in_dest(b'def', 'the_blob') + + for i in reversed(range(9)): + # Make sure to write new OIDs as well as new TIDs, + # But this time in *reverse* of the order we wrote them before. + self._write_value_for_key_in_src(PersistentMapping(i=i**2), key=i) + self._write_value_for_key_in_src(Blob(b'123'), 'the_blob') + self._check_value_of_key_in_src(PersistentMapping(i=4), 2) self._write_value_for_key_in_src("hi") - main(['', '--incremental', self.cfgfile]) + self.run_zodbconvert(['', '--incremental', self.cfgfile]) self._check_value_of_key_in_dest("hi") + self._check_value_of_key_in_dest(PersistentMapping(i=4), 2) + self._check_value_of_key_in_dest(b'123', 'the_blob') + + def test_incremental_after_incremental(self): + # In case they do the conversion in different orders + # depending on whether --incremental is specified or not. + self.test_incremental_after_full_copy(first_convert_args=('', '--incremental')) def test_incremental_empty_src_dest(self): # Should work and not raise a POSKeyError - main(['', '--incremental', self.cfgfile]) + self.run_zodbconvert(['', '--incremental', self.cfgfile]) self._check_value_of_key_in_dest(None) @skipIfZapNotSupportedByDest def test_clear_empty_dest(self): x = 10 self._write_value_for_key_in_src(x) - main(['', '--clear', self.cfgfile]) + self.run_zodbconvert(['', '--clear', self.cfgfile]) self._check_value_of_key_in_dest(x) @skipIfZapNotSupportedByDest @@ -159,24 +227,21 @@ def test_clear_full_dest(self): self._write_value_for_key_in_src(2, key='y') # omit z - main(['', '--clear', self.cfgfile]) + self.run_zodbconvert(['', '--clear', self.cfgfile]) self._check_value_of_key_in_dest(1, key='x') self._check_value_of_key_in_dest(2, key='y') self._check_value_of_key_in_dest(None, key='z') def test_no_overwrite(self): - db = self._create_src_db() # create the root object - db.close() - db = self._create_dest_db() # create the root object - db.close() - self.assertRaises(SystemExit, main, ['', self.cfgfile]) + self._create_src_db() # create the root object + self._create_dest_db() # create the root object + with self.assertRaises(SystemExit) as exc: + self.run_zodbconvert(['', self.cfgfile]) + + self.assertIn('Try --clear', str(exc.exception)) - # XXX: Add tests for: - # - copying a blob - # - verifying that a state becomes compressed or uncompressed. - # # Also: # - when the destination keeps history: Verifying iteration # of all the records, including transaction metadata @@ -184,61 +249,82 @@ def test_no_overwrite(self): # - If destination doesn't keep history, verifying only the most recent state# is saved. # # Some of that is probably handled in the History[Free|Preserving][From|To]FileStorageTest, - + # but I think that directly uses copyTransactionsFrom and doesn't have the other handling + # zodbconvert does (e.g., incremental) class FSZODBConvertTests(AbstractZODBConvertBase): + keep_history = True def setUp(self): super(FSZODBConvertTests, self).setUp() - fd, self.srcfile = tempfile.mkstemp() + fd, self.srcfile = tempfile.mkstemp('.fssource') os.close(fd) os.remove(self.srcfile) + self.src_blobs = tempfile.mkdtemp('.fssource') - fd, self.destfile = tempfile.mkstemp() + fd, self.destfile = tempfile.mkstemp('.fsdest') os.close(fd) os.remove(self.destfile) + self.dest_blobs = tempfile.mkdtemp('.fsdest') cfg = self._cfg_header() + self._cfg_source() + self._cfg_dest() - self._write_cfg(cfg) + self.cfgfile = self._write_cfg(cfg) def _cfg_header(self): return "" - def _cfg_source(self): - return """ - + def _cfg_filestorage(self, name, path, blob_dir): + return dedent(""" + path %s + blob-dir %s - """ % self.srcfile + """ % (name, path, blob_dir)) + + def _cfg_one(self, name, path, blob_dir): + return self._cfg_filestorage(name, path, blob_dir) + + def _cfg_source(self): + return self._cfg_one('source', self.srcfile, self.src_blobs) def _cfg_dest(self): - return """ - - path %s - - """ % self.destfile + return self._cfg_one('destination', self.destfile, self.dest_blobs) def _write_cfg(self, cfg): - fd, self.cfgfile = tempfile.mkstemp() + fd, cfgfile = tempfile.mkstemp('.conf', 'zodbconvert-') os.write(fd, cfg.encode('ascii')) os.close(fd) + return cfgfile def tearDown(self): - if os.path.exists(self.destfile): - os.remove(self.destfile) - if os.path.exists(self.srcfile): - os.remove(self.srcfile) - if os.path.exists(self.cfgfile): - os.remove(self.cfgfile) + for fname in self.destfile, self.srcfile, self.cfgfile: + if os.path.exists(fname): + os.remove(fname) + self.destfile = self.srcfile = self.cfgfile = None + for dname in self.src_blobs, self.dest_blobs: + if os.path.exists(dname): + rmtree(dname) + super(FSZODBConvertTests, self).tearDown() + def _load_zconfig(self): + from relstorage.zodbconvert import schema_xml + from relstorage.zodbconvert import StringIO + import ZConfig + + schema = ZConfig.loadSchemaFile(StringIO(schema_xml)) + conf, _ = ZConfig.loadConfig(schema, self.cfgfile) + return conf + def _create_src_storage(self): - return FileStorage(self.srcfile) + conf = self._load_zconfig() + return conf.source.open() def _create_dest_storage(self): - return FileStorage(self.destfile) + conf = self._load_zconfig() + return conf.destination.open() def test_storage_has_data(self): from relstorage.zodbconvert import storage_has_data @@ -248,32 +334,28 @@ def test_storage_has_data(self): db.close() self.assertTrue(storage_has_data(src)) -class ZlibWrappedZODBConvertTests(FSZODBConvertTests): +class ZlibWrappedFSZODBConvertTests(FSZODBConvertTests): + # XXX: Add tests for: + # - verifying that a state becomes compressed or uncompressed. + # def _cfg_header(self): return "%import zc.zlibstorage\n" def _cfg_source(self): return ("\n" - + super(ZlibWrappedZODBConvertTests, self)._cfg_source() + + super(ZlibWrappedFSZODBConvertTests, self)._cfg_source() + "") def _cfg_dest(self): return ("\n" - + super(ZlibWrappedZODBConvertTests, self)._cfg_dest() + + super(ZlibWrappedFSZODBConvertTests, self)._cfg_dest() + "") - def _create_src_storage(self): - return ZlibStorage(super(ZlibWrappedZODBConvertTests, self)._create_src_storage()) - - def _create_dest_storage(self): - return ZlibStorage(super(ZlibWrappedZODBConvertTests, self)._create_dest_storage()) - - def test_suite(): suite = unittest.TestSuite() suite.addTest(unittest.makeSuite(FSZODBConvertTests)) - suite.addTest(unittest.makeSuite(ZlibWrappedZODBConvertTests)) + suite.addTest(unittest.makeSuite(ZlibWrappedFSZODBConvertTests)) return suite if __name__ == '__main__': diff --git a/src/relstorage/tests/util.py b/src/relstorage/tests/util.py index e18ba8a6..a17338f3 100644 --- a/src/relstorage/tests/util.py +++ b/src/relstorage/tests/util.py @@ -363,11 +363,14 @@ def _make_test_classes(self): return classes def _make_zodbconvert_classes(self): - from .reltestbase import AbstractRSDestZodbConvertTests + from .reltestbase import AbstractRSDestHPZodbConvertTests + from .reltestbase import AbstractRSDestHFZodbConvertTests from .reltestbase import AbstractRSSrcZodbConvertTests classes = [] - for base in (AbstractRSSrcZodbConvertTests, AbstractRSDestZodbConvertTests): + for base in (AbstractRSSrcZodbConvertTests, + AbstractRSDestHPZodbConvertTests, + AbstractRSDestHFZodbConvertTests): klass = type( self.__name__ + base.__name__[8:], (self.use_adapter, base), diff --git a/src/relstorage/zodbconvert.py b/src/relstorage/zodbconvert.py index 6eb350b8..9462f2bc 100644 --- a/src/relstorage/zodbconvert.py +++ b/src/relstorage/zodbconvert.py @@ -20,11 +20,9 @@ import argparse import logging import sys -import traceback from io import StringIO import ZConfig -from zope.interface import providedBy from zope.interface import implementer from persistent.timestamp import TimeStamp @@ -214,11 +212,7 @@ def cleanup_and_exit(exit_msg=None): try: destination.copyTransactionsFrom(source) - except: - traceback.print_exc() - cleanup_and_exit(repr(sys.exc_info()[1])) - raise # Won't do anything, but keeps the linters happy - else: + finally: cleanup_and_exit()