Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] setupPolly method for jest #116

Closed
gribnoysup opened this issue Oct 5, 2018 · 8 comments
Closed

[RFC] setupPolly method for jest #116

gribnoysup opened this issue Oct 5, 2018 · 8 comments
Labels
enhancement ✨ New feature or request 📦 core

Comments

@gribnoysup
Copy link
Contributor

gribnoysup commented Oct 5, 2018

Hi team! First of all, thanks a lot for this library, great work 👏

This issue is a proposal and request for comments on adding a setupPolly helper for jest environments. After having a discussion with @offirgolan in #115 (comment) I decided to investigate is it possible to make the same hook support with automated name generation for jest/jasmine environments as the one implemented for Mocha and QUnit in the core of this library.

There are two main differences that I spotted between those two mentioned above and jest/jasmine:

  • this is not commonly used in jest, so access to polly instance should be handled differently
  • there is no "normal" way to acces the current test name in hooks

Trying to address those issues led me to the solution that can be considered hacky, so instead of going with the PR right away, I created setup-polly-jest library as a proof of concept (but it works and covered with tests)

During my investigation I found out that you can get access to the full test name, but to do this, you'll need a return value from {test,it} call in the test suite.

The solution is to proxy jasmine.Env.it and jasmine.Env.dit methods to get the access to test case instance, get the full name and also hook into before and after hooks of the test case, overriding another jasmine method.

To address the this issue I decided on a little bit different approach, where setupPolly will return context with polly instance as a property (as a bonus, it will also have clearPolly method to disable polly at any moment in the test workflow)

You can find repository here: https://github.com/gribnoysup/setup-polly-jest and it is even published as @gribnoysup/setup-polly-jest so it should be easy for you to test how it works. Repo contains documentation, unit tests and an "integration" test suite that uses the library itself to setup polly in jest test environment

If you think that this approach is acceptable, I'll be glad to integrate this solution into @pollyjs/corepackage 👍

@offirgolan
Copy link
Collaborator

offirgolan commented Oct 5, 2018

@gribnoysup this is something that has been constantly on my mind so thank you for taking this on. The solution you currently have implemented feels really nice but I'm thinking we can get it closer to parity with a couple of changes:

Formatting Test Name

With your current implementation, the following test:

describe('Module', () => {
 it('Test', async () => {})
});

Will generate a recording name of Module-Test where the expected would be Module/Test. Mocha and Qunit do this so all test recordings are organized under their appropriate module. Would it be possible to build the full test name with / instead of -?

Scoping setupPolly to a module

To achieve even closer parity with Mocha and QUnit, would it be possible to scope setupPolly to a module?

describe('Module', () => {
 const context = setupPolly();

 it('Test', async () => {})
});

This can allow for something like:

describe('Module 1', () => {
  describe('Module a', () => {
    const context = setupPolly({ /* config a */ });

    it('Test 1a', async () => {})
  });

  describe('Module b', () => {
    const context = setupPolly({ /* config b */ });

    it('Test 1b', async () => {})
  });

  describe('Module c', () => {
    /* No Polly */

    it('Test 1c', async () => {})
  });
});

This one isn't all that important since you can do context.polly.configure in each module to configure polly for that specific module.

@gribnoysup
Copy link
Contributor Author

Hi @offirgolan! Thank you so much for the feedback 🙌

I did an investigation for both cases that you mentioned and the results are applied to the library and released for testing purposes as @gribnoysup/setup-polly-jest@0.3.0

Formatting Test Name

This one took a while 😅Basically it seems that jest/jasmine are hiding as much internals from the users of the library as possible, making it pretty hard to get the parent suites (modules) when you only have a spec. I found the solution but it requires traversing the tree of the "registered" suites to figure out who is the parent of the spec we are interested in, and then we need to do the same thing again for another spec. In general, I don't expect this operation to be slow enough to be even barely noticeable.

You can find commit with implementation here: gribnoysup/setup-polly-jest@dc1c73e

Scoping setupPolly to a module

I checked this one and it works as you expect it to work. My implementation uses afterAll hook to remove proxies from the {it,fit} methods and afterAll methods are "scoped" to the suites (modules). I added new test case to confirm that this behavior is there: gribnoysup/setup-polly-jest@5364030

@gribnoysup
Copy link
Contributor Author

I was doing some investigation for other ways to achieve behaviour parity between jest and mocha/qunit in an easier way and found jest-circus which seems to be new, work in progress test runner for jest (jestjs/jest#6295). That means that overriding jasmine environment will stop working with some future major release of jest 🙀

This information made me think that maybe this helper method can be called setupJasmine, to highlight that this solution is specific to jasmine environments, but will work for jest as long as jest-jasmine/runner is used. Upon further investigation, I noticed that jest-jasmine and the latest version of jasmine behave a little bit differently, which breaks scoping part of setupPolly behaviour

I have some ideas on how to solve this, but I guess I'll need some more time to figure out completely how to make this work in both environments 😅

@gribnoysup
Copy link
Contributor Author

gribnoysup commented Oct 13, 2018

I updated the repo and now everything is working both for jasmine and jest-jasmine and also matching Mocha/QUnit as close as possible: scoping, naming with folders, setupPolly method API 👍

@offirgolan if you are still okay with the approach, I'm still happy to port it to @pollyjs/core 😸

@offirgolan
Copy link
Collaborator

@gribnoysup apologies for the delayed response on this! Thank you again for answering all of my concerns and for taking the time to investigate the jest/jasmine issues further. Looking at the repo, I think this is well executed and would be a super useful addition to the library. Feel free to open up a PR into @pollyjs/core with this and I'll do whatever I can to help you get this in asap 😄

@gribnoysup
Copy link
Contributor Author

@offirgolan haha, no worries about the delay and thanks for the feedback 🙌I opened a work-in-progress PR for this feature, I think we can continue the discussion there 👍

@offirgolan offirgolan added enhancement ✨ New feature or request 📦 core labels Oct 21, 2018
@raghukiranp
Copy link

@gribnoysup Thanks for bringing this up, I am looking for support for JEST with polly.

@gribnoysup
Copy link
Contributor Author

After the discussion in PR, the decision was to keep the proposed implementation out of the Polly core. If someone is interested, setupPolly for jest/jasmine is published as a separate library: setup-polly-jest

Big thanks to the Polly team for support and feedback 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request 📦 core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants