From 8e4b05214ac14416a2f229b89d3db82ab6fe7c3e Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 23 Jul 2019 10:39:45 -0600 Subject: [PATCH 01/12] add --coverage for integration tests --- .coveragerc | 2 + integration/conftest.py | 66 ++++++++++++++++++++---------- integration/test_magic_folder.py | 4 +- integration/util.py | 37 +++++++++++------ src/allmydata/scripts/runner.py | 38 +++++++++++++++++ src/allmydata/test/cli/test_cli.py | 5 ++- tox.ini | 6 ++- 7 files changed, 119 insertions(+), 39 deletions(-) diff --git a/.coveragerc b/.coveragerc index 4028f8ea0..eaac16d82 100644 --- a/.coveragerc +++ b/.coveragerc @@ -8,3 +8,5 @@ source = omit = */allmydata/test/* */allmydata/_version.py +parallel = True +branch = True diff --git a/integration/conftest.py b/integration/conftest.py index 98091c7ce..d11970cb1 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -41,6 +41,10 @@ def pytest_addoption(parser): "--keep-tempdir", action="store_true", dest="keep", help="Keep the tmpdir with the client directories (introducer, etc)", ) + parser.addoption( + "--coverage", action="store_true", dest="coverage", + help="Collect coverage statistics", + ) @pytest.fixture(autouse=True, scope='session') def eliot_logging(): @@ -154,6 +158,24 @@ def flog_gatherer(reactor, temp_dir, flog_binary, request): return furl +def _tahoe_runner_optional_coverage(proto, reactor, request, other_args): + """ + Internal helper. Calls spawnProcess with `-m + allmydata.scripts.runner` and `other_args`, optionally inserting a + `--coverage` option if the `request` indicates we should. + """ + if request.config.getoption('coverage'): + args = [sys.executable, '-m', 'coverage', 'run', '-m', 'allmydata.scripts.runner', '--coverage'] + else: + args = [sys.executable, '-m', 'allmydata.scripts.runner'] + args += other_args + return reactor.spawnProcess( + proto, + sys.executable, + args, + ) + + @pytest.fixture(scope='session') @log_call( action_type=u"integration:introducer", @@ -174,11 +196,11 @@ log_gatherer.furl = {log_furl} if not exists(intro_dir): mkdir(intro_dir) done_proto = _ProcessExitedProtocol() - reactor.spawnProcess( + _tahoe_runner_optional_coverage( done_proto, - sys.executable, + reactor, + request, ( - sys.executable, '-m', 'allmydata.scripts.runner', 'create-introducer', '--listen=tcp', '--hostname=localhost', @@ -195,11 +217,11 @@ log_gatherer.furl = {log_furl} # but on linux it means daemonize. "tahoe run" is consistent # between platforms. protocol = _MagicTextProtocol('introducer running') - process = reactor.spawnProcess( + process = _tahoe_runner_optional_coverage( protocol, - sys.executable, + reactor, + request, ( - sys.executable, '-m', 'allmydata.scripts.runner', 'run', intro_dir, ), @@ -241,11 +263,11 @@ log_gatherer.furl = {log_furl} if not exists(intro_dir): mkdir(intro_dir) done_proto = _ProcessExitedProtocol() - reactor.spawnProcess( + _tahoe_runner_optional_coverage( done_proto, - sys.executable, + reactor, + request, ( - sys.executable, '-m', 'allmydata.scripts.runner', 'create-introducer', '--tor-control-port', 'tcp:localhost:8010', '--listen=tor', @@ -262,11 +284,11 @@ log_gatherer.furl = {log_furl} # but on linux it means daemonize. "tahoe run" is consistent # between platforms. protocol = _MagicTextProtocol('introducer running') - process = reactor.spawnProcess( + process = _tahoe_runner_optional_coverage( protocol, - sys.executable, + reactor, + request, ( - sys.executable, '-m', 'allmydata.scripts.runner', 'run', intro_dir, ), @@ -365,11 +387,11 @@ def alice_invite(reactor, alice, temp_dir, request): # consistently fail if we don't hack in this pause...) import time ; time.sleep(5) proto = _CollectOutputProtocol() - reactor.spawnProcess( + _tahoe_runner_optional_coverage( proto, - sys.executable, + reactor, + request, [ - sys.executable, '-m', 'allmydata.scripts.runner', 'magic-folder', 'create', '--poll-interval', '2', '--basedir', node_dir, 'magik:', 'alice', @@ -380,11 +402,11 @@ def alice_invite(reactor, alice, temp_dir, request): with start_action(action_type=u"integration:alice:magic_folder:invite") as a: proto = _CollectOutputProtocol() - reactor.spawnProcess( + _tahoe_runner_optional_coverage( proto, - sys.executable, + reactor, + request, [ - sys.executable, '-m', 'allmydata.scripts.runner', 'magic-folder', 'invite', '--basedir', node_dir, 'magik:', 'bob', ] @@ -416,13 +438,13 @@ def magic_folder(reactor, alice_invite, alice, bob, temp_dir, request): print("pairing magic-folder") bob_dir = join(temp_dir, 'bob') proto = _CollectOutputProtocol() - reactor.spawnProcess( + _tahoe_runner_optional_coverage( proto, - sys.executable, + reactor, + request, [ - sys.executable, '-m', 'allmydata.scripts.runner', 'magic-folder', 'join', - '--poll-interval', '2', + '--poll-interval', '1', '--basedir', bob_dir, alice_invite, join(temp_dir, 'magic-bob'), diff --git a/integration/test_magic_folder.py b/integration/test_magic_folder.py index 2691639e6..c0f630847 100644 --- a/integration/test_magic_folder.py +++ b/integration/test_magic_folder.py @@ -408,7 +408,7 @@ def test_alice_adds_files_while_bob_is_offline(reactor, request, temp_dir, magic bob_node_dir = join(temp_dir, "bob") # Take Bob offline. - yield util.cli(reactor, bob_node_dir, "stop") + yield util.cli(request, reactor, bob_node_dir, "stop") # Create a couple files in Alice's local directory. some_files = list( @@ -422,7 +422,7 @@ def test_alice_adds_files_while_bob_is_offline(reactor, request, temp_dir, magic good = False for i in range(15): - status = yield util.magic_folder_cli(reactor, alice_node_dir, "status") + status = yield util.magic_folder_cli(request, reactor, alice_node_dir, "status") good = status.count(".added-while-offline (36 B): good, version=0") == len(some_files) * 2 if good: # We saw each file as having a local good state and a remote good diff --git a/integration/util.py b/integration/util.py index 4dd20aa49..e4685783c 100644 --- a/integration/util.py +++ b/integration/util.py @@ -117,8 +117,8 @@ def _cleanup_twistd_process(twistd_process, exited): :return: After the process has exited. """ try: - print("signaling {} with KILL".format(twistd_process.pid)) - twistd_process.signalProcess('KILL') + print("signaling {} with TERM".format(twistd_process.pid)) + twistd_process.signalProcess('TERM') print("signaled, blocking on exit") pytest_twisted.blockon(exited) print("exited, goodbye") @@ -134,15 +134,18 @@ def _run_node(reactor, node_dir, request, magic_text): # on windows, "tahoe start" means: run forever in the foreground, # but on linux it means daemonize. "tahoe run" is consistent # between platforms. + if request.config.getoption('coverage'): + args = [sys.executable, '-m', 'coverage', 'run', '-m', 'allmydata.scripts.runner', '--coverage'] + else: + args = [sys.executable, '-m', 'allmydata.scripts.runner'] process = reactor.spawnProcess( protocol, sys.executable, - ( - sys.executable, '-m', 'allmydata.scripts.runner', + args + [ '--eliot-destination', 'file:{}/logs/eliot.json'.format(node_dir), 'run', node_dir, - ), + ], ) process.exited = protocol.exited @@ -178,8 +181,11 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam print("creating", node_dir) mkdir(node_dir) done_proto = _ProcessExitedProtocol() - args = [ - sys.executable, '-m', 'allmydata.scripts.runner', + if request.config.getoption('coverage'): + args = [sys.executable, '-m', 'coverage', 'run', '-m', 'allmydata.scripts.runner', '--coverage'] + else: + args = [sys.executable, '-m', 'allmydata.scripts.runner'] + args = args + [ 'create-node', '--nickname', name, '--introducer', introducer_furl, @@ -331,17 +337,24 @@ def await_file_vanishes(path, timeout=10): raise FileShouldVanishException(path, timeout) -def cli(reactor, node_dir, *argv): +def cli(request, reactor, node_dir, *argv): + """ + Run a tahoe CLI subcommand for a given node, optionally running + under coverage if '--coverage' was supplied. + """ proto = _CollectOutputProtocol() + if request.config.getoption('coverage'): + args = [sys.executable, '-m', 'coverage', 'run', '-m', 'allmydata.scripts.runner'] + else: + args = [sys.executable, '-m', 'allmydata.scripts.runner'] reactor.spawnProcess( proto, sys.executable, - [ - sys.executable, '-m', 'allmydata.scripts.runner', + args + [ '--node-directory', node_dir, ] + list(argv), ) return proto.done -def magic_folder_cli(reactor, node_dir, *argv): - return cli(reactor, node_dir, "magic-folder", *argv) +def magic_folder_cli(request, reactor, node_dir, *argv): + return cli(request, reactor, node_dir, "magic-folder", *argv) diff --git a/src/allmydata/scripts/runner.py b/src/allmydata/scripts/runner.py index 327325e95..71249c432 100644 --- a/src/allmydata/scripts/runner.py +++ b/src/allmydata/scripts/runner.py @@ -194,7 +194,45 @@ def run(): # doesn't return: calls sys.exit(rc) task.react(_run_with_reactor) + +def _setup_coverage(reactor): + """ + Arrange for coverage to be collected if the 'coverage' package is + installed + """ + # can we put this _setup_coverage call after we hit + # argument-parsing? + if not '--coverage' in sys.argv: + return + sys.argv.remove('--coverage') + + try: + import coverage + except ImportError: + return + + os.environ["COVERAGE_PROCESS_START"] = '.coveragerc' + # maybe-start the global coverage, unless it already got started + cov = coverage.process_startup() + if cov is None: + cov = coverage.process_startup.coverage + + def write_coverage_data(*args, **kw): + """ + Make sure that coverage has stopped; internally, it depends on + ataxit handlers running which doesn't always happen (Twisted's + shutdown hook also won't run if os._exit() is called, but it + runs more-often than atexit handlers). + """ + cov.stop() + cov.save() + reactor.addSystemEventTrigger('after', 'shutdown', write_coverage_data) + + def _run_with_reactor(reactor): + + _setup_coverage(reactor) + d = defer.maybeDeferred(parse_or_exit_with_explanation, sys.argv[1:]) d.addCallback(_maybe_enable_eliot_logging, reactor) d.addCallback(dispatch) diff --git a/src/allmydata/test/cli/test_cli.py b/src/allmydata/test/cli/test_cli.py index 1f7f90c7f..8ea2966c6 100644 --- a/src/allmydata/test/cli/test_cli.py +++ b/src/allmydata/test/cli/test_cli.py @@ -2,7 +2,7 @@ import os.path from six.moves import cStringIO as StringIO import urllib, sys import re -from mock import patch +from mock import patch, Mock from twisted.trial import unittest from twisted.python.monkey import MonkeyPatcher @@ -525,7 +525,8 @@ class CLI(CLITestMixin, unittest.TestCase): self.failUnlessEqual(exitcode, 1) def fake_react(f): - d = f("reactor") + reactor = Mock() + d = f(reactor) # normally this Deferred would be errbacked with SystemExit, but # since we mocked out sys.exit, it will be fired with None. So # it's safe to drop it on the floor. diff --git a/tox.ini b/tox.ini index 6164edd5e..3603ea36a 100644 --- a/tox.ini +++ b/tox.ini @@ -49,9 +49,13 @@ commands = trial {env:TAHOE_LAFS_TRIAL_ARGS:--rterrors} {posargs:allmydata} [testenv:integration] +setenv = + COVERAGE_PROCESS_START=.coveragerc commands = # NOTE: 'run with "py.test --keep-tempdir -s -v integration/" to debug failures' - py.test -v integration/ + py.test --coverage -v integration/ + coverage combine + coverage report [testenv:coverage] # coverage (with --branch) takes about 65% longer to run From 3b3626244e5ca873b9f5961c9442d059887a7624 Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 7 Aug 2019 12:39:29 -0600 Subject: [PATCH 02/12] cleanup --- src/allmydata/scripts/runner.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/allmydata/scripts/runner.py b/src/allmydata/scripts/runner.py index 71249c432..0b94c5349 100644 --- a/src/allmydata/scripts/runner.py +++ b/src/allmydata/scripts/runner.py @@ -211,13 +211,17 @@ def _setup_coverage(reactor): except ImportError: return + # this doesn't change the shell's notion of the environment, but + # it makes the test in process_startup() succeed, which is the + # goal here. os.environ["COVERAGE_PROCESS_START"] = '.coveragerc' + # maybe-start the global coverage, unless it already got started cov = coverage.process_startup() if cov is None: cov = coverage.process_startup.coverage - def write_coverage_data(*args, **kw): + def write_coverage_data(): """ Make sure that coverage has stopped; internally, it depends on ataxit handlers running which doesn't always happen (Twisted's From 113c0a690c515bb6c5fd4fe5bbb48eb82891cc6d Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 7 Aug 2019 13:38:35 -0600 Subject: [PATCH 03/12] newsfragment --- newsfragments/3234.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/3234.bugfix diff --git a/newsfragments/3234.bugfix b/newsfragments/3234.bugfix new file mode 100644 index 000000000..4de92db53 --- /dev/null +++ b/newsfragments/3234.bugfix @@ -0,0 +1 @@ +Add coverage information to integration tests From 43162f2ffe866a13cc99b88e69989da36d24228b Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 7 Aug 2019 13:51:05 -0600 Subject: [PATCH 04/12] combine for normal runs, too --- tox.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/tox.ini b/tox.ini index 3603ea36a..9efc839b6 100644 --- a/tox.ini +++ b/tox.ini @@ -68,6 +68,7 @@ commands = pip freeze tahoe --version coverage run --branch -m twisted.trial {env:TAHOE_LAFS_TRIAL_ARGS:--rterrors --reporter=timing} {posargs:allmydata} + coverage combine coverage xml [testenv:codechecks] From 20e191a8b91b71d9299e1b1f1627037f2ef4edde Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 7 Aug 2019 13:59:29 -0600 Subject: [PATCH 05/12] nicer message --- newsfragments/3234.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/newsfragments/3234.bugfix b/newsfragments/3234.bugfix index 4de92db53..35dd44a1a 100644 --- a/newsfragments/3234.bugfix +++ b/newsfragments/3234.bugfix @@ -1 +1 @@ -Add coverage information to integration tests +Collect coverage information from integration tests From 358f0c9ead985ec9a04604280f1e9b176edaf288 Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 7 Aug 2019 13:59:40 -0600 Subject: [PATCH 06/12] error if --coverage but no coverage package --- src/allmydata/scripts/runner.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/allmydata/scripts/runner.py b/src/allmydata/scripts/runner.py index 0b94c5349..24aa66106 100644 --- a/src/allmydata/scripts/runner.py +++ b/src/allmydata/scripts/runner.py @@ -202,14 +202,16 @@ def _setup_coverage(reactor): """ # can we put this _setup_coverage call after we hit # argument-parsing? - if not '--coverage' in sys.argv: + if '--coverage' not in sys.argv: return sys.argv.remove('--coverage') try: import coverage except ImportError: - return + except RuntimeError( + "The 'coveage' package must be installed to use --coverage" + ) # this doesn't change the shell's notion of the environment, but # it makes the test in process_startup() succeed, which is the From 8cde74b49a8fc55c232beb1eed633190a4c1489f Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 7 Aug 2019 14:00:24 -0600 Subject: [PATCH 07/12] bugfix -> misc --- newsfragments/{3234.bugfix => 3234.misc} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename newsfragments/{3234.bugfix => 3234.misc} (100%) diff --git a/newsfragments/3234.bugfix b/newsfragments/3234.misc similarity index 100% rename from newsfragments/3234.bugfix rename to newsfragments/3234.misc From 025b89855ff3fd026774f36d2359f0ef3ca3dd57 Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 7 Aug 2019 14:03:16 -0600 Subject: [PATCH 08/12] refactor so we only check coverage in one place --- integration/conftest.py | 19 +------------------ integration/util.py | 30 +++++++++++++++++++++++------- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/integration/conftest.py b/integration/conftest.py index d11970cb1..afc58fcd1 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -31,6 +31,7 @@ from util import ( _create_node, _run_node, _cleanup_twistd_process, + _tahoe_runner_optional_coverage, ) @@ -158,24 +159,6 @@ def flog_gatherer(reactor, temp_dir, flog_binary, request): return furl -def _tahoe_runner_optional_coverage(proto, reactor, request, other_args): - """ - Internal helper. Calls spawnProcess with `-m - allmydata.scripts.runner` and `other_args`, optionally inserting a - `--coverage` option if the `request` indicates we should. - """ - if request.config.getoption('coverage'): - args = [sys.executable, '-m', 'coverage', 'run', '-m', 'allmydata.scripts.runner', '--coverage'] - else: - args = [sys.executable, '-m', 'allmydata.scripts.runner'] - args += other_args - return reactor.spawnProcess( - proto, - sys.executable, - args, - ) - - @pytest.fixture(scope='session') @log_call( action_type=u"integration:introducer", diff --git a/integration/util.py b/integration/util.py index e4685783c..6f2aee8a8 100644 --- a/integration/util.py +++ b/integration/util.py @@ -126,6 +126,24 @@ def _cleanup_twistd_process(twistd_process, exited): pass +def _tahoe_runner_optional_coverage(proto, reactor, request, other_args): + """ + Internal helper. Calls spawnProcess with `-m + allmydata.scripts.runner` and `other_args`, optionally inserting a + `--coverage` option if the `request` indicates we should. + """ + if request.config.getoption('coverage'): + args = [sys.executable, '-m', 'coverage', 'run', '-m', 'allmydata.scripts.runner', '--coverage'] + else: + args = [sys.executable, '-m', 'allmydata.scripts.runner'] + args += other_args + return reactor.spawnProcess( + proto, + sys.executable, + args, + ) + + def _run_node(reactor, node_dir, request, magic_text): if magic_text is None: magic_text = "client running" @@ -134,14 +152,12 @@ def _run_node(reactor, node_dir, request, magic_text): # on windows, "tahoe start" means: run forever in the foreground, # but on linux it means daemonize. "tahoe run" is consistent # between platforms. - if request.config.getoption('coverage'): - args = [sys.executable, '-m', 'coverage', 'run', '-m', 'allmydata.scripts.runner', '--coverage'] - else: - args = [sys.executable, '-m', 'allmydata.scripts.runner'] - process = reactor.spawnProcess( + + process = _tahoe_runner_optional_coverage( protocol, - sys.executable, - args + [ + reactor, + request, + [ '--eliot-destination', 'file:{}/logs/eliot.json'.format(node_dir), 'run', node_dir, From 0f6002b935936fe0a18bc94bdbaf7a5248737c33 Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 7 Aug 2019 14:08:23 -0600 Subject: [PATCH 09/12] raise not except --- src/allmydata/scripts/runner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/scripts/runner.py b/src/allmydata/scripts/runner.py index 24aa66106..751e27158 100644 --- a/src/allmydata/scripts/runner.py +++ b/src/allmydata/scripts/runner.py @@ -209,7 +209,7 @@ def _setup_coverage(reactor): try: import coverage except ImportError: - except RuntimeError( + raise RuntimeError( "The 'coveage' package must be installed to use --coverage" ) From 899fae5a80bb9b212be40541ae83b9406427f553 Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 7 Aug 2019 14:42:26 -0600 Subject: [PATCH 10/12] misc -> other --- newsfragments/{3234.misc => 3234.other} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename newsfragments/{3234.misc => 3234.other} (100%) diff --git a/newsfragments/3234.misc b/newsfragments/3234.other similarity index 100% rename from newsfragments/3234.misc rename to newsfragments/3234.other From 97e130aa259f07c253bb75c3d25e05113e87f3e5 Mon Sep 17 00:00:00 2001 From: meejah Date: Thu, 8 Aug 2019 09:52:00 -0600 Subject: [PATCH 11/12] refactor; use _tahoe_runner_optional_coverage --- integration/util.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/integration/util.py b/integration/util.py index 6f2aee8a8..029492f2a 100644 --- a/integration/util.py +++ b/integration/util.py @@ -359,16 +359,9 @@ def cli(request, reactor, node_dir, *argv): under coverage if '--coverage' was supplied. """ proto = _CollectOutputProtocol() - if request.config.getoption('coverage'): - args = [sys.executable, '-m', 'coverage', 'run', '-m', 'allmydata.scripts.runner'] - else: - args = [sys.executable, '-m', 'allmydata.scripts.runner'] - reactor.spawnProcess( - proto, - sys.executable, - args + [ - '--node-directory', node_dir, - ] + list(argv), + _tahoe_runner_optional_coverage( + proto, reactor, request, + ['--node-directory', node_dir] + list(argv), ) return proto.done From 0227b0945e860f74a47a790450424c6065cd0509 Mon Sep 17 00:00:00 2001 From: meejah Date: Thu, 8 Aug 2019 09:53:57 -0600 Subject: [PATCH 12/12] refactor; use _tahoe_runner_optional_coverage --- integration/util.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/integration/util.py b/integration/util.py index 029492f2a..41327bead 100644 --- a/integration/util.py +++ b/integration/util.py @@ -197,11 +197,7 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam print("creating", node_dir) mkdir(node_dir) done_proto = _ProcessExitedProtocol() - if request.config.getoption('coverage'): - args = [sys.executable, '-m', 'coverage', 'run', '-m', 'allmydata.scripts.runner', '--coverage'] - else: - args = [sys.executable, '-m', 'allmydata.scripts.runner'] - args = args + [ + args = [ 'create-node', '--nickname', name, '--introducer', introducer_furl, @@ -216,11 +212,7 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam args.append('--no-storage') args.append(node_dir) - reactor.spawnProcess( - done_proto, - sys.executable, - args, - ) + _tahoe_runner_optional_coverage(done_proto, reactor, request, args) created_d = done_proto.done def created(_):