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

Made pretender._handlerFor public #307

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

izelnakri
Copy link

@izelnakri izelnakri commented Sep 1, 2020

Reasoning:

Many complex test suites use this library to mock their backend server, and these tests and route handlers can get very complex. When we write e2e tests it is useful to override route handlers make server respond with error. However in such scenarios it would be also useful to test that user can fix their errors when server goes online/behaves correctly while staying on the same page with the error. Thus a developer would like to override the overridden route with historical route handling logic, instead of overriding with custom logic twice:

test('testing form submit error', function(assert) {
  // Saving previous handler to variable:
  let previousHandler = this.server.pretender.handlerFor('POST', '/users', () => {}).handler;
  
  // mocking the endpoint for error:
  this.server.post(`/users`, () => {
     assert.step('api-call-error');

     return new Response(500, {}, {});
   });

  // click()
   
  // assert error case

  // Override the overridden route with the initial handler, so no more extra mocking needed:
  this.server.post('/users', previousHandler);

  // click()

  // assert success case
});

This change encourages developers to use this pattern, which makes tests behave slightly more like their backend, if their backend is mocked correctly, and makes the test easier to debug since there are less versions of mocking of the same endpoint in the entire test suite.

Additional details:
  • In a separate commit, prettier config which matches the source code added to package.json.
  • In a separte commit, phantomjs removed since it isn't relevant anymore and breaks yarn install for contributing.

@izelnakri
Copy link
Author

izelnakri commented Dec 1, 2020

Hi @xg-wang , we need this functionality in a large codebase for various use-cases, so mirage can also make it public afterwards, let me know if you have further questions. Otherwise we would like this to be merged.

@xg-wang
Copy link
Member

xg-wang commented Dec 2, 2020

Hi @izelnakri, in your example, this.server.post(${notifierBaseURL}/v1/rules, previousHandler); is it supposed to be this.server.post(/users, previousHandler);?
Looks like you want to toggle the handler's behavior in a test by retrieving the handler for the route and verb combination, which you can do:

  1. const originalHandler = server.get(/* ... */);
  2. the handler access a variable that can be toggled in the test to change response.

Is the need here about not able to access the originally defined handler therefore you need to read it from within your test?

@izelnakri
Copy link
Author

yes 🤦 its is this.server.post('/users', previousHandler); again. Updated the Issue description.

@izelnakri
Copy link
Author

izelnakri commented Dec 2, 2020

Interesting, the calls return the handler? I'll try this out and get back to you! Thank you very much for your response/showing me the alternatives! Might be useful to document this in the README.md

@xg-wang
Copy link
Member

xg-wang commented Dec 2, 2020

@izelnakri no problem! Let me know if that works for you. I can update the README later

@izelnakri
Copy link
Author

izelnakri commented Dec 2, 2020

ok I just found one more issue with your proposed solutions:

1- This is interesting for few test cases I will try. Would be also nice to document it.

2- In a big application with many route handlers your second solution would mean exporting the route handler as ES module function export, currently routes can be registered without separating the handler, and its more readable this way. We already kind of put a symbolic reference by calling the http method and declaring a route string:

this.get('/users') so we should be able to lookup a registry by providing this info I think, thus we dont need to refactor all our route handlers to make them overridable with the same route handler default in our large server mock code

@izelnakri
Copy link
Author

izelnakri commented Dec 6, 2020

Hi @xg-wang , I hope the arguments I made above are convincing for this pull request to be merged, as it is just a removal of the _ in the method name and making it public. Without this small change in a large codebase, I had objections of using a private API, although this shouldnt be a private API based on my reasonings above, including your suggestions. Thus, for the benefit of everyone we should have this method not private.

There was a reason why this method was needed internally in the library in the first place, and very good reasons to be made public.

@xg-wang
Copy link
Member

xg-wang commented Dec 9, 2020

@izelnakri I think we need

I'm also not sure what is a public way to re-register a route handler for route-recognizer. That said, providing an easy way to retrieve the registered handler you've registered in pretender is fine. The current way to retrieve them from readme is not great const handler = server.handlers[0];

Sorry I do not currently have time for my own thoughts here, but wanted to share with you what I think is needed to make the API public.

To unblock yourself from the objections it looks like you can do it with public APIs

    let registry = pretender.hosts.forURL(url)[verb];
    let matches = registry.recognize(pretender.parseURL(url).fullpath);

    let match = matches ? matches[0] : null;

    return match;

@izelnakri
Copy link
Author

Awesome, Ill read the points in detail. Thank you for your detailed answer, and looking into this in depth 👍

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