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

e2e: add e2e for canary weight #257

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

Conversation

Charlie17Li
Copy link
Collaborator

Ⅰ. Describe what this PR did

add an e2e testcase for the canary weight pattern

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2023

Codecov Report

Merging #257 (74c7ec1) into main (09e563c) will decrease coverage by 0.13%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #257      +/-   ##
==========================================
- Coverage   44.58%   44.46%   -0.13%     
==========================================
  Files          32       32              
  Lines        5275     5290      +15     
==========================================
  Hits         2352     2352              
- Misses       2754     2769      +15     
  Partials      169      169              

see 1 file with indirect coverage changes

}

// MakeRequestAndCountExpectedResponse make 'totReq' requests and determine whether to test results according to 'fn' callback function
func MakeRequestAndCountExpectedResponse(t *testing.T, r roundtripper.RoundTripper, gwAddr string, expected Assertion, totReq int, fn func(int, int) bool) {
Copy link
Collaborator

@johnlanni johnlanni Mar 27, 2023

Choose a reason for hiding this comment

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

Suggested change
func MakeRequestAndCountExpectedResponse(t *testing.T, r roundtripper.RoundTripper, gwAddr string, expected Assertion, totReq int, fn func(int, int) bool) {
func MakeRequestAndCountExpectedResponse(t *testing.T, r roundtripper.RoundTripper, gwAddr string, expected Assertion, totReq, minSuccessCount int) {

Why not just use minSuccessCount instead of fn? Then the expected information of the test result would be more helpful, as follows: expected minSuccessCount is 40, actual count is 10

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But in some cases, we might want the values to be in a range. eg 40 < x < 60 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just a success Rate ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the total requests should be managed by test suite, and it is not exposed. I think the pkg user is more care about the success or fail rate ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. but if the expected rate is 50%, and there is always a slight deviation between the real rate and expected rate(50%). I am not sure how to control the deviation within a reasonable range, so I give control to the user

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe should have a accepted error interval to set up. Like if the accepted error interval is 5%, and expected rate is 50%, then we accept 45 to 55 %

@johnlanni
Copy link
Collaborator

@Charlie17Li Hi, the e2e test cases seems failed.

@johnlanni
Copy link
Collaborator

@Charlie17Li Is there any progress on this?

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