Beanstalkd: improve tests coverage

[ysmolsky]

Right now tests coverage is at 32%. What is worse, some files are not tested at all:
conn.c, file.c, linux.c, prot.c, walg.c

https://codecov.io/gh/beanstalkd/beanstalkd/list/master/

We might want to pick low hanging fruit by adding integration tests that cover wide area.

[kr]

The tests in testserv.c are mostly integration-style tests. They run srvserve in a subprocess and talk to it over a TCP socket. Since the code being tested is in another process, maybe the coverage tool can't see it.

[sergeyklay]

The coverage tool reads *.gcda and *.gcno files which are created independent of the tool we're use to read them.

[ysmolsky]

@sergeyklay not sure if I understood your statement.

I looked into testsrv.c I see that server is started as fork and the test code is built coverage supported. Am I correct by saying that it does not matter how we start program that is compiled with coverage options because it will write instrumentation anyway?

[sergeyklay]

@ysmolsky Yes, I think so.

[ysmolsky]

I think that our coverage is off because I found this:

If you want to perform test coverage analysis using gcov please be aware that gcov is not necessarily multi-process-safe. If you get strange coverage data, try -j1 and avoid forking in your test cases.

Source: https://github.com/kr/ct

@sergeyklay can you please look at it when you have time?

[ysmolsky]

Also we need to exclude some dirs from codecov with this: https://docs.codecov.io/docs/ignoring-paths

[sergeyklay]

I'm totally missed this. Thank you. Good catch! About excluding dirs: which dirs do you want to exclude?

[ysmolsky]

Most importantly, ct, also for clarity sake: adm, pkg, since we are not going to test them.

[sergeyklay]

We should exclude test utils as well:

[ysmolsky]

While working on testing functions in prot.c I have noticed that even though it is covered by tests at least somewhat, this file is reported as 0% coverage. That is completely wrong.

[kr]

While working on testing functions in prot.c I have noticed that even though it is covered by tests at least somewhat, this file is reported as 0% coverage. That is completely wrong.

Yeah. Sorry, that's what I was trying to address with this comment https://github.com/beanstalkd/beanstalkd/issues/424#issuecomment-506929644.

[ysmolsky]

I have actually found the reason. It is not forking and collecting from separate process. Coverage works fine from any process. The culprit is killpid(pid, sigkill) of the child server, where coverage cannot write cov data. The fix is to setup sigusr2 handler and handle that in tested server to flush cov data before the exit. I wonder if it makes sense to patch upstream ct lib? Do you maintain that still? Or should I fix it as it is vendored in this repo?

On Jul 8, 2019, at 06:33, Keith Rarick [e-mail] wrote:

While working on testing functions in prot.c I have noticed that even though it is covered by tests at least somewhat, this file is reported as 0% coverage. That is completely wrong.

Yeah. Sorry, that's what I was trying to address with this comment #424 (comment).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[kr]

The fix is to setup sigusr2 handler and handle that in tested server to flush cov data before the exit. I wonder if it makes sense to patch upstream ct lib?

Yeah, I agree it makes sense to have ct handle this somehow, since it would be difficult for the test suite to do it. Not 100% sure sigusr2 is the best approach, but I opened https://github.com/kr/ct/issues/18 so we can discuss fixing it.

[kr]

After thinking some more, this seems like it would be easy enough to do in beanstalkd's tests with no changes to ct.

In testserv.c, function mustforksrv could register an atexit function to signal the child and then wait for the child to finish.

...
if (srvpid > 0) {
    atexit(kill_srv_atexit);
    printf("start server port=%d pid=%d\n", port, srvpid);
    return port;
}
...

With kill_srv_atexit defined as something like

// Kill the child process with our chosen signal to give it a chance
// to write gcov data to the filesystem before ct kills it with SIGKILL.
void
kill_srv_atexit(void)
{
    kill(srvpid, SIGUSR2);
    waitpid(srvpid, ...);
}

Might even be good to actually use SIGTERM there instead of SIGUSR2 since we do in fact want to kill the process.

It is trickier than I expected to do something reasonable in ct itself, but I have an idea. I'll comment over there about that.

[ysmolsky]

Closing as the push for it was done, any other kind of improvement should be discussed separately.