Skip to content
This repository was archived by the owner on May 10, 2024. It is now read-only.

PARQUET-564: Add cmake option to run valgrind on each unit test executable#79

Closed
wesm wants to merge 3 commits intoapache:masterfrom
wesm:PARQUET-564
Closed

PARQUET-564: Add cmake option to run valgrind on each unit test executable#79
wesm wants to merge 3 commits intoapache:masterfrom
wesm:PARQUET-564

Conversation

@wesm
Copy link
Member

@wesm wesm commented Mar 13, 2016

This also enables the current test suite to pass cleanly under ctest with this option enabled.

@wesm
Copy link
Member Author

wesm commented Mar 13, 2016

cc @xhochy @asandryh for review

CMakeLists.txt Outdated
add_test(${TEST_NAME}
${BUILD_SUPPORT_DIR}/run-test.sh ${TEST_PATH})
if (PARQUET_TEST_MEMCHECK)
SET_TARGET_PROPERTIES(${TEST_NAME}
Copy link
Member

Choose a reason for hiding this comment

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

This does not work on Linux as it will override all previously set target properties and thus compiling will fail due to the wrong tr1/tuple handling. The following line worked for me and seems to be the common way to append to target properties (note the explicit space between " and -D):

SET_PROPERTY(TARGET ${TEST_NAME} APPEND_STRING PROPERTY COMPILE_FLAGS " -DPARQUET_VALGRIND")

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, thank you, will remember this for the future

@wesm
Copy link
Member Author

wesm commented Mar 14, 2016

@xhochy can you take a look and let me know if this looks good?

I tried using ${MEMORYCHECK_COMMAND} but it does not report leaks as errors (--error-exitcode), so I left the explicit valgrind call as is.

@xhochy
Copy link
Member

xhochy commented Mar 14, 2016

Looking good! ${MEMORYCHECK_COMMAND} just evaluates to /usr/bin/valgrind for me. As it is a very unlikely case that someone wants to use a valgrind installation that is not in PATH, we probably should not care about ${MEMORYCHECK_COMMAND}.

@wesm
Copy link
Member Author

wesm commented Mar 15, 2016

thank you.

@asfgit asfgit closed this in 5c4f645 Mar 15, 2016
@wesm wesm deleted the PARQUET-564 branch March 15, 2016 06:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants