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

Update build #5

Open
wwwlicious opened this issue Aug 13, 2018 · 17 comments
Open

Update build #5

wwwlicious opened this issue Aug 13, 2018 · 17 comments
Assignees
Labels
Milestone

Comments

@wwwlicious
Copy link
Owner

Update the build config

@wwwlicious wwwlicious added this to the 4.5.0 milestone Aug 13, 2018
@wwwlicious wwwlicious self-assigned this Aug 13, 2018
wwwlicious added a commit that referenced this issue Aug 14, 2018
@wwwlicious
Copy link
Owner Author

Still to update Appveyor config

@jezzsantos
Copy link
Contributor

Want a hand?

@wwwlicious
Copy link
Owner Author

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. 👍

@jezzsantos
Copy link
Contributor

jezzsantos commented Aug 14, 2018

OK, will have a look. It a.m. here in ole antipodea, so will let you know something before you wake up. 👍

@jezzsantos
Copy link
Contributor

jezzsantos commented Aug 14, 2018

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.

@jezzsantos
Copy link
Contributor

Hey Scott, sorry was not able to help out much today.
I think I will wait until you have things squared away to your own satisfaction first, before adding to it.
I am happy to contribute the integration tests for LimitRateAttribute if you need me to. Or perhaps you want to do those, so you get to understand how it works better? your call?

I am however, very keen to use this from nuget now that we have the attribute.
Can we soon get a new nuget release out there?

@wwwlicious
Copy link
Owner Author

@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.

@jezzsantos
Copy link
Contributor

jezzsantos commented Aug 15, 2018

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 [LimitRate] is more discoverable than [RateLimit]. However, it is just my opinion.

@wwwlicious
Copy link
Owner Author

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...

@jezzsantos
Copy link
Contributor

jezzsantos commented Aug 15, 2018

Granted, so lets apply the maintainer test then.
What about when you are looking at your DTO code in 6 weeks time?
You have several hundred of these API's to scan.
Which one says what it means - better than the other?

[RateLimit(1, PerSecond)]
public MyDtoResponse Get(MyDto request)
{
....
} 

OR

[LimitRate(1, PerSecond]
public MyDtoResponse Get(MyDto request)
{
...
}

@wwwlicious
Copy link
Owner Author

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.

@jezzsantos
Copy link
Contributor

Righto, @wwwlicious you go for it.

@jezzsantos
Copy link
Contributor

I am looking at repo right now. Did you put in those integration tests and placeholder in readme yet?

@wwwlicious
Copy link
Owner Author

Looks like there is a bug running the lua script with the version of redis-inside.
Will need to figure out if I can get v4 running some other way. Won't be tonight though.

wwwlicious added a commit that referenced this issue Aug 15, 2018
removing appsettings from AttributeLimitProvider
Renaming attribute LimitRate to RateLimit
Fixing Limits collection from multiple providers (would benefit from checking for any duplicates)
@jezzsantos
Copy link
Contributor

jezzsantos commented Aug 15, 2018

In the develop branch I see 5 broken tests

@jezzsantos
Copy link
Contributor

I'm confused @wwwlicious, where do you want me to go and add new tests?

@wwwlicious
Copy link
Owner Author

@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!

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

No branches or pull requests

2 participants