Run tests in parallel #1657
Run tests in parallel #1657
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1657 +/- ##
==========================================
- Coverage 91.44% 91.38% -0.06%
==========================================
Files 727 730 +3
Lines 26375 26455 +80
==========================================
+ Hits 24118 24177 +59
- Misses 2257 2278 +21
☔ View full report in Codecov by Sentry. |
- Use joinPath on DeleteNodeWithEdgesErrorTest to handle windows path test failure - Refactor CSV to Parquet converter
| @@ -64,7 +64,7 @@ jobs: | |||
| run: CC=gcc CXX=g++ make alldebug NUM_THREADS=32 | |||
|
|
|||
| - name: Run test with ASan | |||
| run: ctest | |||
| run: ctest --output-on-failure -j 10 | |||
There was a problem hiding this comment.
I'm not familiar with ASan. Is it ok to run multiple jobs here?
There was a problem hiding this comment.
I think it should be fine.
I noticed one test (CApiConnectionTest.Interrupt) failing in asan CI test, can you take a further look?
There was a problem hiding this comment.
I can take a look but this error is also in other PRs.
There was a problem hiding this comment.
Got it. Can you open an issue on this then?
| @@ -2,11 +2,10 @@ | |||
|
|
|||
| class CApiDatabaseTest : public EmptyDBTest { | |||
| public: | |||
| void SetUp() override {} | |||
| void SetUp() override { EmptyDBTest::SetUp(); } | |||
There was a problem hiding this comment.
@mewim Is it ok to add EmptyDBTest::SetUp() here? I basically need to set the temporary dynamic database path instead of using the fixed one.
There was a problem hiding this comment.
Sorry I missed this. Since it is only setting a path it is fine.
| @@ -64,7 +64,7 @@ jobs: | |||
| run: CC=gcc CXX=g++ make alldebug NUM_THREADS=32 | |||
|
|
|||
| - name: Run test with ASan | |||
| run: ctest | |||
| run: ctest --output-on-failure -j 10 | |||
There was a problem hiding this comment.
I think it should be fine.
I noticed one test (CApiConnectionTest.Interrupt) failing in asan CI test, can you take a further look?
| -CASE DeleteNodeWithEdgeErrorTest | ||
| -SKIP | ||
| -STATEMENT MATCH (a:person) WHERE a.ID = 0 DELETE a | ||
| ---- error | ||
| Runtime exception: Currently deleting a node with edges is not supported. node table 0 nodeOffset 0 has 3 (one-to-many or many-to-many) edges for edge file: ${KUZU_ROOT_DIRECTORY}/test/unittest_temp/r-3-0.lists. |
There was a problem hiding this comment.
Let's simplify the error message here, and remove "for edge file: ${KUZU_ROOT_DIRECTORY}/test/unittest_temp/r-3-0.lists"
CMakeLists.txt
Outdated
| @@ -112,7 +112,7 @@ function(add_kuzu_test TEST_NAME) | |||
| target_link_libraries(${TEST_NAME} PRIVATE test_helper test_runner graph_test) | |||
| target_include_directories(${TEST_NAME} PRIVATE ${PROJECT_SOURCE_DIR}/test/include) | |||
| include(GoogleTest) | |||
| gtest_discover_tests(${TEST_NAME}) | |||
| gtest_discover_tests(${TEST_NAME} DISCOVERY_MODE PRE_TEST) | |||
There was a problem hiding this comment.
Can you double check if this change is necessary, if not, revert. DISCOVERY_MODE requires higher cmake version (3.18+), while we target at 3.11 right now.
This PR implements tests in parallel. The database is now created using a dynamic path instead of a fixed directory defined in the test helper. We use the parallel job feature from the
ctestutility, but we can easily switch to another utility or create our own utility if necessary.The default number of parallel jobs during the tests is 10, but it's possible to change it when running make:
I didn't get much different results when increasing from 10. I believe it's due to I/O bound of the tests, so 10 should be enough to speed up the run substantially.
On my machine, the total time is around 120s without parallelization. When running 10 jobs, the total time drops to less than 20 seconds.
I also noticed a great improvement in
MSVC build & teston CI. The current open PRs average time is around 40 minutes. In this PR, this number drops to around 10 minutes.TODO (future?): we must develop a better clean-up code for the entire test suite. If we signal a stop during the test or if it segfaults, it won't cleanup the temporary files
unittest_temp_*anddataset/parquet_temp_*.