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

Acceptance test Rack::Attack#track #294

Merged
merged 3 commits into from
Mar 13, 2018
Merged

Conversation

grzuy
Copy link
Collaborator

@grzuy grzuy commented Mar 13, 2018

part of #293

@lmansur
Copy link
Contributor

lmansur commented Mar 13, 2018

I feel like both tests could be in the same file but within different describe blocks, since they are testing the behavior of the same method but with different options. What do you think?

@grzuy
Copy link
Collaborator Author

grzuy commented Mar 13, 2018

Despite being exposed to the user with the same public method (#track), i think they have enough differences in purpose that it becomes helpful to think of them in isolation. E.g. the block argument has a different interpretation in each case. In one, it's intended to flag when to track or not, in the other it returns the discriminator for the throttle being tracked.

I considered what you're proposing when i was writing these tests, but i preferred to err on the side of keeping them separate, so that both use cases can be thought of in isolation more easily.

Thank you for you feedback :-) 👍

@lmansur
Copy link
Contributor

lmansur commented Mar 13, 2018

That makes sense 👍

@grzuy grzuy merged commit 5e14b63 into rack:master Mar 13, 2018
@grzuy grzuy deleted the acceptance_test_track branch March 13, 2018 21:50
@grzuy grzuy mentioned this pull request Mar 14, 2018
19 tasks
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.

2 participants