-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update build #5
Comments
Still to update Appveyor config |
Want a hand? |
Well I'm done for the night but you should see that the Feature tests now have an authenticated and unauthenticated client. Rather than mocking everything, the goal is to create real services/dto's using both LimitProviders (I've combined them so that both can be used but the redis script key should prob be refactoring out into a single instance away from limit providers.) Then testing is just sending dto's via the clients to verify everything works and limits are hit, status codes returned as expected etc. I'll pick it up tomorrow night but feel free to checkout dev and work on the above if you want to. 👍 |
OK, will have a look. It a.m. here in ole antipodea, so will let you know something before you wake up. 👍 |
Ideally, that pattern you would want to setup, is having a base test class that has all the tests in it that verifies that any LimitProvider works the way you (the interface designer) says it should work. That way, a new LimitProvider can provide its own test class that inherits, and does specifically what it needs to provide the limits for the tests. |
Hey Scott, sorry was not able to help out much today. I am however, very keen to use this from nuget now that we have the attribute. |
@jezzsantos I'll put in an example of a attribute based integration test and leave you to flesh out more test cases. I'll also update the readme for config and leave a placeholder for describing attribute usage if you want a crask at that too. btw, The plugin is called 'RateLimit', any objection to renaming the attribute to match so there is consistency when typing so both RateLimit and LimitRate are not both used? Will release on nuget once the tests and docs have been updated and api has settled. |
Sweet, Ill have a crack today. Thanks. As far as the name goes, if you look at it from the consumers point fo view. I.e the person who just installed the Plugin and now wants to apply the attribute to their DTO's. They will be applying it to "limit the rate, of their API call (rather than rate the limit :) ). So I feel that |
I disagree on the name. If I've just installed a plugin called RateLimit, with RateLimitFeature and RateLimitHeaders etc, I would find RateLimitAttribute more discoverable because of it's consistency... |
Granted, so lets apply the maintainer test then.
OR
|
When I am RateLimiting my API, I think in terms of it's RateLimit. not it's Limit Rate so to me that makes the most sense. |
Righto, @wwwlicious you go for it. |
I am looking at repo right now. Did you put in those integration tests and placeholder in readme yet? |
Looks like there is a bug running the lua script with the version of redis-inside. |
removing appsettings from AttributeLimitProvider Renaming attribute LimitRate to RateLimit Fixing Limits collection from multiple providers (would benefit from checking for any duplicates)
In the |
I'm confused @wwwlicious, where do you want me to go and add new tests? |
@jezzsantos Broken tests will be rewritten to use client but the lua script will need to fix the lua script/redis embedded instance first. Not gotten to the other bits yet which is why you can't find them... haha! |
Update the build config
The text was updated successfully, but these errors were encountered: