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

Add client version verification test. #3032

Merged
merged 4 commits into from
Oct 13, 2021

Conversation

CPWstatic
Copy link
Contributor

@CPWstatic CPWstatic commented Oct 11, 2021

@CPWstatic CPWstatic added ready-for-testing PR: ready for the CI test cherry-pick-v2.6 PR: need cherry-pick to this version labels Oct 11, 2021
@CPWstatic CPWstatic force-pushed the add_verify_client_test branch 2 times, most recently from 0b30f92 to 9ce8711 Compare October 12, 2021 07:47
graph_spaces["resp"] = conn.verifyClientVersion(req)
conn._iprot.trans.close()

@then(parse("the connection should be rejected"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should specify the message since other errors would also cause the connection to be rejected? We could parse something like Connection rejected because of client version mismtach

Comment on lines +562 to +565
@then(parse("the connection should be rejected"))
def check_client_compatible(graph_spaces):
resp = graph_spaces["resp"]
assert resp.error_code == ErrorCode.E_CLIENT_SERVER_INCOMPATIBLE, f'The client was not rejected by server: {resp}'
Copy link
Contributor

Choose a reason for hiding this comment

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

The client might be rejected for other errors. f'Expected error: ErrorCode.E_CLIENT_SERVER_INCOMPATIBLE, got : {resp.error_code}'

Copy link
Contributor

@Aiee Aiee left a comment

Choose a reason for hiding this comment

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

Generally LGTM.

@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Oct 12, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #3032 (5e28520) into master (d8e5539) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3032      +/-   ##
==========================================
- Coverage   84.20%   84.19%   -0.01%     
==========================================
  Files        1287     1287              
  Lines      115319   115319              
==========================================
- Hits        97100    97098       -2     
- Misses      18219    18221       +2     
Impacted Files Coverage Δ
src/meta/processors/parts/GetSpaceProcessor.cpp 60.60% <0.00%> (-18.19%) ⬇️
src/meta/processors/BaseProcessor.h 76.47% <0.00%> (-14.71%) ⬇️
src/graph/planner/Planner.cpp 75.00% <0.00%> (-5.00%) ⬇️
src/common/thread/GenericWorker.h 92.30% <0.00%> (-3.85%) ⬇️
src/graph/executor/StorageAccessExecutor.h 66.23% <0.00%> (-2.60%) ⬇️
src/graph/executor/query/IndexScanExecutor.cpp 88.88% <0.00%> (-2.54%) ⬇️
src/graph/service/PermissionCheck.cpp 78.72% <0.00%> (-2.13%) ⬇️
src/common/meta/NebulaSchemaProvider.cpp 87.37% <0.00%> (-0.86%) ⬇️
src/common/meta/ServerBasedSchemaManager.cpp 63.82% <0.00%> (-0.69%) ⬇️
...c/graph/scheduler/AsyncMsgNotifyBasedScheduler.cpp 73.61% <0.00%> (-0.62%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76bdc3f...5e28520. Read the comment docs.

@yixinglu yixinglu merged commit 66aec86 into vesoft-inc:master Oct 13, 2021
Sophie-Xie pushed a commit that referenced this pull request Oct 13, 2021
Co-authored-by: Yee <2520865+yixinglu@users.noreply.github.com>
critical27 pushed a commit that referenced this pull request Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-v2.6 PR: need cherry-pick to this version ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants