greptimedb icon indicating copy to clipboard operation
greptimedb copied to clipboard

refactor: add tests-integration module

Open sunng87 opened this issue 2 years ago • 1 comments

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:

  • [x] ~~move test_util from datanode into tests-integration~~ still in use for datanode's tests
  • [x] 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
  • [x] ~~tests for mysql handler in datanode is to be removed as in #556~~
  • [x] resolve residual test issues in #474

Checklist

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

Refer to a related PR or issue link (optional)

N/A

sunng87 avatar Nov 19 '22 11:11 sunng87

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%) :arrow_down:

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%) :arrow_down:
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%) :arrow_up:

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

codecov[bot] avatar Nov 25 '22 10:11 codecov[bot]

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

v0y4g3r avatar Nov 28 '22 07:11 v0y4g3r

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

sunng87 avatar Nov 28 '22 08:11 sunng87

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

That's right.

v0y4g3r avatar Nov 28 '22 08:11 v0y4g3r