api_test failure

[nekopsykose]

When building the same way the CI seems to do, and not passing CMAKE_BUILD_TYPE so it defaults to Release (at the bottom of CMakeLists.txt), cmake adds -DNDEBUG (it also does this for MinSizeRel, for trivia). This makes all assert()s do nothing, and the tests pass.

When explicitly passing another type like =Debug or =None, so -DNDEBUG is not present, the asserts are active, so this test fails:

$ ./api_test/api_test
Assertion failed: b->flags & CMARK_NODE__OPEN (/tmp/mytemp.bcmZP4/src/blocks.c: finalize: 262)
zsh: abort (core dumped)  ./api_test/api_test

I think this is not caught in CI because nothing explicitly sets another type so the asserts are not checked. Or is that intentional?

I can reproduce this with a basic

cmake -B b -G Ninja -DCMAKE_BUILD_TYPE=Debug
ninja -C b
ctest --test-dir b --output-on-failure

on debian/arch at the time of writing, on db0da216ba81435ce1547f9d1159a7f91298d187.

[jgm]

Wow, no, that wasn't intentional. Thanks for catching that.

I can reproduce locally with:

mkdir build2
cd build2
cmake .. -DCMAKE_BUILD_TYPE=Debug
ctest --output-on-failure

@nwellnhof maybe the easiest approach would be to change the default build type, but this might break or silently change some existing builds, so maybe better to change the Makefile and CI to ensure that Debug builds are used for routine testing. Thoughts?

[nwellnhof]

I wouldn't change the default build type. CI should obviously run with a debug build. We already have a "debug" target in the Makefile that produces a debug build.

[jgm]

But for local testing, too, it would be nice if make test actually tested the api! Is there a way to tell cmake to always build the api_test with debug mode, regardless of whether that is being used for the main build?

[nwellnhof]

The assertions we're talking about aren't in the test executable but in the libcmark binary (blocks.c in this case). So you have to build the library in debug mode. It doesn't matter in which mode api_test is built.

make test will always test the API. It's just that we could miss some issues when testing locally if assertions are disabled. As long as the CI tests run in debug mode, this shouldn't be a problem.

[jgm]

Oh, I see. I thought these were in api_test.

[nwellnhof]

The failed assertion is caused by 014907ea and the tests added in 0d77ca12. I'd suggest to always flag the root node as open in cmark_parser_new_with_mem_into_root.

[jgm]

I shouldn't have closed this until that is fixed.

[jgm]

@nwellnhof now we're getting a new error: https://github.com/commonmark/cmark/actions/runs/8271832425/job/22632455239#step:4:121 Any ideas what causes that?

[nekopsykose]

clang 14 (used in that run) defaults to -gdwarf-5 which that older (pre- 3.20) version of valgrind can't understand. see https://github.com/llvm/llvm-project/issues/56550 for a few more details

you can manually pass -gdwarf-4, or find a way to upgrade valgrind in the ci