Kyle Ackerman <kack@kyleackerman.net> wrote:
> On Wednesday, December 11th, 2024 at 12:09 PM, Stefan Sperling <stsp@stsp.name> wrote:
> > On Thu, Dec 05, 2024 at 08:26:18PM +0000, Kyle Ackerman wrote:
> > 
> > > This is a WIP diff to give an option for regression tests.
> > > 
> > > Here is an example once you apply the patch
> > > 
> > > `cd ./regress/cmdline/ MEM_CHECK=1 ./send.sh`
> > > 
> > > You can set MEM_CHECK to {1,2,3,4} which will be passed down to MALLOC_OPTIONS.
> > > If a regression produces a memory leak, then the kdump is left in $testroot, otherwise
> > > it is deleted with $testroot. The kdump is also printed.
> > > I currently only have this implemented in ./add.sh and ./send.sh. Trying to enable
> > > MEM_CHECK on a regress without it being implemented will cause it to fail.
> > > The current pain point with the diff below is that ktrace(1) has a return value, so
> > > it may cause issues as we check got's return value in some regressions.
> > 
> > 
> > Thanks a lot! This is a great starting point.
> > 
> > Below is a diff against the main branch, which I based on yours.
> > There are some simplications, and I would like to separate out the
> > memory leak tests from the rest, for two reasons:
> > 
> > 1) We don't want to carry the burden of thinking about memleaks
> > testing while writing tests for other bugs. That would just get
> > in the way of regular bug fixing. And this solves the problem of
> > dealing with ktrace-specific exit codes since the memleak tests
> > can be written to take this into account.
> > 
> > 2) We can run memleak checks by default when they're separated out.

Good points, and I agee. The separate tests should make it simpler to
design tests to uncover any leaks. If we relied on the regression test
suite, it's unlikely we'd get as thorough coverage and, like you say,
it would only interfere with regular bug fixing.

> > Attached is a patch relative to the one you sent, showing the tweaks
> > I made on top of yours. Both diffs applied together produce the diff
> > below.
> > 
> > If you're happy with this I would commit these 2 diffs in a series and
> > we can keep tweaking it in-tree, adding more memleak tests over time.
> > 
> > diff refs/heads/main refs/heads/memleak
> > commit - 75bd1d8cea00a4cbdbdf6bcea2eb272df2fef76e
> > commit + f432cdf424b63d4701af6c0170531a713e82d83c
> 
> Looks good to me, thanks!

Likewise, ok for me. Thanks, stsp!

And thanks, Kyle, for kickstarting this; I think it's a valuable
addition to the test suite. Having this tool in base is such a boon
to development, and making use of it like this is clever.

-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68