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

test: simply the expected output for some curl test cases #16572

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Sep 11, 2023

Link to #16454

Random spaces may be inserted between fields in a marshalled json response in grpc-gateway v2, so simply the expected output to make it work for both v1 and v2.

@serathius
Copy link
Member

serathius commented Sep 11, 2023

This PR lowers quality of testing by skipping testing some fields, which we are no longer able to cover using a naive "find matching line" validation.

Could we just use json parsing for JSON API? For example create something like EtcdctlV3 that has spawnJsonCommand to parse JSON responses.

As we add more tests, the more complicated they are, and the more abstraction leak we get. Abstraction leak happens when abstractions you created doesn't correctly reflect the modeled problem. Like in those tests, we are using "find matching line" logic to analyse JSON responses, it doesn't work as JSON can have arbitrary whitespaces injected.

Until we fix our abstraction we will continue to need to patch our tests like in this PR.

Signed-off-by: Benjamin Wang <wachao@vmware.com>
@ahrtr
Copy link
Member Author

ahrtr commented Sep 11, 2023

This PR lowers quality of testing by skipping testing some fields, which we are no longer able to cover using a naive "find matching line" validation.

  • Partially agree with this. Updated the verification on the response for /v3/kv/txn.
  • For other verification, we only need to verify what the etcdserver returns. Use "/v3/election/resign" as an example, etcdserver just returns an ErrMissingLeaderKey, but gRPC adds some other info such as "code": 2. We only need to verify what the etceserver returns.
  • Note that we are NOT testing the full functionalities. We just need to ensure the RESTful APIs are not broken when we upgrade grpc-gateway from v1 to v2.

@ahrtr
Copy link
Member Author

ahrtr commented Sep 11, 2023

Until we fix our abstraction we will continue to need to patch our tests like in this PR.

Pls do not make it big on this PR. If you have any additional thoughts, please raise a separate ticket to discuss; and anyone is welcome to refactor or improve existing test cases.

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Agree it would be best to more formally parse the json responses.

With that said, provided we have a follow-up issue created to track the work I am comfortable for it to be addressed at a later date.

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@ahrtr
Copy link
Member Author

ahrtr commented Sep 12, 2023

thx all for the review!

@ahrtr ahrtr merged commit cc282a8 into etcd-io:main Sep 12, 2023
27 checks passed
@ahrtr ahrtr deleted the fix_curl_test_20230911 branch June 12, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants