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

Unit test fixup #7

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

Unit test fixup #7

wants to merge 7 commits into from

Conversation

dougluce
Copy link

I've redone the tests in Mocha with Supertest as a harness for Express. I think I've addressed a couple of problems that the tests had. I hope this may be of some benefit.

dougluce and others added 7 commits October 28, 2014 13:17
This message was given when rate limiting is hit:

```
express deprecated res.json(status, obj): Use res.status(status).json(obj) instead node_modules/express-rate/lib/rate.js:61:17
```

The deprecation is mentioned in https://github.com/strongloop/express/releases/tag/4.7.0

Also update the docs and example.
First set of tests cast under Mocha.  Simplified them a bit further
with async.  Did a little refactoring.  Hopefully it's better.
Check Redis directly to see whether the key has expired.  This should
avoid adding fudge factors to the test duration, although fudge is
still necessary to compensate for Redis' 1-second expiration
resolution.

Also expire the proper key out of the store.  When the middleware is
being used, the route key has the remotekey added to it.  Since that's
not strictly known until a request is made, I've put in the localhost
IP address as that's what express is giving me for a remote address on
my box.  No clue if that holds up universally.

Also tried to make the logic a little cleaner, although it's still far
from perfect.
I'm using a single handler for all these tests. This introduces
potential for one test to affect other tests (i.e. it's not very
"unit-y"). I think the gain in showing resiliancy during multiple uses
is worth the downside.

Removed the timeout from the Redis test as the interval is now a
second and it really should be able to end in under 2.
So that tests will run.  NPM also reformatted this thing, looks a
little more standard now.
So the common stuff doesn't look like it has exceptions just for
Redis.
@dougluce
Copy link
Author

@ivolo need any help with managing issues or pull requests?

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.

1 participant