Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v1] test: need ASAN testing in commit process

["Alexander Tikhonov" <avtikhon@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5518 bytes --]


Igor,

Thank you for your review, I've made all changes that you suggested. 
>Среда, 20 ноября 2019, 17:40 +03:00 от Igor Munkin <imun@tarantool.org>:
>
>Sasha,
>
>The patch doesn't seem to be cherry-picked but partially based on
>55f7586. Please adjust the commit message and changes considering the
>comments I left below. 
Done.
>
>On 06.11.19, Alexander V. Tikhonov wrote:
>> Added ASAN tesing in commit process, used clang-8 for
>> ASAN build under debian-buster image. Added for testing
>> only the passing test suites, the rest of the tests
>> not used and will be enabled durring issue #4360.
>
>Please drop few lines about the fact you've reduced a number of targets
>being tested via Travis, therefore this part is stripped from the
>original patch.
Wrote more information on LTO.
>
>> 
>> Closes #4359
>> 
>> (cherry picked from commit 55f7586ac36760f31255fee7b70606f466f30e04)
>
>As I mentioned above, adjust this line, because it's not a cherry-pick
>but adapting to 1.10 branch.
Corrected the message.
>
>> ---
>> 
>> Github:  https://github.com/tarantool/tarantool/tree/avtikhon/asan_1.10
>> 
>>  .gitlab-ci.yml             |  5 +++++
>>  .travis.mk                 | 24 ++++++++++++++++++++++++
>>  test/app-tap/json.skipcond |  7 +++++++
>>  test/unit/guard.skipcond   |  7 +++++++
>>  test/unit/msgpack.skipcond |  7 +++++++
>>  5 files changed, 50 insertions(+)
>>  create mode 100644 test/app-tap/json.skipcond
>>  create mode 100644 test/unit/guard.skipcond
>>  create mode 100644 test/unit/msgpack.skipcond
>> 
>> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
>> index 04bb4b1cc..5d3702ae5 100644
>> --- a/.gitlab-ci.yml
>> +++ b/.gitlab-ci.yml
>> @@ -85,6 +85,11 @@ release_lto_clang8:
>>    script:
>>      - ${GITLAB_MAKE} test_debian_no_deps
>> 
>> +release_asan_clang8:
>> +  <<: *docker_test_clang8_definition
>> +  script:
>> +    - ${GITLAB_MAKE} test_asan_debian_no_deps
>> +
>>  osx_13_release:
>>    <<: *release_only_definition
>>    <<: *vbox_definition
>> diff --git a/.travis.mk b/.travis.mk
>> index bfb3016da..8454eb4a5 100644
>> --- a/.travis.mk
>> +++ b/.travis.mk
>> @@ -16,6 +16,9 @@ test: test_$(TRAVIS_OS_NAME)
>>  # Redirect some targets via docker
>>  test_linux: docker_test_debian
>>  coverage: docker_coverage_debian
>> +lto: docker_test_debian
>> +lto_clang8: docker_test_debian_clang8
>
>All LTO related stuff has been stripped from the original patch, thus
>these definitions seems to be excess.
Right, that was mistake - removed these lines.
>
>> +asan: docker_test_asan_debian
>> 
>>  docker_%:
>>  	mkdir -p ~/.cache/ccache
>> @@ -68,6 +71,8 @@ deps_buster_clang_8: deps_debian
>>  	apt-get update
>>  	apt-get install -y clang-8 llvm-8-dev
>> 
>> +# Release
>> +
>>  build_debian:
>>  	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
>>  	make -j
>> @@ -77,6 +82,10 @@ test_debian_no_deps: build_debian
>> 
>>  test_debian: deps_debian test_debian_no_deps
>> 
>> +test_debian_clang8: deps_debian deps_buster_clang_8 test_debian_no_deps
>> +
>> +# Debug with coverage
>> +
>>  build_coverage_debian:
>>  	cmake . -DCMAKE_BUILD_TYPE=Debug -DENABLE_GCOV=ON
>>  	make -j
>> @@ -97,6 +106,21 @@ test_coverage_debian_no_deps: build_coverage_debian
>> 
>>  coverage_debian: deps_debian test_coverage_debian_no_deps
>> 
>> +# ASAN
>> +
>> +build_asan_debian:
>> +	CC=clang-8 CXX=clang++-8 cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
>> +	CC=clang-8 CXX=clang++-8 cmake . -DENABLE_ASAN=ON ${CMAKE_EXTRA_PARAMS}
>> +	make -j
>> +
>> +test_asan_debian_no_deps: build_asan_debian
>> +	# temporary excluded engine/ and replication/ suites with some tests from other suites by issue #4360
>> +	cd test && ASAN=ON ASAN_OPTIONS=detect_leaks=0 ./test-run.py --force $(TEST_RUN_EXTRA_PARAMS) \
>> +		app/ app-tap/ box/ box-py/ box-tap/ engine_long/ long_run-py/ luajit-tap/ \
>> +		replication-py/ small/ sql/ sql-tap/ swim/ unit/ vinyl/ wal_off/ xlog/ xlog-py/
>> +
>> +test_asan_debian: deps_debian deps_buster_clang_8 test_asan_debian_no_deps
>> +
>>  #######
>>  # OSX #
>>  #######
>
>Side note: I guess it's a good practice for future to write a rationale
>for excluded tests right to skipcond file.
That is the traditional way that is in use for now, may be decide to change
in the future, but it will be the standalone patch for all the skipcond files.
>
>> diff --git a/test/app-tap/json.skipcond b/test/app-tap/json.skipcond
>> new file mode 100644
>> index 000000000..e46fd1088
>> --- /dev/null
>> +++ b/test/app-tap/json.skipcond
>> @@ -0,0 +1,7 @@
>> +import os
>> +
>> +# Disabled at ASAN build due to issue #4360.
>> +if os.getenv("ASAN") == 'ON':
>> +    self.skip = 1
>> +
>> +# vim: set ft=python:
>> diff --git a/test/unit/guard.skipcond b/test/unit/guard.skipcond
>> new file mode 100644
>> index 000000000..e46fd1088
>> --- /dev/null
>> +++ b/test/unit/guard.skipcond
>> @@ -0,0 +1,7 @@
>> +import os
>> +
>> +# Disabled at ASAN build due to issue #4360.
>> +if os.getenv("ASAN") == 'ON':
>> +    self.skip = 1
>> +
>> +# vim: set ft=python:
>> diff --git a/test/unit/msgpack.skipcond b/test/unit/msgpack.skipcond
>> new file mode 100644
>> index 000000000..e46fd1088
>> --- /dev/null
>> +++ b/test/unit/msgpack.skipcond
>> @@ -0,0 +1,7 @@
>> +import os
>> +
>> +# Disabled at ASAN build due to issue #4360.
>> +if os.getenv("ASAN") == 'ON':
>> +    self.skip = 1
>> +
>> +# vim: set ft=python:
>
>> -- 
>> 2.17.1
>> 
>
>-- 
>Best regards,
>IM
>


-- 
Alexander Tikhonov

[-- Attachment #2: Type: text/html, Size: 7954 bytes --]