Command line test runner
up vote
2
down vote
favorite
This is a simple program to run a command-line program with given parameters and verify that it writes the expected output to stdout or stderr. The specification file is passed as the first argument. It's formatted like this:
echo foo -> foo
man -> What manual page do you want?
with program and arguments to the left of -> and output on the right.
I don't often work so low-level, so I'm most interested in whether I'm managing resources properly.
#define _GNU_SOURCE
#include <errno.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#define MAX_ARGS 10
#define MAX_LINE 256
const char* parse_args(char **args, const char *line)
{
size_t index = 0;
size_t delimiter_index = 0;
while (args[index] != NULL && index < MAX_ARGS - 1)
{
++index;
args[index] = strtok(NULL, " ");
if (args[index] && strncmp(args[index], "->", 2) == 0)
{
delimiter_index = index;
args[index+1] = strtok(NULL, "n");
break;
}
}
args[delimiter_index] = '';
const char *expected_result = args[delimiter_index + 1];
if (delimiter_index == 0 || expected_result == NULL)
{
fprintf(stderr, "Line <%s> is malformedn", line);
return NULL;
}
if (strlen(expected_result) > MAX_LINE)
{
fprintf(stderr,
"Invalid specification. Only results less than %d chars supported",
MAX_LINE);
return NULL;
}
return expected_result;
}
bool run_test(char *line)
{
char *args[MAX_ARGS] = {};
args[0] = strtok(line, " ");
if (!args[0]) return true;
const char* expected_result = parse_args(args, line);
if (!expected_result)
{
return false;
}
int stdout_pipe[2];
if (pipe(stdout_pipe) != 0)
{
fprintf(stderr, "Unable to open pipen");
return false;
}
pid_t pid = fork();
if (pid == -1)
{
fprintf(stderr, "Unable to fork processn");
return false;
}
if (pid == 0)
{
// Redirect streams so caller can read stdout
dup2(stdout_pipe[1], STDOUT_FILENO);
dup2(stdout_pipe[1], STDERR_FILENO);
close(stdout_pipe[0]);
execvp(args[0], args);
// exec* only ever returns if it fails
fprintf(stderr, "Failed to execute test program: %sn",
strerror(errno));
return false;
}
close(stdout_pipe[1]);
char actual_result[MAX_LINE] = {};
ssize_t bytes_read = read(stdout_pipe[0], actual_result, MAX_LINE);
if (bytes_read == -1)
{
fprintf(stderr, "Unable to read program outputn");
return false;
}
// Strip newline
if (actual_result[bytes_read - 1] == 'n')
{
actual_result[bytes_read - 1] = '';
}
if (strcmp(actual_result, expected_result) != 0)
{
fprintf(stderr, "Test Failed: ");
for (int arg = 0; arg < MAX_ARGS; ++ arg)
{
if (!args[arg]) break;
fprintf(stderr, "%s ", args[arg]);
}
fprintf(stderr, "n");
fprintf(stderr, "Expected: %sn", expected_result);
fprintf(stderr, "Actual: %sn", actual_result);
return false;
}
return true;
}
int main(int argc, char *argv)
{
if (argc != 2)
{
fprintf(stderr, "Usage: %s <test_specification>n", argv[0]);
return 1;
}
const char *spec_pathname = argv[1];
FILE* spec = fopen(spec_pathname, "r");
if (!spec)
{
fprintf(stderr, "Cannot open <%s> for readingn", spec_pathname);
return 1;
}
int return_code = 0;
char *line = NULL;
size_t line_length = 0;
while (getline(&line, &line_length, spec) != -1)
{
if (!run_test(line))
{
return_code = 1;
}
}
free(line);
return return_code;
}
c unix c99
add a comment |
up vote
2
down vote
favorite
This is a simple program to run a command-line program with given parameters and verify that it writes the expected output to stdout or stderr. The specification file is passed as the first argument. It's formatted like this:
echo foo -> foo
man -> What manual page do you want?
with program and arguments to the left of -> and output on the right.
I don't often work so low-level, so I'm most interested in whether I'm managing resources properly.
#define _GNU_SOURCE
#include <errno.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#define MAX_ARGS 10
#define MAX_LINE 256
const char* parse_args(char **args, const char *line)
{
size_t index = 0;
size_t delimiter_index = 0;
while (args[index] != NULL && index < MAX_ARGS - 1)
{
++index;
args[index] = strtok(NULL, " ");
if (args[index] && strncmp(args[index], "->", 2) == 0)
{
delimiter_index = index;
args[index+1] = strtok(NULL, "n");
break;
}
}
args[delimiter_index] = '';
const char *expected_result = args[delimiter_index + 1];
if (delimiter_index == 0 || expected_result == NULL)
{
fprintf(stderr, "Line <%s> is malformedn", line);
return NULL;
}
if (strlen(expected_result) > MAX_LINE)
{
fprintf(stderr,
"Invalid specification. Only results less than %d chars supported",
MAX_LINE);
return NULL;
}
return expected_result;
}
bool run_test(char *line)
{
char *args[MAX_ARGS] = {};
args[0] = strtok(line, " ");
if (!args[0]) return true;
const char* expected_result = parse_args(args, line);
if (!expected_result)
{
return false;
}
int stdout_pipe[2];
if (pipe(stdout_pipe) != 0)
{
fprintf(stderr, "Unable to open pipen");
return false;
}
pid_t pid = fork();
if (pid == -1)
{
fprintf(stderr, "Unable to fork processn");
return false;
}
if (pid == 0)
{
// Redirect streams so caller can read stdout
dup2(stdout_pipe[1], STDOUT_FILENO);
dup2(stdout_pipe[1], STDERR_FILENO);
close(stdout_pipe[0]);
execvp(args[0], args);
// exec* only ever returns if it fails
fprintf(stderr, "Failed to execute test program: %sn",
strerror(errno));
return false;
}
close(stdout_pipe[1]);
char actual_result[MAX_LINE] = {};
ssize_t bytes_read = read(stdout_pipe[0], actual_result, MAX_LINE);
if (bytes_read == -1)
{
fprintf(stderr, "Unable to read program outputn");
return false;
}
// Strip newline
if (actual_result[bytes_read - 1] == 'n')
{
actual_result[bytes_read - 1] = '';
}
if (strcmp(actual_result, expected_result) != 0)
{
fprintf(stderr, "Test Failed: ");
for (int arg = 0; arg < MAX_ARGS; ++ arg)
{
if (!args[arg]) break;
fprintf(stderr, "%s ", args[arg]);
}
fprintf(stderr, "n");
fprintf(stderr, "Expected: %sn", expected_result);
fprintf(stderr, "Actual: %sn", actual_result);
return false;
}
return true;
}
int main(int argc, char *argv)
{
if (argc != 2)
{
fprintf(stderr, "Usage: %s <test_specification>n", argv[0]);
return 1;
}
const char *spec_pathname = argv[1];
FILE* spec = fopen(spec_pathname, "r");
if (!spec)
{
fprintf(stderr, "Cannot open <%s> for readingn", spec_pathname);
return 1;
}
int return_code = 0;
char *line = NULL;
size_t line_length = 0;
while (getline(&line, &line_length, spec) != -1)
{
if (!run_test(line))
{
return_code = 1;
}
}
free(line);
return return_code;
}
c unix c99
add a comment |
up vote
2
down vote
favorite
up vote
2
down vote
favorite
This is a simple program to run a command-line program with given parameters and verify that it writes the expected output to stdout or stderr. The specification file is passed as the first argument. It's formatted like this:
echo foo -> foo
man -> What manual page do you want?
with program and arguments to the left of -> and output on the right.
I don't often work so low-level, so I'm most interested in whether I'm managing resources properly.
#define _GNU_SOURCE
#include <errno.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#define MAX_ARGS 10
#define MAX_LINE 256
const char* parse_args(char **args, const char *line)
{
size_t index = 0;
size_t delimiter_index = 0;
while (args[index] != NULL && index < MAX_ARGS - 1)
{
++index;
args[index] = strtok(NULL, " ");
if (args[index] && strncmp(args[index], "->", 2) == 0)
{
delimiter_index = index;
args[index+1] = strtok(NULL, "n");
break;
}
}
args[delimiter_index] = '';
const char *expected_result = args[delimiter_index + 1];
if (delimiter_index == 0 || expected_result == NULL)
{
fprintf(stderr, "Line <%s> is malformedn", line);
return NULL;
}
if (strlen(expected_result) > MAX_LINE)
{
fprintf(stderr,
"Invalid specification. Only results less than %d chars supported",
MAX_LINE);
return NULL;
}
return expected_result;
}
bool run_test(char *line)
{
char *args[MAX_ARGS] = {};
args[0] = strtok(line, " ");
if (!args[0]) return true;
const char* expected_result = parse_args(args, line);
if (!expected_result)
{
return false;
}
int stdout_pipe[2];
if (pipe(stdout_pipe) != 0)
{
fprintf(stderr, "Unable to open pipen");
return false;
}
pid_t pid = fork();
if (pid == -1)
{
fprintf(stderr, "Unable to fork processn");
return false;
}
if (pid == 0)
{
// Redirect streams so caller can read stdout
dup2(stdout_pipe[1], STDOUT_FILENO);
dup2(stdout_pipe[1], STDERR_FILENO);
close(stdout_pipe[0]);
execvp(args[0], args);
// exec* only ever returns if it fails
fprintf(stderr, "Failed to execute test program: %sn",
strerror(errno));
return false;
}
close(stdout_pipe[1]);
char actual_result[MAX_LINE] = {};
ssize_t bytes_read = read(stdout_pipe[0], actual_result, MAX_LINE);
if (bytes_read == -1)
{
fprintf(stderr, "Unable to read program outputn");
return false;
}
// Strip newline
if (actual_result[bytes_read - 1] == 'n')
{
actual_result[bytes_read - 1] = '';
}
if (strcmp(actual_result, expected_result) != 0)
{
fprintf(stderr, "Test Failed: ");
for (int arg = 0; arg < MAX_ARGS; ++ arg)
{
if (!args[arg]) break;
fprintf(stderr, "%s ", args[arg]);
}
fprintf(stderr, "n");
fprintf(stderr, "Expected: %sn", expected_result);
fprintf(stderr, "Actual: %sn", actual_result);
return false;
}
return true;
}
int main(int argc, char *argv)
{
if (argc != 2)
{
fprintf(stderr, "Usage: %s <test_specification>n", argv[0]);
return 1;
}
const char *spec_pathname = argv[1];
FILE* spec = fopen(spec_pathname, "r");
if (!spec)
{
fprintf(stderr, "Cannot open <%s> for readingn", spec_pathname);
return 1;
}
int return_code = 0;
char *line = NULL;
size_t line_length = 0;
while (getline(&line, &line_length, spec) != -1)
{
if (!run_test(line))
{
return_code = 1;
}
}
free(line);
return return_code;
}
c unix c99
This is a simple program to run a command-line program with given parameters and verify that it writes the expected output to stdout or stderr. The specification file is passed as the first argument. It's formatted like this:
echo foo -> foo
man -> What manual page do you want?
with program and arguments to the left of -> and output on the right.
I don't often work so low-level, so I'm most interested in whether I'm managing resources properly.
#define _GNU_SOURCE
#include <errno.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#define MAX_ARGS 10
#define MAX_LINE 256
const char* parse_args(char **args, const char *line)
{
size_t index = 0;
size_t delimiter_index = 0;
while (args[index] != NULL && index < MAX_ARGS - 1)
{
++index;
args[index] = strtok(NULL, " ");
if (args[index] && strncmp(args[index], "->", 2) == 0)
{
delimiter_index = index;
args[index+1] = strtok(NULL, "n");
break;
}
}
args[delimiter_index] = '';
const char *expected_result = args[delimiter_index + 1];
if (delimiter_index == 0 || expected_result == NULL)
{
fprintf(stderr, "Line <%s> is malformedn", line);
return NULL;
}
if (strlen(expected_result) > MAX_LINE)
{
fprintf(stderr,
"Invalid specification. Only results less than %d chars supported",
MAX_LINE);
return NULL;
}
return expected_result;
}
bool run_test(char *line)
{
char *args[MAX_ARGS] = {};
args[0] = strtok(line, " ");
if (!args[0]) return true;
const char* expected_result = parse_args(args, line);
if (!expected_result)
{
return false;
}
int stdout_pipe[2];
if (pipe(stdout_pipe) != 0)
{
fprintf(stderr, "Unable to open pipen");
return false;
}
pid_t pid = fork();
if (pid == -1)
{
fprintf(stderr, "Unable to fork processn");
return false;
}
if (pid == 0)
{
// Redirect streams so caller can read stdout
dup2(stdout_pipe[1], STDOUT_FILENO);
dup2(stdout_pipe[1], STDERR_FILENO);
close(stdout_pipe[0]);
execvp(args[0], args);
// exec* only ever returns if it fails
fprintf(stderr, "Failed to execute test program: %sn",
strerror(errno));
return false;
}
close(stdout_pipe[1]);
char actual_result[MAX_LINE] = {};
ssize_t bytes_read = read(stdout_pipe[0], actual_result, MAX_LINE);
if (bytes_read == -1)
{
fprintf(stderr, "Unable to read program outputn");
return false;
}
// Strip newline
if (actual_result[bytes_read - 1] == 'n')
{
actual_result[bytes_read - 1] = '';
}
if (strcmp(actual_result, expected_result) != 0)
{
fprintf(stderr, "Test Failed: ");
for (int arg = 0; arg < MAX_ARGS; ++ arg)
{
if (!args[arg]) break;
fprintf(stderr, "%s ", args[arg]);
}
fprintf(stderr, "n");
fprintf(stderr, "Expected: %sn", expected_result);
fprintf(stderr, "Actual: %sn", actual_result);
return false;
}
return true;
}
int main(int argc, char *argv)
{
if (argc != 2)
{
fprintf(stderr, "Usage: %s <test_specification>n", argv[0]);
return 1;
}
const char *spec_pathname = argv[1];
FILE* spec = fopen(spec_pathname, "r");
if (!spec)
{
fprintf(stderr, "Cannot open <%s> for readingn", spec_pathname);
return 1;
}
int return_code = 0;
char *line = NULL;
size_t line_length = 0;
while (getline(&line, &line_length, spec) != -1)
{
if (!run_test(line))
{
return_code = 1;
}
}
free(line);
return return_code;
}
c unix c99
c unix c99
asked Dec 12 at 18:27
User319
734413
734413
add a comment |
add a comment |
3 Answers
3
active
oldest
votes
up vote
3
down vote
accepted
fprintf(stderr, "Unable to read program outputn");
loses important information: exactly why the program output couldn't be read. Preferperror
.
Ditto for other
fprintf(stderr, ....)
.
The parent doesn't
wait
for children. You may produce plenty of zombies.The parent doesn't
close(stdout_pipe[0]);
when done with a child. You may run out of file descriptors.
The spec file format seems naive:
- There is no way to deal with a multi-line output.
- It mixes child's
stdout
andstderr
. If the child produces both, their order is unpredictable. - It doesn't specify the child's return status.
add a comment |
up vote
2
down vote
while (args[index] != NULL && index < MAX_ARGS - 1)
I suggest that you refactor this loop a little bit; here's one option:
for (size_t index = 1; index < MAX_ARGS && args[index]; index++) { // ...
This:
args[delimiter_index] = '';
if it works without warning, that's only by accident. You're confusing pointer assignment with character assignment. It should actually be
args[delimiter_index] = NULL;
In all likelihood, both parse_args
and run_test
should be static
because no one else is importing them.
For code like
if (pipe(stdout_pipe) != 0)
You should strongly consider calling perror
to get a human-readable error string.
You can combine these fprintf
calls and still have the strings on separate lines:
fprintf(stderr, "n");
fprintf(stderr, "Expected: %sn", expected_result);
fprintf(stderr, "Actual: %sn", actual_result);
can be something like
fprintf(stderr, "n"
"Expected: %sn"
"Actual: %sn", expected_result, actual_result);
add a comment |
up vote
1
down vote
Conformance
A couple of violations of Standard C:
209552.c: In function ‘run_test’:
209552.c:52:28: warning: ISO C forbids empty initializer braces [-Wpedantic]
char *args[MAX_ARGS] = {};
^
209552.c:92:36: warning: ISO C forbids empty initializer braces [-Wpedantic]
char actual_result[MAX_LINE] = {};
^
Another pedantic point, which you're probably aware of: this clearly targets POSIX systems, where we are guaranteed that argc
is at least 1, and so argv[0]
is always usable; however, when writing portable programs, code like this can be dangerous:
if (argc != 2)
{
fprintf(stderr, "Usage: %s <test_specification>n", argv[0]);
return 1;
}
Input file format
There are some severe limitations to the input file format. It's impossible to specify commands or arguments that contain space characters, and it's impossible to specify output of more than one line.
Addressing the last point first, perhaps we should consider writing one file per test, with the command as first line, and then all subsequent lines being the expected output. Adapt the program to read loop over the command-line arguments, reading every file that's specified. It's easy for users to test lots of commands, using wildcards (e.g. run_tests *.test
).
As for the first problem, we could consider using a shell to parse it instead of strtok()
- just take the whole command and pass it to /bin/sh -c
.
Perhaps we also want to check the exit status of the program under test - I think that's an important part of the program's interface.
Output format
For syntax errors in the file format, we could improve the error message by writing the file name and line number, rather than just the contents. This would then be consistent with error messages from compilers and other tools, which can be parsed (such as in Emacs, where goto-error
will take the user directly to the problem line).
Parsing function
The parsing might be simpler if we first divide at ->
(or newline, in my proposed input format), and then process the input and output sides separately. We really should emit a good error when the MAX_ARGS
limit is violated (as we do for MAX_LINE
). A worthwhile enhancement would be to eliminate these arbitrary limits.
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209552%2fcommand-line-test-runner%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
3
down vote
accepted
fprintf(stderr, "Unable to read program outputn");
loses important information: exactly why the program output couldn't be read. Preferperror
.
Ditto for other
fprintf(stderr, ....)
.
The parent doesn't
wait
for children. You may produce plenty of zombies.The parent doesn't
close(stdout_pipe[0]);
when done with a child. You may run out of file descriptors.
The spec file format seems naive:
- There is no way to deal with a multi-line output.
- It mixes child's
stdout
andstderr
. If the child produces both, their order is unpredictable. - It doesn't specify the child's return status.
add a comment |
up vote
3
down vote
accepted
fprintf(stderr, "Unable to read program outputn");
loses important information: exactly why the program output couldn't be read. Preferperror
.
Ditto for other
fprintf(stderr, ....)
.
The parent doesn't
wait
for children. You may produce plenty of zombies.The parent doesn't
close(stdout_pipe[0]);
when done with a child. You may run out of file descriptors.
The spec file format seems naive:
- There is no way to deal with a multi-line output.
- It mixes child's
stdout
andstderr
. If the child produces both, their order is unpredictable. - It doesn't specify the child's return status.
add a comment |
up vote
3
down vote
accepted
up vote
3
down vote
accepted
fprintf(stderr, "Unable to read program outputn");
loses important information: exactly why the program output couldn't be read. Preferperror
.
Ditto for other
fprintf(stderr, ....)
.
The parent doesn't
wait
for children. You may produce plenty of zombies.The parent doesn't
close(stdout_pipe[0]);
when done with a child. You may run out of file descriptors.
The spec file format seems naive:
- There is no way to deal with a multi-line output.
- It mixes child's
stdout
andstderr
. If the child produces both, their order is unpredictable. - It doesn't specify the child's return status.
fprintf(stderr, "Unable to read program outputn");
loses important information: exactly why the program output couldn't be read. Preferperror
.
Ditto for other
fprintf(stderr, ....)
.
The parent doesn't
wait
for children. You may produce plenty of zombies.The parent doesn't
close(stdout_pipe[0]);
when done with a child. You may run out of file descriptors.
The spec file format seems naive:
- There is no way to deal with a multi-line output.
- It mixes child's
stdout
andstderr
. If the child produces both, their order is unpredictable. - It doesn't specify the child's return status.
answered Dec 12 at 20:49
vnp
38.4k13096
38.4k13096
add a comment |
add a comment |
up vote
2
down vote
while (args[index] != NULL && index < MAX_ARGS - 1)
I suggest that you refactor this loop a little bit; here's one option:
for (size_t index = 1; index < MAX_ARGS && args[index]; index++) { // ...
This:
args[delimiter_index] = '';
if it works without warning, that's only by accident. You're confusing pointer assignment with character assignment. It should actually be
args[delimiter_index] = NULL;
In all likelihood, both parse_args
and run_test
should be static
because no one else is importing them.
For code like
if (pipe(stdout_pipe) != 0)
You should strongly consider calling perror
to get a human-readable error string.
You can combine these fprintf
calls and still have the strings on separate lines:
fprintf(stderr, "n");
fprintf(stderr, "Expected: %sn", expected_result);
fprintf(stderr, "Actual: %sn", actual_result);
can be something like
fprintf(stderr, "n"
"Expected: %sn"
"Actual: %sn", expected_result, actual_result);
add a comment |
up vote
2
down vote
while (args[index] != NULL && index < MAX_ARGS - 1)
I suggest that you refactor this loop a little bit; here's one option:
for (size_t index = 1; index < MAX_ARGS && args[index]; index++) { // ...
This:
args[delimiter_index] = '';
if it works without warning, that's only by accident. You're confusing pointer assignment with character assignment. It should actually be
args[delimiter_index] = NULL;
In all likelihood, both parse_args
and run_test
should be static
because no one else is importing them.
For code like
if (pipe(stdout_pipe) != 0)
You should strongly consider calling perror
to get a human-readable error string.
You can combine these fprintf
calls and still have the strings on separate lines:
fprintf(stderr, "n");
fprintf(stderr, "Expected: %sn", expected_result);
fprintf(stderr, "Actual: %sn", actual_result);
can be something like
fprintf(stderr, "n"
"Expected: %sn"
"Actual: %sn", expected_result, actual_result);
add a comment |
up vote
2
down vote
up vote
2
down vote
while (args[index] != NULL && index < MAX_ARGS - 1)
I suggest that you refactor this loop a little bit; here's one option:
for (size_t index = 1; index < MAX_ARGS && args[index]; index++) { // ...
This:
args[delimiter_index] = '';
if it works without warning, that's only by accident. You're confusing pointer assignment with character assignment. It should actually be
args[delimiter_index] = NULL;
In all likelihood, both parse_args
and run_test
should be static
because no one else is importing them.
For code like
if (pipe(stdout_pipe) != 0)
You should strongly consider calling perror
to get a human-readable error string.
You can combine these fprintf
calls and still have the strings on separate lines:
fprintf(stderr, "n");
fprintf(stderr, "Expected: %sn", expected_result);
fprintf(stderr, "Actual: %sn", actual_result);
can be something like
fprintf(stderr, "n"
"Expected: %sn"
"Actual: %sn", expected_result, actual_result);
while (args[index] != NULL && index < MAX_ARGS - 1)
I suggest that you refactor this loop a little bit; here's one option:
for (size_t index = 1; index < MAX_ARGS && args[index]; index++) { // ...
This:
args[delimiter_index] = '';
if it works without warning, that's only by accident. You're confusing pointer assignment with character assignment. It should actually be
args[delimiter_index] = NULL;
In all likelihood, both parse_args
and run_test
should be static
because no one else is importing them.
For code like
if (pipe(stdout_pipe) != 0)
You should strongly consider calling perror
to get a human-readable error string.
You can combine these fprintf
calls and still have the strings on separate lines:
fprintf(stderr, "n");
fprintf(stderr, "Expected: %sn", expected_result);
fprintf(stderr, "Actual: %sn", actual_result);
can be something like
fprintf(stderr, "n"
"Expected: %sn"
"Actual: %sn", expected_result, actual_result);
answered Dec 12 at 19:31
Reinderien
2,062616
2,062616
add a comment |
add a comment |
up vote
1
down vote
Conformance
A couple of violations of Standard C:
209552.c: In function ‘run_test’:
209552.c:52:28: warning: ISO C forbids empty initializer braces [-Wpedantic]
char *args[MAX_ARGS] = {};
^
209552.c:92:36: warning: ISO C forbids empty initializer braces [-Wpedantic]
char actual_result[MAX_LINE] = {};
^
Another pedantic point, which you're probably aware of: this clearly targets POSIX systems, where we are guaranteed that argc
is at least 1, and so argv[0]
is always usable; however, when writing portable programs, code like this can be dangerous:
if (argc != 2)
{
fprintf(stderr, "Usage: %s <test_specification>n", argv[0]);
return 1;
}
Input file format
There are some severe limitations to the input file format. It's impossible to specify commands or arguments that contain space characters, and it's impossible to specify output of more than one line.
Addressing the last point first, perhaps we should consider writing one file per test, with the command as first line, and then all subsequent lines being the expected output. Adapt the program to read loop over the command-line arguments, reading every file that's specified. It's easy for users to test lots of commands, using wildcards (e.g. run_tests *.test
).
As for the first problem, we could consider using a shell to parse it instead of strtok()
- just take the whole command and pass it to /bin/sh -c
.
Perhaps we also want to check the exit status of the program under test - I think that's an important part of the program's interface.
Output format
For syntax errors in the file format, we could improve the error message by writing the file name and line number, rather than just the contents. This would then be consistent with error messages from compilers and other tools, which can be parsed (such as in Emacs, where goto-error
will take the user directly to the problem line).
Parsing function
The parsing might be simpler if we first divide at ->
(or newline, in my proposed input format), and then process the input and output sides separately. We really should emit a good error when the MAX_ARGS
limit is violated (as we do for MAX_LINE
). A worthwhile enhancement would be to eliminate these arbitrary limits.
add a comment |
up vote
1
down vote
Conformance
A couple of violations of Standard C:
209552.c: In function ‘run_test’:
209552.c:52:28: warning: ISO C forbids empty initializer braces [-Wpedantic]
char *args[MAX_ARGS] = {};
^
209552.c:92:36: warning: ISO C forbids empty initializer braces [-Wpedantic]
char actual_result[MAX_LINE] = {};
^
Another pedantic point, which you're probably aware of: this clearly targets POSIX systems, where we are guaranteed that argc
is at least 1, and so argv[0]
is always usable; however, when writing portable programs, code like this can be dangerous:
if (argc != 2)
{
fprintf(stderr, "Usage: %s <test_specification>n", argv[0]);
return 1;
}
Input file format
There are some severe limitations to the input file format. It's impossible to specify commands or arguments that contain space characters, and it's impossible to specify output of more than one line.
Addressing the last point first, perhaps we should consider writing one file per test, with the command as first line, and then all subsequent lines being the expected output. Adapt the program to read loop over the command-line arguments, reading every file that's specified. It's easy for users to test lots of commands, using wildcards (e.g. run_tests *.test
).
As for the first problem, we could consider using a shell to parse it instead of strtok()
- just take the whole command and pass it to /bin/sh -c
.
Perhaps we also want to check the exit status of the program under test - I think that's an important part of the program's interface.
Output format
For syntax errors in the file format, we could improve the error message by writing the file name and line number, rather than just the contents. This would then be consistent with error messages from compilers and other tools, which can be parsed (such as in Emacs, where goto-error
will take the user directly to the problem line).
Parsing function
The parsing might be simpler if we first divide at ->
(or newline, in my proposed input format), and then process the input and output sides separately. We really should emit a good error when the MAX_ARGS
limit is violated (as we do for MAX_LINE
). A worthwhile enhancement would be to eliminate these arbitrary limits.
add a comment |
up vote
1
down vote
up vote
1
down vote
Conformance
A couple of violations of Standard C:
209552.c: In function ‘run_test’:
209552.c:52:28: warning: ISO C forbids empty initializer braces [-Wpedantic]
char *args[MAX_ARGS] = {};
^
209552.c:92:36: warning: ISO C forbids empty initializer braces [-Wpedantic]
char actual_result[MAX_LINE] = {};
^
Another pedantic point, which you're probably aware of: this clearly targets POSIX systems, where we are guaranteed that argc
is at least 1, and so argv[0]
is always usable; however, when writing portable programs, code like this can be dangerous:
if (argc != 2)
{
fprintf(stderr, "Usage: %s <test_specification>n", argv[0]);
return 1;
}
Input file format
There are some severe limitations to the input file format. It's impossible to specify commands or arguments that contain space characters, and it's impossible to specify output of more than one line.
Addressing the last point first, perhaps we should consider writing one file per test, with the command as first line, and then all subsequent lines being the expected output. Adapt the program to read loop over the command-line arguments, reading every file that's specified. It's easy for users to test lots of commands, using wildcards (e.g. run_tests *.test
).
As for the first problem, we could consider using a shell to parse it instead of strtok()
- just take the whole command and pass it to /bin/sh -c
.
Perhaps we also want to check the exit status of the program under test - I think that's an important part of the program's interface.
Output format
For syntax errors in the file format, we could improve the error message by writing the file name and line number, rather than just the contents. This would then be consistent with error messages from compilers and other tools, which can be parsed (such as in Emacs, where goto-error
will take the user directly to the problem line).
Parsing function
The parsing might be simpler if we first divide at ->
(or newline, in my proposed input format), and then process the input and output sides separately. We really should emit a good error when the MAX_ARGS
limit is violated (as we do for MAX_LINE
). A worthwhile enhancement would be to eliminate these arbitrary limits.
Conformance
A couple of violations of Standard C:
209552.c: In function ‘run_test’:
209552.c:52:28: warning: ISO C forbids empty initializer braces [-Wpedantic]
char *args[MAX_ARGS] = {};
^
209552.c:92:36: warning: ISO C forbids empty initializer braces [-Wpedantic]
char actual_result[MAX_LINE] = {};
^
Another pedantic point, which you're probably aware of: this clearly targets POSIX systems, where we are guaranteed that argc
is at least 1, and so argv[0]
is always usable; however, when writing portable programs, code like this can be dangerous:
if (argc != 2)
{
fprintf(stderr, "Usage: %s <test_specification>n", argv[0]);
return 1;
}
Input file format
There are some severe limitations to the input file format. It's impossible to specify commands or arguments that contain space characters, and it's impossible to specify output of more than one line.
Addressing the last point first, perhaps we should consider writing one file per test, with the command as first line, and then all subsequent lines being the expected output. Adapt the program to read loop over the command-line arguments, reading every file that's specified. It's easy for users to test lots of commands, using wildcards (e.g. run_tests *.test
).
As for the first problem, we could consider using a shell to parse it instead of strtok()
- just take the whole command and pass it to /bin/sh -c
.
Perhaps we also want to check the exit status of the program under test - I think that's an important part of the program's interface.
Output format
For syntax errors in the file format, we could improve the error message by writing the file name and line number, rather than just the contents. This would then be consistent with error messages from compilers and other tools, which can be parsed (such as in Emacs, where goto-error
will take the user directly to the problem line).
Parsing function
The parsing might be simpler if we first divide at ->
(or newline, in my proposed input format), and then process the input and output sides separately. We really should emit a good error when the MAX_ARGS
limit is violated (as we do for MAX_LINE
). A worthwhile enhancement would be to eliminate these arbitrary limits.
answered Dec 13 at 10:30
Toby Speight
23.3k639111
23.3k639111
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Some of your past answers have not been well-received, and you're in danger of being blocked from answering.
Please pay close attention to the following guidance:
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209552%2fcommand-line-test-runner%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown