Repository: prophile/jacquard

Issue:
`jacquard load-experiment` erases `concluded` dates

Discussion:

Patch:
diff --git a/jacquard/cli.py b/jacquard/cli.py
index 263ae91..a2e102e 100644
--- a/jacquard/cli.py
+++ b/jacquard/cli.py
@@ -115,7 +115,7 @@ def main(args=sys.argv[1:], config=None):
             options.func(config, options)
         except CommandError as exc:
             (message,) = exc.args
-            sys.stderr.write("%s\n", message)
+            print(message, file=sys.stderr)
             exit(1)
 
 
diff --git a/jacquard/experiments/commands.py b/jacquard/experiments/commands.py
index d93fee3..dcdf99d 100644
--- a/jacquard/experiments/commands.py
+++ b/jacquard/experiments/commands.py
@@ -91,6 +91,7 @@ class Conclude(BaseCommand):
             experiment = Experiment.from_store(store, options.experiment)
 
             current_experiments = store.get('active-experiments', [])
+            concluded_experiments = store.get('concluded-experiments', [])
 
             if options.experiment not in current_experiments:
                 raise CommandError(
@@ -98,6 +99,7 @@ class Conclude(BaseCommand):
                 )
 
             current_experiments.remove(options.experiment)
+            concluded_experiments.append(options.experiment)
 
             close(
                 store,
@@ -118,6 +120,7 @@ class Conclude(BaseCommand):
             experiment.save(store)
 
             store['active-experiments'] = current_experiments
+            store['concluded-experiments'] = concluded_experiments
 
 
 class Load(BaseCommand):
@@ -143,7 +146,7 @@ class Load(BaseCommand):
         parser.add_argument(
             '--skip-launched',
             action='store_true',
-            help="do not error on launched experiments",
+            help="do not load or error on launched experiments",
         )
 
     @retrying
@@ -151,6 +154,7 @@ class Load(BaseCommand):
         """Run command."""
         with config.storage.transaction() as store:
             live_experiments = store.get('active-experiments', ())
+            concluded_experiments = store.get('concluded-experiments', ())
 
             for file in options.files:
                 definition = yaml.safe_load(file)
@@ -167,6 +171,16 @@ class Load(BaseCommand):
                             experiment.id,
                         )
 
+                elif experiment.id in concluded_experiments:
+                    if options.skip_launched:
+                        continue
+
+                    else:
+                        raise CommandError(
+                            "Experiment %r has concluded, refusing to edit" %
+                            experiment.id,
+                        )
+
                 experiment.save(store)
 
 


Test patch:
diff --git a/jacquard/experiments/tests/test_smoke.py b/jacquard/experiments/tests/test_smoke.py
index 2b10c85..4839686 100644
--- a/jacquard/experiments/tests/test_smoke.py
+++ b/jacquard/experiments/tests/test_smoke.py
@@ -1,6 +1,10 @@
+import io
+import copy
 import datetime
-from unittest.mock import Mock
+import contextlib
+from unittest.mock import Mock, patch
 
+import pytest
 import dateutil.tz
 
 from jacquard.cli import main
@@ -46,6 +50,7 @@ def test_conclude_no_branch():
 
     assert 'concluded' in config.storage['experiments/foo']
     assert 'foo' not in config.storage['active-experiments']
+    assert 'foo' in config.storage['concluded-experiments']
     assert not config.storage['defaults']
 
 
@@ -57,4 +62,107 @@ def test_conclude_updates_defaults():
 
     assert 'concluded' in config.storage['experiments/foo']
     assert 'foo' not in config.storage['active-experiments']
+    assert 'foo' in config.storage['concluded-experiments']
     assert config.storage['defaults'] == BRANCH_SETTINGS
+
+
+def test_load_after_launch_errors():
+    config = Mock()
+    config.storage = DummyStore('', data=DUMMY_DATA_POST_LAUNCH)
+
+    experiment_data = {'id': 'foo'}
+    experiment_data.update(DUMMY_DATA_PRE_LAUNCH['experiments/foo'])
+
+    stderr = io.StringIO()
+    with contextlib.redirect_stderr(stderr), pytest.raises(SystemExit):
+        with patch(
+            'jacquard.experiments.commands.yaml.safe_load',
+            return_value=experiment_data,
+        ), patch(
+            'jacquard.experiments.commands.argparse.FileType',
+            return_value=str,
+        ):
+            main(('load-experiment', 'foo.yaml'), config=config)
+
+    stderr_content = stderr.getvalue()
+    assert "Experiment 'foo' is live, refusing to edit" in stderr_content
+
+    fresh_data = DummyStore('', data=DUMMY_DATA_POST_LAUNCH)
+    assert fresh_data.data == config.storage.data, "Data should be unchanged"
+
+
+def test_load_after_launch_with_skip_launched():
+    config = Mock()
+    config.storage = DummyStore('', data=DUMMY_DATA_POST_LAUNCH)
+
+    experiment_data = {'id': 'foo'}
+    experiment_data.update(DUMMY_DATA_PRE_LAUNCH['experiments/foo'])
+
+    stderr = io.StringIO()
+    with contextlib.redirect_stderr(stderr), patch(
+        'jacquard.experiments.commands.yaml.safe_load',
+        return_value=experiment_data,
+    ), patch(
+        'jacquard.experiments.commands.argparse.FileType',
+        return_value=str,
+    ):
+        main(('load-experiment', '--skip-launched', 'foo.yaml'), config=config)
+
+    fresh_data = DummyStore('', data=DUMMY_DATA_POST_LAUNCH)
+    assert fresh_data.data == config.storage.data, "Data should be unchanged"
+
+    stderr_content = stderr.getvalue()
+    assert '' == stderr_content
+
+
+def test_load_after_conclude_errors():
+    config = Mock()
+    config.storage = DummyStore('', data=DUMMY_DATA_POST_LAUNCH)
+
+    main(('conclude', 'foo', 'bar'), config=config)
+    original_data = copy.deepcopy(config.storage.data)
+
+    experiment_data = {'id': 'foo'}
+    experiment_data.update(DUMMY_DATA_PRE_LAUNCH['experiments/foo'])
+
+    stderr = io.StringIO()
+    with contextlib.redirect_stderr(stderr), pytest.raises(SystemExit):
+        with patch(
+            'jacquard.experiments.commands.yaml.safe_load',
+            return_value=experiment_data,
+        ), patch(
+            'jacquard.experiments.commands.argparse.FileType',
+            return_value=str,
+        ):
+            main(('load-experiment', 'foo.yaml'), config=config)
+
+    assert original_data == config.storage.data, "Data should be unchanged"
+
+    stderr_content = stderr.getvalue()
+    assert "Experiment 'foo' has concluded, refusing to edit" in stderr_content
+
+
+def test_load_after_conclude_with_skip_launched():
+    config = Mock()
+    config.storage = DummyStore('', data=DUMMY_DATA_POST_LAUNCH)
+
+    main(('conclude', 'foo', 'bar'), config=config)
+    original_data = copy.deepcopy(config.storage.data)
+
+    experiment_data = {'id': 'foo'}
+    experiment_data.update(DUMMY_DATA_PRE_LAUNCH['experiments/foo'])
+
+    stderr = io.StringIO()
+    with contextlib.redirect_stderr(stderr), patch(
+        'jacquard.experiments.commands.yaml.safe_load',
+        return_value=experiment_data,
+    ), patch(
+        'jacquard.experiments.commands.argparse.FileType',
+        return_value=str,
+    ):
+        main(('load-experiment', '--skip-launched', 'foo.yaml'), config=config)
+
+    assert original_data == config.storage.data, "Data should be unchanged"
+
+    stderr_content = stderr.getvalue()
+    assert '' == stderr_content
diff --git a/jacquard/tests/test_cli.py b/jacquard/tests/test_cli.py
index bae650f..bf32262 100644
--- a/jacquard/tests/test_cli.py
+++ b/jacquard/tests/test_cli.py
@@ -3,7 +3,9 @@ import textwrap
 import contextlib
 import unittest.mock
 
-from jacquard.cli import main
+import pytest
+
+from jacquard.cli import CommandError, main
 from jacquard.storage.dummy import DummyStore
 
 
@@ -59,3 +61,28 @@ def test_run_write_command():
     assert output.getvalue() == ''
 
     assert config.storage.data == {'defaults': '{"foo": "bar"}'}
+
+
+def test_erroring_command():
+    config = unittest.mock.Mock()
+
+    ERROR_MESSAGE = "MOCK ERROR: Something went wrong in the"
+
+    mock_parser = unittest.mock.Mock()
+    mock_options = unittest.mock.Mock()
+    mock_options.func = unittest.mock.Mock(
+        side_effect=CommandError(ERROR_MESSAGE),
+    )
+    mock_parser.parse_args = unittest.mock.Mock(
+        return_value=mock_options,
+    )
+
+    stderr = io.StringIO()
+    with contextlib.redirect_stderr(stderr), pytest.raises(SystemExit):
+        with unittest.mock.patch(
+            'jacquard.cli.argument_parser',
+            return_value=mock_parser,
+        ):
+            main(['command'], config=config)
+
+    assert stderr.getvalue() == ERROR_MESSAGE + "\n"