Skip to content

Conversation

esclapes
Copy link

@esclapes esclapes commented Apr 26, 2017

This is WIP. Creating PR to get some feedback.

I am interested in using axios-vcr in my tests, but I use axios instances. I am proposing adding a second parameter to VCR.mountCassette with options, being the first one in this case instance

I am defaulting to require('axios') for backwards compatibility. As far as I can see, the change do not break current tests, but I have duplicated the main test file to test against my instance and found a few issues:

  • The interceptor is not skipping calls when file exists
  • That test is skipped at the moment because enabling it breaks things in a weird way. May be you know why.
  • How could we share the tests against both axios and and instance? I don't like the duplication that testing both options would bring. The current new test file is just a work in progress and I hope we can find a better way
  • I would also consider a global VCR configuration to avoid adding the options on every test.

I hope you can have a look at this in case I am missing something, specially with the failing (skipped) case.

EDIT: some typos

@esclapes
Copy link
Author

I see default parameters are not supported on travis, I will update the PR next week.

@nettofarah
Copy link
Owner

Hey, @esclapes.
Thank you for the PR.
I'm really excited to get this working 😃. It is a feature I've been wanting to add for a while now.

I'm pretty sure we can make this work, but I'm a little swamped this week.
I'll take a more in-depth look sometime during the weekend.

@nettofarah
Copy link
Owner

Hey, @esclapes.
Just thought of a possibly simpler idea:

You could explicitly require RequestMiddleware and ResponseMiddleware and attach them to your axios instance.

Here's a quick example:

function mountCustomCassette(axiosInstance, cassettePath) {
  const { RequestMiddleware, ResponseMiddleware } = require('axios-vrc')
  
  const resInterceptor = axiosInstance.interceptors.response.use(
    ResponseMiddleware.success(cassettePath),
    ResponseMiddleware.failure
  );
  
  const reqInterceptor = axiosIntance.interceptors.request.use(
    RequestMiddleware.success(cassettePath),
    RequestMiddleware.failure
  )

  return {
    resInterceptor,
    reqInterceptor
  }
}

function unmount(axiosIntance, interceptors) {
  axiosInstance.interceptors.response.eject(interceptors.responseInterceptor);
  axiosInstance.interceptors.request.eject(interceptors.requestInterceptor);
}

You could even introduce your own custom logic if you want to.

Hope this helps

@esclapes
Copy link
Author

esclapes commented May 1, 2017

I did contemplate that option, as the middleware are exposed, but I thought I made sense to offer an options on the main api.

I also think that allowing for an config object makes sense in order to keep it configurable: expiration, refresh and so on)

I still haven't dig into the non-passing test within this PR, I am quite surprised that it passes for the global axios, but not for the instance. And specially how the error is affecting the rest of the test suite. I will report back back if I make get to work on it.

Feel free to close this PR if you think that adding this feature is not in your short term plans.

@esclapes
Copy link
Author

esclapes commented May 1, 2017

And thanks for your help @nettofarah

@esclapes
Copy link
Author

esclapes commented May 1, 2017

Well, the cause of the test failing is just that the digest returns a different hash as I was using an extra custom header on my instance.

The cascading of errors was due to the fact that the cassete was not ejected properly after the first failed test, and that creates a race condition.

Probably the first issue could be solved by adding a new fixture or updating the digest function.

The second could be just a test fix, eject the cassete on promise rejection. However it is probably a good opportunity to review the expected behaviour when a cassete is mounted without removing a previous one, or whith multiple cassettes.

@nettofarah
Copy link
Owner

hey, @esclapes.
You might want to try your branch against #4 and see if your use-case is more reliable with those changes.

@esclapes esclapes closed this Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants