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

Router check tool: Add testing support for routes with runtime numerator 0 #9193

Merged
merged 5 commits into from
Dec 5, 2019

Conversation

LisaLudique
Copy link
Contributor

@LisaLudique LisaLudique commented Dec 2, 2019

Description: Prior to this change, it was not possible to test routes with runtime fraction 0/denom, as a route is only matched when the random_value field is less than the default_value fraction. This change sets runtime fraction numerators to 1 (arbitrary nonzero value) so that those routes can be tested. #730
Risk Level: Low
Testing: added unit test in route_tests.sh to show coverage
Docs Changes: included in router_check.rst
Release Notes: included

Lisa Lu added 2 commits December 2, 2019 15:48
Signed-off-by: Lisa Lu <lisalu@lyft.com>
Signed-off-by: Lisa Lu <lisalu@lyft.com>
Signed-off-by: Lisa Lu <lisalu@lyft.com>
@jyotimahapatra
Copy link
Contributor

jyotimahapatra commented Dec 4, 2019

I was wondering if we could send a runtime override setting from the tool input to override the numerator, but that will be more complex and make the input params much larger(in case we have multiple runtimes). The simpler approach LGTM.

Lets add some more test cases that i mentioned in the other comment and i think we should be fine.

Signed-off-by: Lisa Lu <lisalu@lyft.com>
jyotimahapatra
jyotimahapatra previously approved these changes Dec 5, 2019
dio
dio previously approved these changes Dec 5, 2019
Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks

@dio
Copy link
Member

dio commented Dec 5, 2019

LGTM. Hum, please merge master and resolve conflict. Thanks!

@LisaLudique LisaLudique dismissed stale reviews from dio and jyotimahapatra via 0ca2723 December 5, 2019 18:12
Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thank you

@mattklein123 mattklein123 merged commit 3fa6069 into envoyproxy:master Dec 5, 2019
@LisaLudique LisaLudique deleted the MESH-303 branch December 6, 2019 17:48
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.

4 participants