Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for running units tests with valgrind. #185

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Add support for running units tests with valgrind. #185

wants to merge 12 commits into from

Conversation

ossama-othman
Copy link
Member

Leverage the Autoconf Archive macro AX_VALGRIND_CHECK to enable support for running the mptcpd unit tests through valgrind, e.g.:

    ./configure --enable-valgrind
    make check-valgrind

@ossama-othman ossama-othman added the enhancement New feature or request label Dec 6, 2021
@ossama-othman ossama-othman added this to the Q4-2021 milestone Dec 6, 2021
@ossama-othman ossama-othman added this to In progress in mptcpd 0.9 via automation Dec 6, 2021
@ossama-othman ossama-othman self-assigned this Dec 6, 2021
@coveralls
Copy link

coveralls commented Dec 6, 2021 •
edited

Pull Request Test Coverage Report for Build 1831551729

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 53.266%
Totals Coverage Status
Change from base Build 1790578941: 0.0%
Covered Lines: 1060
Relevant Lines: 1990
💛 - Coveralls

@ossama-othman
Copy link
Member Author

The Valgrind build is failing because of legitimate memory leaks in two unit tests. The changes are behaving as expected.

mjmartineau
mjmartineau reviewed Dec 6, 2021
View reviewed changes
Copy link
Member

@mjmartineau mjmartineau left a comment

Choose a reason for hiding this comment

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

While I knew you had submitted the fix for autoconf archive, I still got stuck trying to build and run on my local system until I looked through the workflow and saw that it's downloading your not-yet-merged changes :) By following the sequence of commands in valgrind.yml I was able to successfully run "make check-valgrind" on Fedora 34, with the expected two memcheck failures.

It would definitely help to document that autoconf dependency!

Does autoconf give any way to check the serial number of the m4 file? If someone decides to try out this feature it would be good to provide some information, since it will take a while (years?) for the autoconf-archive fix to show up in distros.

@ossama-othman
Copy link
Member Author

It would definitely help to document that autoconf dependency!

Good point. It's really only needed if someone is building mptcpd from a git repository or is modifying the autoconf/automake files shipped with the mptcpd tar archive. A mptcpd tar archive created through make dist or make distcheck doesn't have this issue since the macro content is placed into the generated configure script shipped with the tar archive. This assumes that the person that created the tar archive had the updated AX_VALGRIND_CHECK macro installed when creating it.

Does autoconf give any way to check the serial number of the m4 file? If someone decides to try out this feature it would be good to provide some information, since it will take a while (years?) for the autoconf-archive fix to show up in distros.

The Automake manual mentions that the aclocal Perl script take into account the serial number when deciding which copy of a M4 file to use, but I'm not aware of any features in Autoconf or Automake that would allow one to check the serial number. We'd likely have to roll our own serial check.

@ossama-othman ossama-othman force-pushed the valgrind-check branch 2 times, most recently from db87790 to cc5ffa5 Compare January 6, 2022 21:30
ossama-othman
ossama-othman commented Jan 13, 2022
View reviewed changes
bootstrap Outdated Show resolved Hide resolved
@ossama-othman ossama-othman removed this from In progress in mptcpd 0.9 Feb 1, 2022
@ossama-othman ossama-othman added this to In progress in mptcpd 0.10 via automation Feb 1, 2022
@ossama-othman ossama-othman modified the milestones: Q4-2021, Q1-2022 Feb 1, 2022
@ossama-othman
Add support for running units tests with valgrind.
36323e5 
Leverage the Autoconf Archive macro AX_VALGRIND_CHECK to enable
support for running the mptcpd unit tests through valgrind, e.g.:

./configure --enable-valgrind
make check-valgrind
@ossama-othman
github: New workflow that runs valgrind on tests.
bea718d
@ossama-othman
github: Dump Valgrind memcheck errors.
e5da145
@ossama-othman
Move ax_valgrind_check.m4 download to bootstrap.
3679920
@ossama-othman
bootstrap: Untabify.
cdf65cc
@ossama-othman
github: Drop error dump that doesn't get executed.
009dbfa
@ossama-othman
README: Describe how to run tests through Valgrind.
a602db3
@ossama-othman
github: Drop pandoc from Valgrind workflow.
6098577
@ossama-othman
tests: Add Makefile rule to build libmptcpd.
e614bd4 
Add an explicit rule in tests/lib/Makefile.am to build `libmptcpd.la'
to allow `make check-valgrind' to be performed without first running
`make' (i.e. `make all').
@ossama-othman
bootstrap: Update ax_valgrind_check.m4 URL.
5ac2396 
The GitHub pull request (autoconf-archive/autoconf-archive#242) that
contains the AX_VALGRIND_CHECK GNU Autoconf Archive macro recursive
`make' target fixes has been merged to the master development branch,
and is now part of the v2022.02.11 release.  Update the URL for the
patched `ax_valgrind_check.m4' file accordingly.
@ossama-othman
bootstrap: Do not modify configure script.
f778439 
Instead of modifying the `configure.ac' file, issue a warning that
"make check-valgrind" may not work properly.  This prevents changes to
the `configure.ac' caused by the `bootstrap' script from being
inadvertently committed to a git repository.
@ossama-othman ossama-othman marked this pull request as draft February 11, 2022 20:09
@ossama-othman
bootstrap: Use the CGIT URL instead of GITWEB.
537fa51 
Running `wget' on the GITWEB based URL caused a HTML page to be
downloaded instead of the plain text `ax_valgrind.check.m4' file.  Use
the CGIT based URL instead.
@ossama-othman ossama-othman marked this pull request as ready for review February 11, 2022 21:17
@ossama-othman
Copy link
Member Author

I should probably bump the copyright year on the modified files.

ossama-othman
ossama-othman commented Feb 12, 2022
View reviewed changes
bootstrap
echo Patched AX_VALGRIND_CHECK GNU Autoconf Archive macro not found.
echo -n "Attempting to download patched ax_valgrind_check.m4... "
cd m4
wget --quiet --timeout 10 http://git.savannah.gnu.org/cgit/autoconf-archive.git/plain/m4/ax_valgrind_check.m4
Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps it would be better to make the download optional and switched off by default.

@ossama-othman ossama-othman marked this pull request as draft February 14, 2022 02:34
@ossama-othman ossama-othman removed this from In progress in mptcpd 0.10 Jun 27, 2022
@ossama-othman ossama-othman added this to In progress in mptcpd 0.11 via automation Jun 27, 2022
@ossama-othman ossama-othman removed this from In progress in mptcpd 0.11 Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjmartineau mjmartineau mjmartineau left review comments

Assignees

@ossama-othman ossama-othman

Labels
enhancement New feature or request
Projects
None yet
Milestone
Q1-2022
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ossama-othman @coveralls @mjmartineau