[READY] Run valgrind in CI

[bstaletic]

Python 3.8 is finally nice enough to not get in the way of running valgrind with PYMALLOC=malloc. The question is what should we run through valgrind? Here are the options:

• ycm_core_tests, which embeds the python interpreter. Currently valgrind detects a leak in pybind
• ycm_benchmarks, same comment as the above.
• nose tests? I haven't tried this, but valgrind will detect another leak in pybind
• Some combination of the above?

Currently the CI is running only a single benchmark, from ycm_benchmarks binary.

CI is also complaining about a missing libsqlite3-dev package. We probably should fix it, even though the tests aren't broken.

---

This change is 

[codecov]

Codecov Report

Merging #1383 into master will increase coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1383      +/-   ##
==========================================
+ Coverage   96.90%   97.00%   +0.10%     
==========================================
  Files          95       95              
  Lines        7490     7491       +1     
  Branches      154      154              
==========================================
+ Hits         7258     7267       +9     
+ Misses        190      182       -8     
  Partials       42       42              

[puremourning]

ideally we'd run tests that cover the maximal amount of our code, but don't take forever to run under the vm.

so maybe:

• ycm_core_tests
• pytest of ycmd/tests/.py, ycmd/tests/clang/.py

(i.e. the core identifier engine and the libclang) ?

Depends how hard it is to run the pytest with valgrind python. I think I've managed this before but IIRC it was a bit of a faff.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

build.py, line 580 at r1 (raw file):

                 '--error-exitcode=1',
                 '--leak-check=full',
                 '--suppressions=' + p.join( DIR_OF_THIS_SCRIPT, 'supp' ),

prefer to call the file valgrind.suppressions or something like that.

[bstaletic]

Ugh... I just realized that I'll have to redo suppressions when we move to pytest/python3.6 because some internal functions, visible in debug build and therefore in the suppressions, have changed.

I'll wait until we merge pytest. Marking as WIP.

[bstaletic]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

build.py, line 580 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

prefer to call the file valgrind.suppressions or something like that.

Done.

---

run_tests.py, line 229 at r2 (raw file):

      p.join( DIR_OF_THIS_SCRIPT, 'ycmd', 'tests', '[cdefhimpru]*_test.py' ) )
    pytests_args += glob.glob(
      p.join( DIR_OF_THIS_SCRIPT, 'ycmd', 'tests', 's[iu]*_test.py' ) )

The choice of tests deserves an explanation.

• bindings and clang, because they were clear candidates for valgrind-testing ycm_core.
• pcdefhimpru is every ycmd/tests/*_test.py except those starting with s or g.
  • s was skipped because of shutdown_test.py that requirees all completers.
  • g was skipped because of get_completions_test.py that has a weird failure. pytest get_completions_test.py fails, but ./run_tests.py get_completions_test.py works. Adding valgrind on top, at least in azure, consistently breaks the test.
• s[iu] matches subcommands_test.py and signature_help_test.py, but still avoids shutdown_test.py.

---

valgrind.suppressions, line 216 at r2 (raw file):

}
{
	<insert_a_suppression_name_here>

I'm not sure what are these final 6 leaks, 1 of which is "definite".

What I do know is that the final 2 leaks only appear when adding the ycmd/tests/*_test.py files to the list of tests to run. All 6 of them happen when pytest has finished.

[puremourning]

Reviewed 1 of 7 files at r2, 1 of 2 files at r3.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

run_tests.py, line 229 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

The choice of tests deserves an explanation.

• bindings and clang, because they were clear candidates for valgrind-testing ycm_core.
• pcdefhimpru is every ycmd/tests/*_test.py except those starting with s or g.
  • s was skipped because of shutdown_test.py that requirees all completers.
  • g was skipped because of get_completions_test.py that has a weird failure. pytest get_completions_test.py fails, but ./run_tests.py get_completions_test.py works. Adding valgrind on top, at least in azure, consistently breaks the test.
• s[iu] matches subcommands_test.py and signature_help_test.py, but still avoids shutdown_test.py.

is there not a way to tell pytest to exclude certain files rather than this somewhat brittle glob?

maybe this:
http://doc.pytest.org/en/latest/example/pythoncollection.html

i think this approach is just likely to get annoying. i mean i'm happy to exclude the tests you've identified, but this approach seems doomed to future confusion.

---

valgrind.suppressions, line 216 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I'm not sure what are these final 6 leaks, 1 of which is "definite".

What I do know is that the final 2 leaks only appear when adding the ycmd/tests/*_test.py files to the list of tests to run. All 6 of them happen when pytest has finished.

custom memory managers, which it looks like might be in use here, can often interfere with memory checkers (or at least make them less effective)

not sure that we need to worry too much about leak-on-exit anyway

---

azure/lint.sh, line 17 at r3 (raw file):

test ${python_version} == ${YCM_PYTHON_VERSION}
YCM_TESTRUN=1 python build.py --clang-completer --clang-tidy --valgrind
PYTHONMALLOC=malloc python run_tests.py --valgrind --skip-build --no-flake8

doesn't --valgrind add the PYTHONMALLOC setting ? does that make this one redundant or am i misunderstanding ?

[puremourning]

Reviewed 3 of 7 files at r2, 1 of 2 files at r3.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

[bstaletic]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

run_tests.py, line 229 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

is there not a way to tell pytest to exclude certain files rather than this somewhat brittle glob?

maybe this:
http://doc.pytest.org/en/latest/example/pythoncollection.html

i think this approach is just likely to get annoying. i mean i'm happy to exclude the tests you've identified, but this approach seems doomed to future confusion.

I figured out why get_completions_test was failing - the cregex library wasn't being built. We just need to exclude shutdown_test.py.

Done.

---

valgrind.suppressions, line 216 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

custom memory managers, which it looks like might be in use here, can often interfere with memory checkers (or at least make them less effective)

not sure that we need to worry too much about leak-on-exit anyway

Alright, I'll pretend that it is fine.

Do I need to add any comment here?

---

azure/lint.sh, line 17 at r3 (raw file):

Previously, puremourning (Ben Jackson) wrote…

doesn't --valgrind add the PYTHONMALLOC setting ? does that make this one redundant or am i misunderstanding ?

Right. Done.

[puremourning]

Let's go for it. If we find that the tests are taking too long and annoying us, we can consider strategies.

Reviewed 2 of 2 files at r4.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

---

valgrind.suppressions, line 216 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Alright, I'll pretend that it is fine.

Do I need to add any comment here?

probably worthwhile. we're unlikely to remember why most of these suppression's exist, but the important ones worth a quick comment if that's easy to add

[bstaletic]

Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

---

valgrind.suppressions, line 216 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

probably worthwhile. we're unlikely to remember why most of these suppression's exist, but the important ones worth a quick comment if that's easy to add

Comment added to all the suppressions.

[puremourning]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! 2 of 2 LGTMs obtained

[bstaletic]

Just a note. --ignore doesn't work when you explicitly select a file via glob, so --ignore=ycmd/tests/shutdown_test.py in run_tests.py doesn't work. The solution is to use --deselect instead. I'll push the change once we fix CI in #1400.

[puremourning]

Reviewed 2 of 2 files at r6.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale)

[bstaletic]

Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

---

run_tests.py, line 229 at r6 (raw file):

      p.join( DIR_OF_THIS_SCRIPT, 'ycmd', 'tests', '*_test.py' ) )
    # Avoids needing all completers for a valgrind run
    pytests_args += [ '--deselect', p.join( DIR_OF_THIS_SCRIPT,

Pytest didn't want to listen until I used a relative path to deselect the shutdown_test.py.

I'm not going to merge this PR, in case you want to comment on it.

[bstaletic]

Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

---

pytest.ini, line 8 at r7 (raw file):

log_date_format = "%Y-%m-%d %H:%M:%S"
log_format = "%(asctime)s - %(levelname)s - %(message)s"
markers = valgrind_skip

This registers a simple mark. We can use it to filter tests.

---

run_tests.py, line 231 at r7 (raw file):

      p.join( DIR_OF_THIS_SCRIPT, 'ycmd', 'tests', '*_test.py' ) )
    # Avoids needing all completers for a valgrind run
    pytests_args += [ '-m', 'not valgrind_skip' ]

Pytest seems unable to accept --deselect with an absolute path, so instead I opted for a custom mark.

---

ycmd/tests/shutdown_test.py, line 50 at r7 (raw file):

  @pytest.mark.valgrind_skip

Finally, only two, instead of four, tests need to be avoided.
The other two tests should be fast enough for valgrind.

[puremourning]

Reviewed 3 of 3 files at r7.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale)

[mergify]

Thanks for sending a PR!