parroty / exvcr

HTTP request/response recording library for elixir, inspired by VCR.
MIT License
720 stars 131 forks source link

Fix unstable behavior without `async: false` #126

Closed BKStephens closed 6 years ago

BKStephens commented 6 years ago

Since Meck mocks globally, I added a process/module (MockLock) that controls access to this resource. Now, before ExVCR.Mock.mock_methods calls :meck.expect, it request a lock from MockLock. When the lock becomes available, MockLock sends :lock_granted to the calling process. When the calling process is done, it calls ExVCR.MockLock.release_lock(). Right before MockLock sends :lock_granted to calling process, it starts to monitor the process. If the calling process goes down, MockLock will be notified and release the lock.

I also had to make ExVCR.Setting work with async. In order to achieve this, I used the same pattern for naming the ets table as in ExVCR.Adapter.Hackney.Store, which is to include the pid in the ets table name. This does introduce a breaking change: calling ExVCR.Config functions outside of test process, such as in setup_all, will not work. Instead, ExVCR.Config should be called in setup or test. I have documented this in the README Notes section.

Another change is that I removed the clear_mock option. Now, :meck.unload is called for each test every time. This is necessary for MockLock/async to work correctly.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.4%) to 93.684% when pulling 489826bfd2d3bfcd1d132314730040c3e0293d56 on BKStephens:fix_unstable_async into 4fdd8b06af1c27c4d7fd7333bf50bd48bd188a9e on parroty:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.4%) to 93.684% when pulling 489826bfd2d3bfcd1d132314730040c3e0293d56 on BKStephens:fix_unstable_async into 4fdd8b06af1c27c4d7fd7333bf50bd48bd188a9e on parroty:master.

bgeihsgt commented 6 years ago

@parroty have you had a chance to look at this? I'm also interested in having this for my team.

parroty commented 6 years ago

Thanks for the extensive implementation! I'll be pushing.

andrewtimberlake commented 6 years ago

Is it possible this broke how ExVCR.Config.cassette_library_dir/1 works? After upgrading all my tests just stopped working and I had new cassettes created in the default path. I reverted to 0.9.1 and all my tests work again.

BKStephens commented 6 years ago

@andrewtimberlake This change did bring about a breaking change that calling ExVCR.Config functions from outside the test process will not work (such as within setup_all).

You should still be able to call ExVCR.Config.cassette_library_dir/1 from a setup block or configure it in config/test.exs. Are you calling calling ExVCR.Config.cassette_library_dir/1 from within a setup_all? If so, try moving it to setup.

nihalgonsalves commented 6 years ago

@parroty I don't think that this was a suitable change for a minor version bump. The readme prior to this PR had multiple examples that used setup_all. Deprecating that, like @BKStephens noted originally in this thread, is a breaking change.

0.10.0 also seems to have caused some timeout issues that I haven't had time to investigate, so I have been forced to downgrade manually to 0.9.1 for now as well.

andrewtimberlake commented 6 years ago

I did have the config in setup_all as @nihalgonsalves mentioned that was in the docs. When I get a chance I’ll move to setup and update again. Thanks