GNOME Bugzilla – Bug 550433
g_test_init doesn't recognize --help
Last modified: 2012-08-17 01:33:01 UTC
Currently g_test_init recognizes a bunch of arguments that are parsed manually. Ergo -h and --help respectively don't work. The code doesn't make use of GOptionContext.
Created attachment 117842 [details] [review] Add --help output This patch adds recognition of -? and --help to the command line argument parsing in g_test_init and prints an output much like what g_option_context_get_help would have produced. The output is not localized, should it be? One line is specially tweaked so that it looks good on an 80 columns console, is that appropriate?
I'd much rather accept a patch to make use of the option parser that lives next file in the same library. Really, this should not have been accepted in this form...
(In reply to comment #2) > I'd much rather accept a patch to make use of the option parser that lives next > file in the same library. Really, this should not have been accepted in this > form... You mean using GOptionContext. In fact I wanted to do that originally as you can see in the bug description, but, which I should have mentioned, I was pointed out while writing the patch, that to be able to actually test GOptionContext it cannot be used in g_test_init. ie. the code should rely on as few functionality as possible.
Ah well. Thats an argument I have to agree with, grudgingly. Maybe add a little comment explaining this.
Other than that, the patch looks reasonable.
Committed, including a two-liner saying that GOptionContext is not used for a reason.
Comment on attachment 117842 [details] [review] Add --help output Only saw this now and have a few review bits still: >diff --git a/glib/gtestutils.c b/glib/gtestutils.c >index 8c0eb1f..0b77908 100644 >--- a/glib/gtestutils.c >+++ b/glib/gtestutils.c >@@ -347,6 +347,27 @@ parse_args (gint *argc_p, > } > argv[i] = NULL; > } >+ else if (strcmp ("-?", argv[i]) == 0 || strcmp ("--help", argv[i]) == 0) This should definitely support '-h' as --help alias. We do that everywhere else and '-h' is a widely used abbreviatiuon for --help. >+ { >+ printf ("Usage:\n" >+ " %s [OPTION...]\n\n" >+ "Help Options:\n" >+ " -?, --help Show help options\n" >+ "Test Options:\n" >+ " -l List test cases available in a test executable\n" >+ " -seed=RANDOMSEED Provide a random seed to reproduce test\n" The docs from gtester here read: --seed=SEEDSTRING start all tests with random number seed SEEDSTRING Would be nice to keep them consistent. >+ " runs using random numbers\n" >+ " --verbose Run tests verbosely\n" >+ " -q, --quiet Run tests quietly\n" >+ " -p TESTPATH execute all tests matching TESTPATH\n" >+ " -m {perf|slow|thorough|quick} Execute tests according modes\n" >+ " --debug-log debug test logging output\n" Hm, documentiong this option is at least questionable... >+ " -k, --keep-going gtester-specific argument\n" The docs for -k should be removed, it's really only supported by gtester, not the tests themselves (because resuming a failing test requires a new fork). >+ " --GTestLogFD=N gtester-specific argument\n" >+ " --GTestSkipCount=N gtester-specific argument\n", The docs for these two options also should be removed, they are not part of our officially supproted command line interface for test programs (i.e. implementation details). The most important docs for unit tests are actually missing, here're the relevant snippets from gtester -h that are also supported by individual unit tests: Options: -h, --help show this help message -l list paths of available test cases -m=perf, -m=slow, -m=quick -m=thorough run test cases in mode perf, slow/thorough or quick (default) -p=TESTPATH only start test cases matching TESTPATH --seed=SEEDSTRING start all tests with random number seed SEEDSTRING -o=LOGFILE write the test log to LOGFILE -q, --quiet suppress per test output --verbose report success per testcase >+ argv[0]); >+ exit (0); >+ } > } > /* collapse argv */ > e = 1;
(In reply to comment #7) > (From update of attachment 117842 [details] [review] [edit]) > Only saw this now and have a few review bits still: > > >diff --git a/glib/gtestutils.c b/glib/gtestutils.c > >index 8c0eb1f..0b77908 100644 > >--- a/glib/gtestutils.c > >+++ b/glib/gtestutils.c > >@@ -347,6 +347,27 @@ parse_args (gint *argc_p, > > } > > argv[i] = NULL; > > } > >+ else if (strcmp ("-?", argv[i]) == 0 || strcmp ("--help", argv[i]) == 0) > > This should definitely support '-h' as --help alias. We do that everywhere > else and '-h' is a widely used abbreviatiuon for --help. This actually led me to file bug 556706 since unfortunately we do *not* currently do the same help switches, although -h is supported by more Gtk+ tools than -?. In the bug I suggested to advertise -h but also support -?. Thus I shall do so in this bug as well - depending on whether the proposal in bug 556706 is accepted. > >+ { > >+ printf ("Usage:\n" > >+ " %s [OPTION...]\n\n" > >+ "Help Options:\n" > >+ " -?, --help Show help options\n" > >+ "Test Options:\n" > >+ " -l List test cases > > available in a test executable\n" > >+ " -seed=RANDOMSEED Provide a random seed > > to reproduce test\n" > > The docs from gtester here read: > --seed=SEEDSTRING start all tests with random number seed > SEEDSTRING > Would be nice to keep them consistent. Note that gtestutils already has documentation and that's what I based the output on. I agree, it should be fixed either way. > >+ " runs using random numbers\n" > >+ " --verbose Run tests verbosely\n" > >+ " -q, --quiet Run tests quietly\n" > >+ " -p TESTPATH execute all tests matching TESTPATH\n" > >+ " -m {perf|slow|thorough|quick} Execute tests according modes\n" > >+ " --debug-log debug test logging output\n" > > Hm, documentiong this option is at least questionable... > > >+ " -k, --keep-going gtester-specific argument\n" > > The docs for -k should be removed, it's really only supported by gtester, not > the tests themselves (because resuming a failing test requires a new fork). > > >+ " --GTestLogFD=N gtester-specific argument\n" > >+ " --GTestSkipCount=N gtester-specific argument\n", > > The docs for these two options also should be removed, they are not part > of our officially supproted command line interface for test programs (i.e. > implementation details). Well, they are documented in the reference. I conclude that those options should not be in the reference either. It does make sense to me, from the nature of the options. > The most important docs for unit tests are actually missing, here're the > relevant snippets from gtester -h that are also supported by individual unit > tests: > Options: > -h, --help show this help message > -l list paths of available test cases > -m=perf, -m=slow, -m=quick -m=thorough > run test cases in mode perf, slow/thorough or > quick (default) > -p=TESTPATH only start test cases matching TESTPATH > --seed=SEEDSTRING start all tests with random number seed > SEEDSTRING > -o=LOGFILE write the test log to LOGFILE > -q, --quiet suppress per test output > --verbose report success per testcase From what I can see, only these two are missing: > --g-fatal-warnings make warnings fatal (abort) I agree, it should be added, I overlooked that. > -o=LOGFILE write the test log to LOGFILE That's not supported by g_test_init at all. If you think it should be, please file a bug ^_^ My verdict is, the output provided by g_test_init should be improved and after that I suggest to apply according changes to gtester, so that we have the same wording.
(In reply to comment #8) > > >+ else if (strcmp ("-?", argv[i]) == 0 || strcmp ("--help", argv[i]) == 0) > > > > This should definitely support '-h' as --help alias. We do that everywhere > > else and '-h' is a widely used abbreviatiuon for --help. > > This actually led me to file bug 556706 since unfortunately we do *not* > currently do the same help switches, although -h is supported by more Gtk+ > tools than -?. Ok, thanks. Let's not block this bug on the --help issue though, the remaining things here can still be fixed. > > >+ { > > >+ printf ("Usage:\n" > > >+ " %s [OPTION...]\n\n" > > >+ "Help Options:\n" > > >+ " -?, --help Show help options\n" > > >+ "Test Options:\n" > > >+ " -l List test cases > > > available in a test executable\n" > > >+ " -seed=RANDOMSEED Provide a random seed > > > to reproduce test\n" > > > > The docs from gtester here read: > > --seed=SEEDSTRING start all tests with random number seed > > SEEDSTRING > > Would be nice to keep them consistent. > > Note that gtestutils already has documentation and that's what I based the > output on. I agree, it should be fixed either way. gtester is the canonical reference if in doubt ;) You're right about the reference needing adoptions as well then, please include that in your patch. > > >+ " --GTestLogFD=N gtester-specific argument\n" > > >+ " --GTestSkipCount=N gtester-specific argument\n", > > > > The docs for these two options also should be removed, they are not part > > of our officially supproted command line interface for test programs (i.e. > > implementation details). > > Well, they are documented in the reference. Again, the reference needs fixing as well. > I conclude that those options should not be in the reference either. It does > make sense to me, from the nature of the options. I guess the reference *could* mention those including -k if it made clear that those are actually internal/reserved. > > The most important docs for unit tests are actually missing, here're the > > relevant snippets from gtester -h that are also supported by individual unit > > tests: > > Options: > > -h, --help show this help message > > -l list paths of available test cases > > -m=perf, -m=slow, -m=quick -m=thorough > > run test cases in mode perf, slow/thorough or > > quick (default) > > -p=TESTPATH only start test cases matching TESTPATH > > --seed=SEEDSTRING start all tests with random number seed > > SEEDSTRING > > -o=LOGFILE write the test log to LOGFILE > > -q, --quiet suppress per test output > > --verbose report success per testcase > > -o=LOGFILE write the test log to LOGFILE > > That's not supported by g_test_init at all. If you think it should be, please > file a bug ^_^ Sorry, "-o" is bogus, it's a c'n'p error i frogot to remove. > My verdict is, the output provided by g_test_init should be improved and after > that I suggest to apply according changes to gtester, so that we have the same > wording. As mentioned above, gtester should be considered the canonical reference (except for gtester-only options of course), the other doc snippets were contributed or added after the fact, so need fixing.
Created attachment 120780 [details] [review] Improve help output (In reply to comment #9) > (In reply to comment #8) > > > >+ else if (strcmp ("-?", argv[i]) == 0 || strcmp ("--help", argv[i]) == 0) > > > > > > This should definitely support '-h' as --help alias. We do that everywhere > > > else and '-h' is a widely used abbreviatiuon for --help. > > > > This actually led me to file bug 556706 since unfortunately we do *not* > > currently do the same help switches, although -h is supported by more Gtk+ > > tools than -?. > > Ok, thanks. Let's not block this bug on the --help issue though, the remaining > things here can still be fixed. Okay, so here is a patch which does the following: - Accept -h and -?, help output shows '-h, --help'. - Adapt the wording except for "-m" since the gtestutil version is much nicer - Change the format slightly to look like GOptionContext output - Remove -k and and the two internal options - Removed the documentation for those options. I think it doesn't seem all that useful if you practically need the source to understand them. - Actually moved the options around to match getesters order - "Start all tests with random number seed SEEDSTRING" + "Start all tests with random seed SEEDSTRING" I actually left out the 'number' because it doesn't fit my 80 column console otherwise. I think it's still clear enough. If you are fine with those changes I'd like to adjust gtester afterwards with regard to the remaining differences.
Created attachment 120796 [details] [review] Adkjust GTester help format, recognize -? This adjusts GTester according to the previous g-test-utils change.
(In reply to comment #3) > (In reply to comment #2) > > I'd much rather accept a patch to make use of the option parser that lives next > > file in the same library. Really, this should not have been accepted in this > > form... > > You mean using GOptionContext. In fact I wanted to do that originally as you > can see in the bug description, but, which I should have mentioned, I was > pointed out while writing the patch, that to be able to actually test > GOptionContext it cannot be used in g_test_init. ie. the code should rely on as > few functionality as possible. This clearly sucks for libraries that live on top of glib, and want to use GTester. Just like we have gtk_init() and gtk_get_option_group(), we could have a g_test_init() and a g_test_get_option_group().
ping
Pushed a similar patch