Repository: kevin1024/vcrpy

Issue:
Don't save requests from decorated tests if decorated test fails
As the title says: Don't save requests from decorated tests if decorated test fails

Does VCR already do this? If so awesome, and you guys should definitely say so in the README somewhere. Otherwise this seems like a solid feature. We have one test in particular where our 3rd party goes down more often than I'd care to admit, and if that bad response gets saved that will be quite aggravating.


Discussion:
neozenith: I’ll delegate review to @jairhenrique as they seem to be the most recent active contributor. 

Unfortunately I have other priorities come up and I’m no longer involved in this project sorry. 
jairhenrique: @Terseus please, update the docs with this new param.
Terseus: Of course, I'll look into it.

Also will check what's happening with the pipeline, it worked on my machine™. 
Terseus: Hi @jairhenrique

It turns out, it didn't worked in my machine after all :)
I had a bug in my setup and the tests for pypy3 were skipped.

After that found out that the fail was because a bug introduced by the `record_on_exception` implementation in Pypy, it should be fixed now.
Also I'd added a small section on how to use it in the "Advanced Features" page.
codecov-commenter: # [Codecov](https://codecov.io/gh/kevin1024/vcrpy/pull/668?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#668](https://codecov.io/gh/kevin1024/vcrpy/pull/668?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (d943244) into [master](https://codecov.io/gh/kevin1024/vcrpy/commit/60ac99c9079a96a6bb74a9168430e5df3fb439e2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (60ac99c) will **decrease** coverage by `0.29%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
- Coverage   89.80%   89.50%   -0.30%     
==========================================
  Files          26       26              
  Lines        1726     1734       +8     
  Branches      257      310      +53     
==========================================
+ Hits         1550     1552       +2     
- Misses        140      145       +5     
- Partials       36       37       +1     
```


| [Impacted Files](https://codecov.io/gh/kevin1024/vcrpy/pull/668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [vcr/cassette.py](https://codecov.io/gh/kevin1024/vcrpy/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dmNyL2Nhc3NldHRlLnB5) | `96.31% <100.00%> (+0.10%)` | :arrow_up: |
| [vcr/config.py](https://codecov.io/gh/kevin1024/vcrpy/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dmNyL2NvbmZpZy5weQ==) | `95.03% <100.00%> (+0.06%)` | :arrow_up: |
| [vcr/serializers/yamlserializer.py](https://codecov.io/gh/kevin1024/vcrpy/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dmNyL3NlcmlhbGl6ZXJzL3lhbWxzZXJpYWxpemVyLnB5) | `80.00% <0.00%> (-20.00%)` | :arrow_down: |
| [vcr/migration.py](https://codecov.io/gh/kevin1024/vcrpy/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dmNyL21pZ3JhdGlvbi5weQ==) | `73.46% <0.00%> (-3.07%)` | :arrow_down: |
| [vcr/stubs/httpx\_stubs.py](https://codecov.io/gh/kevin1024/vcrpy/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dmNyL3N0dWJzL2h0dHB4X3N0dWJzLnB5) | `95.87% <0.00%> (-2.07%)` | :arrow_down: |
| [vcr/stubs/\_\_init\_\_.py](https://codecov.io/gh/kevin1024/vcrpy/pull/668/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dmNyL3N0dWJzL19faW5pdF9fLnB5) | `93.06% <0.00%> (+0.57%)` | :arrow_up: |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

jairhenrique: @Terseus I fix the CI error. Can you rebase with master again!?
Terseus: @jairhenrique Sure, done.
jairhenrique: @Terseus thanks!

Patch:
diff --git a/docs/advanced.rst b/docs/advanced.rst
index f3eb68c..fb287fa 100644
--- a/docs/advanced.rst
+++ b/docs/advanced.rst
@@ -404,3 +404,25 @@ the Cassette ``allow_playback_repeats`` option.
         for x in range(10):
             response = urllib2.urlopen('http://www.zombo.com/').read()
         assert cass.all_played
+
+Discards Cassette on Errors
+---------------------------
+
+By default VCR will save the cassette file even when there is any error inside
+the enclosing context/test.
+
+If you want to save the cassette only when the test succeedes, set the Cassette
+``record_on_exception`` option to ``False``.
+
+.. code:: python
+
+    try:
+        my_vcr = VCR(record_on_exception=False)
+        with my_vcr.use_cassette('fixtures/vcr_cassettes/synopsis.yaml') as cass:
+            response = urllib2.urlopen('http://www.zombo.com/').read()
+            raise RuntimeError("Oops, something happened")
+    except RuntimeError:
+        pass
+
+    # Since there was an exception, the cassette file hasn't been created.
+    assert not os.path.exists('fixtures/vcr_cassettes/synopsis.yaml')
diff --git a/docs/conf.py b/docs/conf.py
index 94e6d8a..a3ffd5e 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -94,7 +94,7 @@ version = release = find_version("..", "vcr", "__init__.py")
 #
 # This is also used if you do content translation via gettext catalogs.
 # Usually you set "language" from the command line for these cases.
-language = None
+language = "en"
 
 # There are two options for replacing |today|: either, you set today to some
 # non-false value, then it is used:
diff --git a/vcr/cassette.py b/vcr/cassette.py
index 901b7fb..5822afa 100644
--- a/vcr/cassette.py
+++ b/vcr/cassette.py
@@ -45,7 +45,11 @@ class CassetteContextDecorator:
     this class as a context manager in ``__exit__``.
     """
 
-    _non_cassette_arguments = ("path_transformer", "func_path_generator")
+    _non_cassette_arguments = (
+        "path_transformer",
+        "func_path_generator",
+        "record_on_exception",
+    )
 
     @classmethod
     def from_args(cls, cassette_class, **kwargs):
@@ -55,6 +59,7 @@ class CassetteContextDecorator:
         self.cls = cls
         self._args_getter = args_getter
         self.__finish = None
+        self.__cassette = None
 
     def _patch_generator(self, cassette):
         with contextlib.ExitStack() as exit_stack:
@@ -64,9 +69,6 @@ class CassetteContextDecorator:
             log.debug(log_format.format(action="Entering", path=cassette._path))
             yield cassette
             log.debug(log_format.format(action="Exiting", path=cassette._path))
-            # TODO(@IvanMalison): Hmmm. it kind of feels like this should be
-            # somewhere else.
-            cassette._save()
 
     def __enter__(self):
         # This assertion is here to prevent the dangerous behavior
@@ -84,10 +86,22 @@ class CassetteContextDecorator:
         if other_kwargs.get("path_transformer"):
             transformer = other_kwargs["path_transformer"]
             cassette_kwargs["path"] = transformer(cassette_kwargs["path"])
-        self.__finish = self._patch_generator(self.cls.load(**cassette_kwargs))
+        self.__cassette = self.cls.load(**cassette_kwargs)
+        self.__finish = self._patch_generator(self.__cassette)
         return next(self.__finish)
 
-    def __exit__(self, *args):
+    def __exit__(self, *exc_info):
+        exception_was_raised = any(exc_info)
+        record_on_exception = self._args_getter().get("record_on_exception", True)
+        if record_on_exception or not exception_was_raised:
+            self.__cassette._save()
+            self.__cassette = None
+        # Fellow programmer, don't remove this `next`, if `self.__finish` is
+        # not consumed the unpatcher functions accumulated in the `exit_stack`
+        # object created in `_patch_generator` will not be called until
+        # `exit_stack` is not garbage collected.
+        # This works in CPython but not in Pypy, where the unpatchers will not
+        # be called until much later.
         next(self.__finish, None)
         self.__finish = None
 
diff --git a/vcr/config.py b/vcr/config.py
index 45412e3..a991c95 100644
--- a/vcr/config.py
+++ b/vcr/config.py
@@ -49,6 +49,7 @@ class VCR:
         cassette_library_dir=None,
         func_path_generator=None,
         decode_compressed_response=False,
+        record_on_exception=True,
     ):
         self.serializer = serializer
         self.match_on = match_on
@@ -80,6 +81,7 @@ class VCR:
         self.path_transformer = path_transformer
         self.func_path_generator = func_path_generator
         self.decode_compressed_response = decode_compressed_response
+        self.record_on_exception = record_on_exception
         self._custom_patches = tuple(custom_patches)
 
     def _get_serializer(self, serializer_name):
@@ -123,6 +125,7 @@ class VCR:
         func_path_generator = kwargs.get("func_path_generator", self.func_path_generator)
         cassette_library_dir = kwargs.get("cassette_library_dir", self.cassette_library_dir)
         additional_matchers = kwargs.get("additional_matchers", ())
+        record_on_exception = kwargs.get("record_on_exception", self.record_on_exception)
 
         if cassette_library_dir:
 
@@ -149,6 +152,7 @@ class VCR:
             "path_transformer": path_transformer,
             "func_path_generator": func_path_generator,
             "allow_playback_repeats": kwargs.get("allow_playback_repeats", False),
+            "record_on_exception": record_on_exception,
         }
         path = kwargs.get("path")
         if path:


Test patch:
diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py
index c6da8e8..013f849 100644
--- a/tests/integration/test_config.py
+++ b/tests/integration/test_config.py
@@ -60,3 +60,23 @@ def test_missing_matcher():
     with pytest.raises(KeyError):
         with my_vcr.use_cassette("test.yaml", match_on=["notawesome"]):
             pass
+
+
+def test_dont_record_on_exception(tmpdir):
+    my_vcr = vcr.VCR(record_on_exception=False)
+
+    @my_vcr.use_cassette(str(tmpdir.join("dontsave.yml")))
+    def some_test():
+        assert b"Not in content" in urlopen("http://httpbin.org/get")
+
+    with pytest.raises(AssertionError):
+        some_test()
+
+    assert not os.path.exists(str(tmpdir.join("dontsave.yml")))
+
+    # Make sure context decorator has the same behavior
+    with pytest.raises(AssertionError):
+        with my_vcr.use_cassette(str(tmpdir.join("dontsave2.yml"))):
+            assert b"Not in content" in urlopen("http://httpbin.org/get").read()
+
+    assert not os.path.exists(str(tmpdir.join("dontsave2.yml")))