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

refactor: add tests-integration module #590

Merged
merged 5 commits into from
Nov 29, 2022

Conversation

sunng87
Copy link
Member

@sunng87 sunng87 commented Nov 19, 2022

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

While cleanup residual code review suggestions in #474 , I just realized this http_test is no longer suitable to sit in datanode module:

  • it uses an integration test methodology that calls http api directly, which is not a unit test
  • the functionality is no longer full contained in datanode, it covers logic from servers, datanode to frontend
  • it prevents us from further decoupling datanode and frontend in terms of dependency

This PR creates a dedicated tests-integration module to hold tests like http_test. It will depend on datanode, frontend and meta in future. if this idea sounds good to you, I'm going to move forward:

  • move test_util from datanode into tests-integration still in use for datanode's tests
  • move grpc_test, which is similar to current http_test, into tests-integration. And note that datanode still needs its own grpc_test as unit test to test our internal datanode-frontend communication.
  • please add in comment if there are more tests to be moved into tests-integration
  • tests for mysql handler in datanode is to be removed as in Remove MySQL server in datanode #556
  • resolve residual test issues in feat: Update Http SQL api for dashboard requirements #474

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

N/A

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 611 files.

Valid Invalid Ignored Fixed
520 1 90 0
Click to see the invalid file list
  • tests-integration/src/lib.rs

tests-integration/src/lib.rs Show resolved Hide resolved
@sunng87 sunng87 changed the title refactor: add integration-tests module refactor: add tests-integration module Nov 19, 2022
@sunng87 sunng87 force-pushed the test/improve-http-integration-tests branch from 3611c66 to a785904 Compare November 25, 2022 08:26
@sunng87 sunng87 marked this pull request as ready for review November 25, 2022 10:15
@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #590 (07b3178) into develop (30940e6) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #590      +/-   ##
===========================================
- Coverage    86.35%   86.35%   -0.01%     
===========================================
  Files          404      406       +2     
  Lines        51238    51307      +69     
===========================================
+ Hits         44249    44305      +56     
- Misses        6989     7002      +13     
Flag Coverage Δ
rust 86.35% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/datanode/src/lib.rs 100.00% <ø> (ø)
src/servers/src/http.rs 77.91% <100.00%> (-4.18%) ⬇️
tests-integration/src/lib.rs 100.00% <100.00%> (ø)
tests-integration/src/test_util.rs 100.00% <100.00%> (ø)
src/storage/src/engine.rs 82.38% <0.00%> (+0.47%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Looks good to me

tests-integration/tests/http.rs Show resolved Hide resolved
@v0y4g3r
Copy link
Contributor

v0y4g3r commented Nov 28, 2022

I just found that tests inside src/datanode/src/tests/instance_test.rs should also be moved to test-integration module.

@sunng87
Copy link
Member Author

sunng87 commented Nov 28, 2022

@v0y4g3r I checked that module and it seems for testing datanode only? Correct me if I'm wrong.

@v0y4g3r
Copy link
Contributor

v0y4g3r commented Nov 28, 2022

@v0y4g3r I checked that module and it seems for testing datanode only? Correct me if I'm wrong.

That's right.

Copy link
Contributor

@v0y4g3r v0y4g3r left a comment

Choose a reason for hiding this comment

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

LGTM

@v0y4g3r v0y4g3r merged commit 75dcf24 into develop Nov 29, 2022
@v0y4g3r v0y4g3r deleted the test/improve-http-integration-tests branch November 29, 2022 08:28
@waynexia waynexia mentioned this pull request Dec 1, 2022
2 tasks
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
* refactor: add integration-tests module

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* test: move grpc module to tests-integration

* test: adapt new standalone mode

* test: improve http assertion

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants