Add tool for automated test generation

[shivansh]

Summary

The generated tests verify whether a utility is properly linked.
Details: https://wiki.freebsd.org/SummerOfCode2017/SmokeTestingOfBaseUtilities

Test Plan

https://github.com/shivansh/smoketestsuite#dependencies

[shivansh]

Minor updates:

• Remove bogus files.
• Update README.

[asomers]

tools/tools/smoketestsuite/README.md

9 ↗

(On Diff #32739)

You should remove the deprecated_tests section, as that directory seems to no longer exist

24 ↗

(On Diff #32739)

Is it really still "in-progress"?

38 ↗

(On Diff #32739)

Remove references to github

tools/tools/smoketestsuite/src/add_testcase.h

28 ↗

(On Diff #32739)

Could you please format the C++ files according to style(9) ?

tools/tools/smoketestsuite/src/generate_test.cpp

276 ↗

(On Diff #32739)

Why do you need to use threads? And what's the purpose of the timeout? This program seems pretty deterministic.

tools/tools/smoketestsuite/src/generated_tests/README.md

2 ↗

(On Diff #32739)

github -> svn

6 ↗

(On Diff #32739)

Since this file is no longer hosted on github, people will be reading the plain text version more often than the rendered version. So you should change the formatting so that columns are aligned in the plain text version.

tools/tools/smoketestsuite/src/generated_tests/date_test.sh

1 ↗

(On Diff #32739)

There is no need to commit example output

tools/tools/smoketestsuite/src/generated_tests/ln_test.sh

1 ↗

(On Diff #32739)

This whole file is out of date and should be removed.

tools/tools/smoketestsuite/src/generated_tests/mkdir_test.sh

1 ↗

(On Diff #32739)

This whole file is out of date and should be removed.

tools/tools/smoketestsuite/src/generated_tests/rmdir_test.sh

1 ↗

(On Diff #32739)

This whole file is out of date and should be removed.

tools/tools/smoketestsuite/src/generated_tests/stdbuf_test.sh

1 ↗

(On Diff #32739)

This whole file is out of date and should be removed.

tools/tools/smoketestsuite/src/license

1 ↗

(On Diff #32739)

There's no need to add copies of the license file, since it's already in the header of every source file.

tools/tools/smoketestsuite/src/scripts/validate.sh

1 ↗

(On Diff #32739)

This file should either be converted to use svn, or removed.

[shivansh]

tools/tools/smoketestsuite/src/generate_test.cpp

276 ↗

(On Diff #32739)

Threads are used so that a test generation can be stopped during runtime. Quoting a previous mail which I shared on the purpose of doing this -

On Tue, Jul 25, 2017 at 8:08 PM Alan Somers <[e-mail]> wrote:

> On Tue, Jul 25, 2017 at 8:24 AM, Shivansh Rai <[e-mail]>
> wrote:
> > Hello all,
> >
> > I have made a new repository for collecting generated tests [1] which
> > currently contains 57 tests for utilities randomly selected from section
> 1
> > via the script "fetch_groff.sh" [2]. This repo will be updated with all
> the
> > generated tests in future. Currently some of these tests are machine
> > dependent, but after I implement the functionality to automate test runs
> [3]
> > (and hence generate annotations), these testcases will go away.
> > On another note, I am currently working on a functionality to stop
> execution
> > of tests which hang after a certain duration (I am using boost::thread
> for
> > the purpose) [4]. This will be helpful in generating a large number of
> tests
> > without halting on failure while generating a single test. Although the
> > problem is that after successful compilation, the execution step ends up
> > giving a segmentation fault when I use boost libraries. I suspect the
> > problem is due to improper linking of object files (inclusion of  
> improper
> > boost libs in Makefile) or probably I am not using proper compilation
> flags
> > ; so trying figuring it out (experimenting with boost for the first
> time).
> >
> > [1]: https://github.com/shivrai/smoketests
> > [2]:
> >
> https://github.com/shivrai/smoketestsuite/blob/master/scripts/fetch_groff.sh
> > [3]:
> https://lists.freebsd.org/pipermail/soc-status/2017-July/001094.html
> > [4]:
> >
> https://github.com/shivrai/smoketestsuite/commit/6f0ce7950ca2c6d9265eb8933f9e50960b1c0e76
> >
> > Thank you.
> > With best regards,
> > Shivansh Rai

> Do you mean stopping the execution of a test that hangs, or stopping
> the generation of tests that hang?  Kyua can already do the former.

I meant the latter, i.e. if we are generating a test but the utility has an
option which needs arguments via user input (e.g. `ln -i`), the "test
generation" will hang until the specific user input is provided (the
automation tool currently cannot differentiate between these kind of
options and others, I'll try to add a functionality to ignore these options
in the near future).
I tried to clean up these specific tests which I came across in this
commit:
https://github.com/shivrai/smoketests/commit/8a84a4956f5f396dd14dbce60ac4d8a5b8b11f9a

> By default, every test times out after 5 minutes, but you can change
> it either in the Makefile or in the atf test file itself.

Interesting!
Thanks.
-- Shivansh

> -Alan

tools/tools/smoketestsuite/src/license

1 ↗

(On Diff #32739)

This license file is prepended by the tool to the generated tests. I am thinking of renaming it to something more logical.
Is it advisable to move the contents of this whole file into a string variable ? (if we don't want an extra license file)

[asomers]

tools/tools/smoketestsuite/src/generate_test.cpp

276 ↗

(On Diff #32739)

Makes sense. Though in that specific case, the problem could've been prevented by closing standard input before invoking ln. You should probably do that anyway, since there's no way to provide any meaningful input. For example:

$ touch /tmp/xxxx
$ touch /tmp/yyyy
$$ ln -i /tmp/xxxx /tmp/yyyy < /dev/null
replace /tmp/yyyy? not replaced

tools/tools/smoketestsuite/src/license

1 ↗

(On Diff #32739)

Ahh, I understand. It certainly would be possible to make it a string variable. However, it's dicey whether you can legally claim copyright over the output of your tool. The person invoking the tool has a better case, especially if he had to write any annotations. And of course, it won't be 2017 forever. So you should change this whole thing into a string template, with the year and copyright owner's name filled in at runtime.

[shivansh]

tools/tools/smoketestsuite/src/generate_test.cpp

276 ↗

(On Diff #32739)

Wow! I wish I knew this earlier ;
While this approach is extremely convenient (and I have added this in the implementation, works for all section 1 utilities except a few), utilities like pax(1) and ed(1) still wait for user input (for e.g. pax -r </dev/null or ed -x </dev/null) ; so in cases like these the timeout (of 1 second) in boost::thread::timed_join() will help us.

[shivansh]

Address changes pointed by @asomers -

• Take username as input (to be added in license) during runtime
• Replace "timed_join()"(deprecated) with "try_join_for()"
• Add relevant updates to Makefile
• Remove the bogus license file
• Remove the (demo) generated tests
• Remove the test validation script (the plan is to add this file after testing it against svn)
• Format the files according to style(9)
• Remove references to github from the README

PS. My apologies for the delay, just finished the mid-semester exams.

[shivansh]

Update directory structure in README

[shivansh]

• Update scripts according to style(9)
• Minor fixes concerning directory locations

[shivansh]

A brief summary of changes [1] -

• Skip processing the directory entries for "." and ".."
• Increase the timeout value to 2
• Perform a case-insensitive match with "usage:"
• Remove the logic for checking if test scripts to be generated already exist. The current implementation for this functionality was incomplete, and will be reintroduced once fully implemented.
• Remove local groff scripts and tests as a part of "clean" recipe
• Remove bogus license file as it is prepended to all the files

Apart from the above, I have also updated the directory structure by placing all the files (previously in the directory "src/") in the project root (like all other tools located in "src/tools/tools"). So now, "make" can be run directly from inside "src/tools/tools/smoketestsuite". Relevant updates were made in the instructions in README.

On a side-note, when generating the tests via "make run", it will so happen that the tool will start showing failures after generating tests for first 10 utilities. I wrote on this issue briefly sometime back [2] and this problem was largely solved by closing stdin (as suggested by @asomers). But utilities like ed(1) and pax(1) aren't affected by closing stdin, and hence the behavior which was described in the previous link is reproduced. I am currently looking into this problem.
A quick and dirty solution for this (for the time being) is to remove these groff scripts from the locally extracted ones: ed.1, pax.1, passwd.1 ; apart from this issue (and further suggestions from mentors), I have fixed all other issues which I wanted to be done before the tool got merged.

[1]: https://github.com/shivansh/smoketestsuite/compare/adc5f27...67200ab
[2]: https://lists.freebsd.org/pipermail/soc-status/2017-September/001107.html

[shivansh]

[FIX] Avoid adding variable "usage_output" in case of an empty output

[asomers]

tools/tools/smoketestsuite/README

4

Remove markdown formatting

23

Remove markdown formatting

34

"Clone" is alien terminology to SVN users. You should probably just delete this sentence, since anybody reading it has already checked out head.

35

You should fix this. All of the scripts know where they live, and everything in the source tree can be described relative to one of those scripts. Just use "$0" to get the path to the script and make other paths relative to that.

tools/tools/smoketestsuite/add_testcase.cpp

155

Here and elsewhere, you should combine the "}" and "else {" onto a single line.

tools/tools/smoketestsuite/add_testcase.h

2

style(9) says to use C-style comments, not C++-style. That's even true for C++ programs, for example cddl/usr.sbin/zfsd.

tools/tools/smoketestsuite/generate_test.cpp

155

Nice use of C++11 features.

tools/tools/smoketestsuite/scripts/fetch_groff.sh

33 ↗

(On Diff #33242)

Instead of assuming a certain location for $src, just set it like "$0/../../../../..". Likewise, don't use pwd below.

46 ↗

(On Diff #33242)

Why not section 8 ?

tools/tools/smoketestsuite/src/generate_license.cpp

39 ↗

(On Diff #33222)

I would be happier if this could be a command line option instead of a prompt. If you do make it a prompt, then you should make it clear that the "name" is for the copyright owner, who is not necessarily the author. Also, if it's a command line option, then the default value can be id -P | cut -d : -f 8

[shivansh]

tools/tools/smoketestsuite/src/generate_test.cpp

276 ↗

(On Diff #32739)

I have been able to narrow down the issue into this: https://github.com/shivansh/smoketestsuite/blob/svn/src/generate_test.cpp#L279 ; it is not fixed, but I think I might be closer to the fix.

[shivansh]

• Add functionality to kill (child) process when stuck on a blocking read.
  
  Instead of spawning a thread for each utility, we create a (child) shell process for executing the shell command and mux its file descriptors via select() with an introduced timeout for finishing execution.
  
  When pclose() is called on the stream returned by popen(), it waits indefinitely for the created shell process to terminate in case where the shell command waits for the user input via a blocking read (e.g. passwd(1)). Hence, we define a custom function which alongside returning the FILE* stream (as with popen()) also returns the pid of the newly created (child) shell process, which can later be used for signalling.

• Add command line option for specifying copyright owner. ` Usage: ./generate_tests -name <copyright_owner> `

• Avoid using pwd as location of scripts is known. Relative paths are used.

• Fetch section 8 utilities along with section 1 ones.

[shivansh]

Remove boost libs and header files which are no longer required

[shivansh]

A few more changes/updates:

• Generate a tabular-like format for the overall progress during execution.
• Avoid hardcoding section numbers.
• When select() fails and returns -1, cleanup and exit.
• Makefile: update clean recipe.

[asomers]

tools/tools/smoketestsuite/scripts/fetch_groff.sh

37 ↗

(On Diff #33634)

Nope, this still depends on PWD. In order to remove the dependency on PWD, you need to make everything relative to the location of this file. See usr.sbin/makefs/tests/makefs_ffs_tests.sh for an example.

tools/tools/smoketestsuite/utils.cpp

46

STDIN_FILENO and STDOUT_FILENO are defined in unistd.h. Don't redefine them.

185

I find it confusing that a single function takes both pointer and reference arguments. Also, a more general way to write this function would be to omit the type argument and instead return the PID and three file descriptors, like this Ruby gem: https://github.com/ahoward/open4. That way the caller can decide which file descriptors it wants to close and which ones it cares about.

215

Instead of casting here, you should simply declare argv as const char *argv[4]

289

None of the enclosed functions ever throw exceptions, so you can eliminate the try...catch block.

tools/tools/smoketestsuite/utils.h

51

more than 80 characters per line

[shivansh]

Address changes requested by @asomers:

• Update the signature of 'utils::POpen()' The function is generalized to omit the 'type' argument and instead returns the PID of the created shell process and the two file descriptors. This way the caller can decide which file descriptors are required and which ones are to be closed.

• Remove the try...catch block The enclosed functions 'feof()' and 'fgets()' don't throw an exception.

• Introduce line-breaks where 80 character limit is exceeded

• Avoid redefining STDIN_FILENO and STDOUT_FILENO -- defined in unistd.h

[shivansh]

Address changes requested by @asomers:

• [Scripts] Remove dependence on PWD

I'm not sure if this fulfils the purpose, because if I invoke the script from a directory other than tools/tools/smoketestsuite, it won't work (I've only incorporated the use of $0 as was requested in the review). In case this wasn't what @asomers had originally in mind, I'll try to come up with an alternative.

[shivansh]

Some more fixes -

• Kill the (forked) shell process only when select() doesn't return within TIMEOUT Previously, SIGTERM was being sent in any case after TIMEOUT to the pid of the shell process. This can be fatal in cases when the shell process cleanly exits before TIMEOUT and its pid is re-used.
• Remove the groff script for pax(1) for which the tool hangs waiting for user input The shell process spawned for executing pax(1) doesn't seem to respond to SIGTERM.
• Free allocated memory
• Remove 'typedef's

[shivansh]

Some more fixes and updates -

• Display number of generated and pending testcases during runtime
• [Logging] Add a gcc variadic macro to conditionally compile debug printing
• [Logging] Move calls to 'perror()' inside 'DEBUGP()'
• Sleep for 0.1 second after finishing test generation for a utility
• Update TIMEOUT to 1 second from 2 seconds
• Avoid exiting when 'select()' encounters an error (and returns -1)
• Wrap around the test descriptions of the generated testcases exceeding the 80-character limit
• [Scripts] Remove utilities which generate a segmentation fault during execution
• [Scripts] Remove non-executable utilities
• [Scripts] Copy the groff scripts only for the utilities which do not contain tests

[shivansh]

Some more fixes -

• Fix (mysterious) segmentation fault encountered for some utilities -- turns out I made a blunder (hah!). Lesson learnt: NULL and "" are not equivalent.
• Add logging functionality for perror() -- messages produced by perror() are now only available in debug mode, otherwise it disrupts the (rather good-looking) tabular format.

[asomers]

tools/tools/smoketestsuite/generate_test.cpp

72

Conventional style for comments this long is to put them in the line above what they describe. And if they stretch over multiple lines, the leading /* and trailing */ get their own lines.

170

This will generate garbage output when directed to a file, or maybe even a serial console. If you're going to do clever stuff like this, you should probably conditionalize it on isatty(fileno(stderr)). That will take care of the case where output is redirected to a pipe or a file.

281

What's the sleep for?

tools/tools/smoketestsuite/utils.cpp

212

You leak memory and file descriptors here.

292

Are you sure this is the right file descriptor? It looks to me like you're trying to read from the child's STDIN instead of its STDOUT.

304

s/since few/since a few/

tools/tools/smoketestsuite/utils.h

60

This comment should be on its own line too.

64

And this one.

[shivansh]

tools/tools/smoketestsuite/utils.cpp

292

(Writing the following detailed description for my future reference too as for a minute I got confused).

The segment where the file descriptors are duplicated -

case 0:                 /* Child. */
        /*
         * TODO Verify if the following operations on both read
         * and write file-descriptors (irrespective of the value
         * of 'type', which is no longer being used) is safe.
         */
        if (pdes[WRITE] != STDOUT_FILENO)
                dup2(pdes[WRITE], STDOUT_FILENO);
        else
                fcntl(pdes[WRITE], F_SETFD, 0);
        if (pdes[READ] != STDIN_FILENO)
                dup2(pdes[READ], STDIN_FILENO);

In the above segment, data which the child will write to its STDOUT will be written to pdes[WRITE], which will then appear on pdes[READ] from where it can be read by the parent process.

Referencing from the man page for pipe(2) -

By convention, the first descriptor is normally used as the read end of
the pipe, and the second is normally the write end, so that data written
to fildes[1] appears on (i.e., can be read from) fildes[0].

For being sure, I confirmed the above by using pipe_descr->writefd instead of pipe_descr->readfd for generating a test for ls(1) -- the generated testcase gets an empty output (unlike currently where the output correctly lists the files and directories in pwd).

[shivansh]

Address changes requested by @asomers -

• Handle the case when output is redirected to pipe or a file
• Fix fd and memory leak when vfork() fails
• Update multi-line comments to single-line where applicable

[shivansh]

Some more fixes and updates -

• Add subroutine to update groff directory -- this is much faster than the script 'fetch_utils.sh' and removes the requirement of running make fetch_groff.
• Remove (now unrequired) "fetch_groff.sh" ; Update instructions.
• Sandbox utility-specific commands to restrict introduced side-effects -- All the utility-specific commands are executed inside a temporary directory such that all the side-effects (core dumps, generated executables etc.) that are generated will be restricted inside this directory. This directory is created on every invocation of the tool and removed after the tool finishes generating all the tests.
• Add interrupt handler for SIGINT

[shivansh]

Add "batch mode" for test generation

Instead of generating tests for all the utilities, batch mode allows
generation of tests for first batch_limit (specified by tester) number
of utilities selected from scripts/utils_list. The generated test is
then placed at its correct location in the src tree and the corresponding
makefile is also created.

This will be useful in quickly setting up a diff for a fixed number of
generated tests to be sent for review.

This is mode optional and the tester can still use the original mode for
generating tests for all the utilities (the choice is made at runtime).

[shivansh]

Summary of changes: https://github.com/shivansh/smoketestsuite/compare/28ba84e...90006f3

[shivansh]

Remove generated file

This file wasn't supposed to be committed, my apologies for the noise.