miragejs / discuss

Ask questions, get help, and share what you've built with Mirage
MIT License
2 stars 0 forks source link

Mechanism for per-test request assertions #39

Open aquamme opened 8 years ago

aquamme commented 8 years ago

First off, thanks for the nice addon @samselikoff.

In acceptance tests, I oftentimes want to assert that my app made a request to the right endpoint with the right parameters. This can be accomplished by overriding request handlers in the test, like so:

test('Item should be hidden when the hide button is selected', function (assert) {
  let item = server.create('item');
  server.put('/items/:id', (schema, request) => {
    assert.equal(request.params.id, item.id);
    // perform the usual logic for this endpoint, then...
    return item;
  });

  // test
  visit(...);
});

This pattern works, but it entails duplicating whatever logic you may have in the request handler you overrode.

To solve this problem, I propose adding a hook that lets you examine the parameters passed to mirage before the request handler is invoked. Something like:

  server.beforePut('/items/:id', request => {
    assert.equal(request.items.id, expectedItemId);
  });

By default, the beforeVERB hooks would be NOOPs. Any return value would be discarded. All beforeVERB hooks would be reset to their default NOOP between tests.

I'm glad to do the work to add the feature, but wanted to discuss the idea and the potential API first.

samselikoff commented 8 years ago

Thanks for opening this, and you're welcome!

Lots of folks have asked about this so I'm happy to brainstorm. Reading through your comment made me think the ideal API really has nothing to do with redefining route handlers, but with assertions. Just thinking out loud, I wonder if there's a possibility to bring along some custom assertions with Mirage?

test('Item should be hidden when the hide button is selected', function(assert) {
  let item = server.create('item');

  visit(`items/${item.id}`);
  click('.hide-item');
  click('.save');

  andThen(() => {
    assert.server.received('PUT').toURL(`/items/${id}`).withParams({
      hidden: true
    });
  });
});

This would be pretty involved (lots to think through in the API here), but I know it'd be valuable.

FWIW if my endpoints are doing run-of-the-mill crud I often will just assert on Mirage's server state directly:

assert.equal(server.db.items.find(id).hidden, true);
aquamme commented 8 years ago

That custom assertion syntax is certainly aesthetically pleasing -- I'm a fan :smiley: I'll spend some time pondering and looking over how it might be used in the app I'm working on. A few questions that come to mind immediately:

How might we handle asserting that a request didn't happen?

assert.server.didntReceive('POST').toURL('/posts');
assert.server.did.not.receive('GET').toURL('/articles');

Should order matter?

assert.server.toURL('/posts').received('POST');

Which methods should be required, and which optional?

assert.server.received('PUT');

It did occur to me after posting that some assertions could simply be made against Mirage's DB. I wonder how that might fit into the best-practices picture if we were to add an assertion DSL.

samselikoff commented 8 years ago

I have a small note about it in the docs here. But there are some situations where it's just more pragmatic to assert the actual server call was made. I had an app that used Ember Data, but had a few non-RESTful calls (fire-and-forget, to initiate some backend task). I really just wanted to test that the UI was firing the AJAX call and forget about it, so I think the DSL would be useful.

Thinking out loud, it might be easier to store the state on the server object itself, and then do

assert.ok(server.received('PUT'));
assert.equal(server.receivedRequests, 2);

This frees us up from any qunit integrations, and also could be useful in dev. Perhaps by default, we enable this only in test env.

Then if you want to assert no server call was made,

assert.equal(server.receivedRequests, 0);

or for your more specific case,

assert.ok(server.didNotReceive('get').to('/posts/1');

Other nice thing about this is you can put all assertions at the end, if you have to queue up an async assertion sometimes you have to write it at the beginning of the test (making it less readable).

samselikoff commented 8 years ago

I think order should matter, I don't think server.to('/posts') would ever make sense. Perhaps we could add an api if we wanted to check for any verb. Some options:

server.received().to('/posts')
server.received('any').to('/posts')
server.receivedAny.to('/posts');
server.receivedRequest.to('/posts');
samselikoff commented 8 years ago

Thinking more about this, having the state on the server would be really nice for development especially if we add a server.lastRequest getter. Folks often want to see what went in/out during the last request, this would be really handy.

Also having all this state on server could eventually let us show it in the inspector. Liking this more and more.

aquamme commented 8 years ago

I dig it! I should have time to dive in this weekend.

samselikoff commented 4 years ago

FYI: Transferred this to our Discuss repo, our new home for more open-ended conversations about Mirage!

If things become more concrete + actionable we can create a tracking issue in the main repo.

samselikoff commented 4 years ago

There was also some work done on this feature idea: miragejs/ember-cli-mirage#882