Separate integration tests and unit tests #3760
Conversation
| @@ -1,160 +0,0 @@ | |||
| // The MIT License | |||
There was a problem hiding this comment.
@wxing1292 My understanding is those tests are covered in ./common/persistence/tests. So I can remove the tests from here?
There was a problem hiding this comment.
if there are no tests referencing these setup, feel free to delete
Makefile
Outdated
| INTEG_TEST_ROOT := ./host | ||
| INTEG_TEST_XDC_ROOT := ./host/xdc | ||
| INTEG_TEST_NDC_ROOT := ./host/ndc | ||
|
|
||
| INTEGRATION_TEST_ROOT := ./common/persistence/tests |
There was a problem hiding this comment.
Nit: DB_INTEGRATION_TEST_ROOT
There was a problem hiding this comment.
It also include ES tests and any future dependency integration tests. So I want to just use integration tests.
There was a problem hiding this comment.
ES is also DB in this context. The path clearly points to persistence integration tests. If we have more integration tests in future, we will have another root and another var for it. So I would call this: PERSISTENCE_INTEGRATION_TEST_ROOT (yes, long, but clear).
| suite.Run(t, s) | ||
| } | ||
|
|
||
| // TODO: Merge persistence-tests into the tests directory. |
There was a problem hiding this comment.
Do we need this anymore?
There was a problem hiding this comment.
Yes. I did not delete the tests under persistence-tests dir.
There was a problem hiding this comment.
May be it is time address this TODO, after you merge this PR.
| @@ -59,10 +59,14 @@ endef | |||
|
|
|||
| TEST_TIMEOUT := 20m | |||
|
|
|||
| # TODO: INTEG_TEST should be functional tests | |||
| INTEG_TEST_ROOT := ./host | |||
There was a problem hiding this comment.
How about renaming this ./host dir, too, to something like ./host_tests? It could have a clearer directory name.
There was a problem hiding this comment.
Will do in the following PR
|
Please ensure those test utils don't end up in the final binary. We should probably add build tags to exclude them from the binary. |
Will migrate to use build tag in the following PR and make sure we don't build the test utils. |
| @@ -59,10 +59,14 @@ endef | |||
|
|
|||
| TEST_TIMEOUT := 20m | |||
|
|
|||
| # TODO: INTEG_TEST should be functional tests | |||
There was a problem hiding this comment.
I think you need to address these TODOs first (in separate PR). Otherwise, we gonna have two different integration test sets and w/o looking at this PR it would be extremely hard to understand "why?".
There was a problem hiding this comment.
Or you can do it right after. Just please don't postpone it.
Makefile
Outdated
| INTEG_TEST_ROOT := ./host | ||
| INTEG_TEST_XDC_ROOT := ./host/xdc | ||
| INTEG_TEST_NDC_ROOT := ./host/ndc | ||
|
|
||
| INTEGRATION_TEST_ROOT := ./common/persistence/tests |
There was a problem hiding this comment.
ES is also DB in this context. The path clearly points to persistence integration tests. If we have more integration tests in future, we will have another root and another var for it. So I would call this: PERSISTENCE_INTEGRATION_TEST_ROOT (yes, long, but clear).
| suite.Run(t, s) | ||
| } | ||
|
|
||
| // TODO: Merge persistence-tests into the tests directory. |
There was a problem hiding this comment.
May be it is time address this TODO, after you merge this PR.
What changed?
Why?
Unit tests should not have dependencies other than testing and mocking libs.
How did you test it?
Existing tests.
Potential risks
Is hotfix candidate?