From c9f6411e2b5e711cf31f3756c75ddb4465870b17 Mon Sep 17 00:00:00 2001 From: Vidar Tonaas Fauske Date: Thu, 13 Apr 2017 12:22:13 +0200 Subject: [PATCH 1/5] Use diff args for git diff driver --- nbdime/args.py | 5 +++-- nbdime/gitdiffdriver.py | 7 +++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/nbdime/args.py b/nbdime/args.py index 00a360f4..4796e1b7 100644 --- a/nbdime/args.py +++ b/nbdime/args.py @@ -197,9 +197,10 @@ def add_diff_args(parser): help="process/ignore metadata.") + +diff_exclusives = ('sources', 'outputs', 'attachments', 'metadata') def process_diff_flags(args): - exclusives = ('sources', 'outputs', 'attachments', 'metadata') - process_exclusive_ignorables(args, exclusives) + process_exclusive_ignorables(args, diff_exclusives) set_notebook_diff_targets(args.sources, args.outputs, args.attachments, args.metadata) diff --git a/nbdime/gitdiffdriver.py b/nbdime/gitdiffdriver.py index 7d46c8ab..b550c8ce 100644 --- a/nbdime/gitdiffdriver.py +++ b/nbdime/gitdiffdriver.py @@ -24,7 +24,7 @@ from subprocess import check_call, CalledProcessError from . import nbdiffapp -from .args import add_git_config_subcommand +from .args import add_git_config_subcommand, add_diff_args, diff_exclusives from .utils import locate_gitattributes, ensure_dir_exists @@ -77,6 +77,7 @@ def main(args=None): diff_parser = subparsers.add_parser('diff', description="The actual entrypoint for the diff tool. Git will call this." ) + add_diff_args(diff_parser) # Argument list # path old-file old-hex old-mode new-file new-hex new-mode [ rename-to ] diff_parser.add_argument('path') @@ -97,7 +98,9 @@ def main(args=None): opts = parser.parse_args(args) if opts.subcommand == 'diff': - return nbdiffapp.main([opts.a, opts.b]) + return nbdiffapp.main( + [opts.a, opts.b] + + ['--%s' % name for name in diff_exclusives if getattr(opts, name)]) elif opts.subcommand == 'config': opts.config_func(opts.scope) return 0 From 72a68435b5a4a2737b676f062da80d3018e1bbc7 Mon Sep 17 00:00:00 2001 From: Vidar Tonaas Fauske Date: Tue, 18 Apr 2017 10:32:33 +0200 Subject: [PATCH 2/5] Added test --- nbdime/tests/test_git_diffdriver.py | 43 +++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/nbdime/tests/test_git_diffdriver.py b/nbdime/tests/test_git_diffdriver.py index 46d78ddf..fd683793 100644 --- a/nbdime/tests/test_git_diffdriver.py +++ b/nbdime/tests/test_git_diffdriver.py @@ -39,6 +39,27 @@ """ +expected_source_only = """nbdiff {0} {1} +--- {0} {2} ++++ {1} {3} +## modified /cells/0/source: +@@ -1,3 +1,3 @@ +-def foe(x, y): ++def foo(x, y): + return x + y +-foe(3, 2) ++foo(1, 2) + +## modified /cells/1/source: +@@ -1,3 +1,3 @@ +-def foo(x, y): ++def foe(x, y): + return x * y +-foo(1, 2) ++foe(1, 2) + +""" + def test_git_diff_driver(capsys, nocolor, needs_git): # Simulate a call from `git diff` to check basic driver functionality @@ -60,3 +81,25 @@ def test_git_diff_driver(capsys, nocolor, needs_git): assert r == 0 cap_out = capsys.readouterr()[0] assert cap_out == expected_output.format(fn1, fn2, t1, t2) + + +def test_git_diff_driver_flags(capsys, nocolor, needs_git): + # Simulate a call from `git diff` to check basic driver functionality + test_dir = os.path.abspath(os.path.dirname(__file__)) + + fn1 = pjoin(test_dir, 'files/foo--1.ipynb') + fn2 = pjoin(test_dir, 'files/foo--2.ipynb') + t1 = file_timestamp(fn1) + t2 = file_timestamp(fn2) + + mock_argv = [ + '/mock/path/git-nbdiffdriver', 'diff', '-s', + fn1, + fn1, 'invalid_mock_checksum', '100644', + fn2, 'invalid_mock_checksum', '100644'] + + with mock.patch('sys.argv', mock_argv): + r = gdd_main() + assert r == 0 + cap_out = capsys.readouterr()[0] + assert cap_out == expected_source_only.format(fn1, fn2, t1, t2) From fe2321e3b86ac98e8a49078f7c5f56c9ee2ce9ed Mon Sep 17 00:00:00 2001 From: Vidar Tonaas Fauske Date: Tue, 18 Apr 2017 10:32:49 +0200 Subject: [PATCH 3/5] Add some extra logging info --- nbdime/webapp/nbdimeserver.py | 1 + 1 file changed, 1 insertion(+) diff --git a/nbdime/webapp/nbdimeserver.py b/nbdime/webapp/nbdimeserver.py index e1bbc656..212a9b1d 100644 --- a/nbdime/webapp/nbdimeserver.py +++ b/nbdime/webapp/nbdimeserver.py @@ -313,6 +313,7 @@ def init_app(on_port=None, closable=False, **params): app = make_app(**params) if port != 0 or on_port is None: app.listen(port, address=ip) + _logger.info('Listening on %s, port %d' % (ip, port)) else: sockets = netutil.bind_sockets(0, ip) server = httpserver.HTTPServer(app) From b78cadd327417b3896aa994073bcb662eb1f0fc0 Mon Sep 17 00:00:00 2001 From: Vidar Tonaas Fauske Date: Tue, 18 Apr 2017 13:25:05 +0200 Subject: [PATCH 4/5] Up test timeout for tools They do three request cycles, so triple the timeout. --- nbdime/tests/conftest.py | 4 ++-- nbdime/tests/test_cli_apps.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/nbdime/tests/conftest.py b/nbdime/tests/conftest.py index 6c291bcc..8bcf16b9 100644 --- a/nbdime/tests/conftest.py +++ b/nbdime/tests/conftest.py @@ -17,7 +17,7 @@ from jsonschema import Draft4Validator as Validator from jsonschema import RefResolver -from pytest import yield_fixture, fixture, skip +from pytest import fixture, skip import nbformat from .utils import call, have_git @@ -149,7 +149,7 @@ def git_repo2(tmpdir, request, filespath, needs_git): return repo -@yield_fixture +@fixture def reset_log(): # clear root logger handlers before test and reset afterwards handlers = list(logging.getLogger().handlers) diff --git a/nbdime/tests/test_cli_apps.py b/nbdime/tests/test_cli_apps.py index be0184db..f94d6dac 100644 --- a/nbdime/tests/test_cli_apps.py +++ b/nbdime/tests/test_cli_apps.py @@ -439,7 +439,7 @@ def _wait_up(url, interval=0.1, check=None): break -@pytest.mark.timeout(timeout=WEB_TEST_TIMEOUT) +@pytest.mark.timeout(timeout=3*WEB_TEST_TIMEOUT) def test_difftool(git_repo, request, unique_port): nbdime.gitdifftool.main(['config', '--enable']) cmd = get_output('git config --get --local difftool.nbdime.cmd').strip() @@ -478,7 +478,7 @@ def _term(): assert process.poll() == 0 -@pytest.mark.timeout(timeout=WEB_TEST_TIMEOUT) +@pytest.mark.timeout(timeout=3*WEB_TEST_TIMEOUT) def test_mergetool(git_repo, request, unique_port): nbdime.gitmergetool.main(['config', '--enable']) cmd = get_output('git config --get --local mergetool.nbdime.cmd').strip() From 99109dd6be456683a842d5fe85f99e3c608db079 Mon Sep 17 00:00:00 2001 From: Vidar Tonaas Fauske Date: Tue, 18 Apr 2017 14:44:27 +0200 Subject: [PATCH 5/5] Add and use test fixture to reset global Tests where the diff targets are set need to reset the state on teardown. Previously, only parts of the tests did this correctly. Now adds a fixture for it, and uses that where necessary. --- nbdime/tests/conftest.py | 10 +++ nbdime/tests/test_cli_apps.py | 4 +- nbdime/tests/test_git_diffdriver.py | 2 +- nbdime/tests/test_merge_notebooks.py | 92 +++++++++++++--------------- 4 files changed, 57 insertions(+), 51 deletions(-) diff --git a/nbdime/tests/conftest.py b/nbdime/tests/conftest.py index 8bcf16b9..45c475a7 100644 --- a/nbdime/tests/conftest.py +++ b/nbdime/tests/conftest.py @@ -23,6 +23,7 @@ from .utils import call, have_git from nbdime.webapp.nbdimeserver import init_app +from nbdime.diffing.notebooks import set_notebook_diff_targets pjoin = os.path.join @@ -323,3 +324,12 @@ def unique_port(): global _port _port += 1 return _port + + +@fixture() +def reset_diff_targets(): + try: + yield + finally: + # Reset diff targets (global variable) + set_notebook_diff_targets() diff --git a/nbdime/tests/test_cli_apps.py b/nbdime/tests/test_cli_apps.py index f94d6dac..b2c5b96f 100644 --- a/nbdime/tests/test_cli_apps.py +++ b/nbdime/tests/test_cli_apps.py @@ -106,7 +106,7 @@ def test_nbdiff_app_unicode_safe(filespath): check_call([sys.executable, '-m', 'nbdime.nbdiffapp', afn, bfn], env=env) -def test_nbdiff_app_only_source(filespath, tmpdir): +def test_nbdiff_app_only_source(filespath, tmpdir, reset_diff_targets): afn = os.path.join(filespath, "multilevel-test-base.ipynb") bfn = os.path.join(filespath, "multilevel-test-local.ipynb") dfn = os.path.join(tmpdir.dirname, "diff_output.json") @@ -122,7 +122,7 @@ def test_nbdiff_app_only_source(filespath, tmpdir): diff = diff[0]['diff'] -def test_nbdiff_app_ignore_source(filespath, tmpdir): +def test_nbdiff_app_ignore_source(filespath, tmpdir, reset_diff_targets): afn = os.path.join(filespath, "multilevel-test-base.ipynb") bfn = os.path.join(filespath, "multilevel-test-local.ipynb") dfn = os.path.join(tmpdir.dirname, "diff_output.json") diff --git a/nbdime/tests/test_git_diffdriver.py b/nbdime/tests/test_git_diffdriver.py index fd683793..1bea079c 100644 --- a/nbdime/tests/test_git_diffdriver.py +++ b/nbdime/tests/test_git_diffdriver.py @@ -83,7 +83,7 @@ def test_git_diff_driver(capsys, nocolor, needs_git): assert cap_out == expected_output.format(fn1, fn2, t1, t2) -def test_git_diff_driver_flags(capsys, nocolor, needs_git): +def test_git_diff_driver_flags(capsys, nocolor, needs_git, reset_diff_targets): # Simulate a call from `git diff` to check basic driver functionality test_dir = os.path.abspath(os.path.dirname(__file__)) diff --git a/nbdime/tests/test_merge_notebooks.py b/nbdime/tests/test_merge_notebooks.py index 9c6bd146..3be5cb99 100644 --- a/nbdime/tests/test_merge_notebooks.py +++ b/nbdime/tests/test_merge_notebooks.py @@ -187,7 +187,6 @@ def _check(partial, expected_partial, decisions, expected_conflicts): for e, d in zip(expected_conflicts, conflicts): # Only check keys specified in expectation value for k in sorted(e.keys()): - #if d[k] != e[k]: import ipdb; ipdb.set_trace() assert d[k] == e[k] @@ -892,64 +891,61 @@ def test_autoresolve_empty_strategies(): _check(partial, expected_partial, decisions, expected_conflicts) -class TestMergeDiffIgnores: +def test_only_sources(db, reset_diff_targets): + base = db["mixed-conflicts--1"] + local = db["mixed-conflicts--2"] + remote = db["mixed-conflicts--3"] + set_notebook_diff_targets(True, False, False, False) - def teardown(self): - set_notebook_diff_targets(True, True, True, True) + merge_args = copy.deepcopy(args) + merge_args.merge_strategy = "mergetool" - def test_only_sources(self, db): - base = db["mixed-conflicts--1"] - local = db["mixed-conflicts--2"] - remote = db["mixed-conflicts--3"] - set_notebook_diff_targets(True, False, False, False) + partial, decisions = merge_notebooks(base, local, remote, merge_args) - merge_args = copy.deepcopy(args) - merge_args.merge_strategy = "mergetool" + assert len(decisions) > 0 + for d in decisions: + path = d['common_path'] + # Still have some decisions on cell root, so avoid with len == 2 check + assert len(path) == 2 or path[2] == 'source' - partial, decisions = merge_notebooks(base, local, remote, merge_args) - assert len(decisions) > 0 - for d in decisions: - path = d['common_path'] - # Still have some decisions on cell root, so avoid with len == 2 check - assert len(path) == 2 or path[2] == 'source' +def test_only_outputs(db, reset_diff_targets): + base = db["mixed-conflicts--1"] + local = db["mixed-conflicts--2"] + remote = db["mixed-conflicts--3"] + set_notebook_diff_targets(False, True, False, False) - def test_only_outputs(self, db): - base = db["mixed-conflicts--1"] - local = db["mixed-conflicts--2"] - remote = db["mixed-conflicts--3"] - set_notebook_diff_targets(False, True, False, False) + merge_args = copy.deepcopy(args) + merge_args.merge_strategy = "mergetool" - merge_args = copy.deepcopy(args) - merge_args.merge_strategy = "mergetool" + partial, decisions = merge_notebooks(base, local, remote, merge_args) - partial, decisions = merge_notebooks(base, local, remote, merge_args) + assert len(decisions) > 0 + for d in decisions: + path = d['common_path'] + # Still have some decisions on cell root, so avoid with len == 2 check + assert len(path) == 2 or path[2] == 'outputs' - assert len(decisions) > 0 - for d in decisions: - path = d['common_path'] - # Still have some decisions on cell root, so avoid with len == 2 check - assert len(path) == 2 or path[2] == 'outputs' - def test_only_metadata(self, db): - base = db["mixed-conflicts--1"] - local = db["mixed-conflicts--2"] - remote = db["mixed-conflicts--3"] - set_notebook_diff_targets(False, False, False, True) +def test_only_metadata(db, reset_diff_targets): + base = db["mixed-conflicts--1"] + local = db["mixed-conflicts--2"] + remote = db["mixed-conflicts--3"] + set_notebook_diff_targets(False, False, False, True) - merge_args = copy.deepcopy(args) - merge_args.merge_strategy = "mergetool" + merge_args = copy.deepcopy(args) + merge_args.merge_strategy = "mergetool" - partial, decisions = merge_notebooks(base, local, remote, merge_args) + partial, decisions = merge_notebooks(base, local, remote, merge_args) - assert len(decisions) > 0 - for d in decisions: - path = d['common_path'] - # Still have some decisions on cell root, so avoid with len == 2 check - assert ( - len(path) == 2 or - path[0] == 'metadata' or - path[2] == 'metadata' or - path[4] == 'metadata' - ) + assert len(decisions) > 0 + for d in decisions: + path = d['common_path'] + # Still have some decisions on cell root, so avoid with len == 2 check + assert ( + len(path) == 2 or + path[0] == 'metadata' or + path[2] == 'metadata' or + path[4] == 'metadata' + )