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

Add request assertion feature to server #882

Closed
wants to merge 7 commits into from

Conversation

aquamme
Copy link

@aquamme aquamme commented Sep 12, 2016

Implements #845

The server tracks all handled requests and provides an API to allow
users to check whether a request matching the given parameters has
been made since the server was started.

  server.received.get.to('/contacts', { name: 'Darth Vader' })
  server.didNotReceive.post.to('any');
  server.received.get.to(/\/secrets.*/);
  server.didNotReceive.request.to('/robot/5');
  server.received.put.with({ name: 'Jack' });

I ended up adding RegExp support mostly because it was simpler than adding support for paths like "/posts/:id".

There is also a getter for the last request made to the server:

  server.lastRequest

Comments and suggestions are welcome!

@@ -0,0 +1,90 @@
/* * RequestAssertion implements a simple DSL for asserting that specific requests were made.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if RequestAssertion is the best name since 'assertion' is a loaded term. Maybe something like RequestChecker?

@aquamme
Copy link
Author

aquamme commented Sep 13, 2016

A few other things I'd like to do/consider/I missed:

  • Add an option to cap the number of requests that the server holds on to
  • Add support for comparing POST payloads with nested attribute Done
  • Have the feature report the number of matching requests instead of a boolean -- might want to rework the DSL a bit if this idea is pursued -- server.received.post.to('/posts') doesn't look like it would return a number

Aaron Quamme added 2 commits September 13, 2016 21:46
The server tracks all handled requests and provides an API to allow
users to check whether a request matching the given parameters has
been made since the server was started.

```javascript
  server.received.get.to('/contacts', { name: 'Darth Vader' })
  server.didNotReceive.post.to('any');
  server.received.get.to(/\/secrets.*/);
  server.didNotReceive.request.to('/robot/5');
```

There is also a getter for the last request made to the server:
```javascript
  server.lastRequest
```
@samselikoff
Copy link
Collaborator

Sorry about delay, hoping to get to this soon.

Just glancing over, will folks want to assert against content type or other things? Should we

assert.ok(this.server.received.post.to('/contacts', {
  with: {
    body: { name: 'Bilbo', age: 111 },
    headers: {
      'ContentType': 'foo'
    }
  })
);

Don't have to change right now, just a quick thought.

@aquamme
Copy link
Author

aquamme commented Sep 29, 2016

I like the idea. I'm on vacation until the 4th, so I won't be able to update the PR until then. Glad to hear any other feedback you may have when you get to it. Thanks!

@samselikoff
Copy link
Collaborator

Few suggestions:

  • Make _requests public: server.requests. This way folks can inspect during dev, or even use w/their assertions (assert.equal(server.requests.length, 3);)
  • Move server.lastRequest to server.requests.last (I think you can define a property on the array)
  • I think the DSL currently doesn't allow server.received.post, right? You have to specify a url? ... thinking about this, I think it's okay.
  • Originally I said enable only in testing by default, but I think we should enable in all envs by default, and maybe add an option to disable if memory gets out of hand. (We can do this in a new PR).

@aquamme
Copy link
Author

aquamme commented Oct 11, 2016

Great feedback -- I should have time to make those changes tomorrow evening.

Aaron Quamme added 2 commits October 12, 2016 19:36
* server.received... returns the number of matching requests
  server.didNotReceive returns a boolean
* Arbitrary FakeXHR attributes can be passed
* Removed 'with' alias for 'to'
* Make server.requests a public attribute
* Removed special behavior when 'any' is passed to 'to'
@aquamme
Copy link
Author

aquamme commented Oct 16, 2016

All right, this is ready for another peek.

server.received... returns the number of matching requests, while server.didNotReceive returns a boolean. So you can do things like

// Two GETs are made to '/contacts', then...
assert.equal(server.received.get.to('/contacts'), 2);
assert.ok(server.received.get.to('/contacts'));
assert.ok(server.didNotReceive.post.to('/contacts'));

You can assert against any FakeXHR attributes:

assert.ok(server.received.get.to('/contacts', { requestHeaders: { 'Foo': 'Bar' } }));

Also made requests a public property and added a last property.

I think the DSL currently doesn't allow server.received.post, right? You have to specify a url? ... thinking about this, I think it's okay.

I don't see how we could make that syntax work while still allowing further chaining with 'to'. That aside, I did have a mechanism for allowing any URLs but I removed it because I didn't like the API (server.received.request.to('any')), but it could easily be added back in some form.

@aquamme
Copy link
Author

aquamme commented Nov 9, 2016

@samselikoff -- any further changes you'd like to see on this? I can write up some documentation once you give it the 👍

@samselikoff
Copy link
Collaborator

Closing this as stale, but keeping a reference to it in miragejs/miragejs#217 in case we want to reference these APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants