On Mon, Mar 14, 2016 at 9:58 AM, David Lamparter <
[email protected]> wrote:
> On Mon, Mar 14, 2016 at 09:32:34AM -0700, Avneesh Sachdev wrote:
> > It looks like we agree that:
> >
> > - test functions should be easier to write.
> >
> > - test code should not be part of a production build (whether via
> > conditional compilation or separate binaries).
> >
> > Given the above, why not let the user use the familiar vtysh interface to
> > invoke tests?
>
> I'm not saying test code shouldn't be run through the CLI. I do think
> dlsym() with a fixed assumed function signature is not a nice way of
> doing things.
>
Using dlsym is a terrible idea for a production build. But, IMHO, so is
allowing unit tests to be invoked.
If we go down the path of a special build, then dlsym doesn't seem too bad,
given that we are talking about people who know how to use gdb :-).
So, to me the higher order bit is how we want to package test functions.
> As I understand it, you're trying to fix 2 things here:
> (a) DEFUN + install_element suckage
> (b) vtysh rebuilding
>
> I think the better option to fix these is to fix them individually.
> (a) could be fixed with some easier to use macro specific for tests
> (b) can be fixed by using telnet, or by using a "catch-all" in vtysh
>
> (a "catch-all" could be "test .LINE" which would make vtysh send the
> command down to the daemons without caring about the arguments; the
> daemon could then implement multiple "test foo A B C" "test bar D E"
> commands which would all work)
>
> The following diff will make all "test ..." commands work in all
> daemons; the daemons should then use DEFUN_NOSH to add specific
> commands (extract.pl need NOT process these).
> => you can add & use test commands without recompiling vtysh
>
>
> diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c
> index 63b596a..b346d6b 100644
> --- a/vtysh/vtysh.c
> +++ b/vtysh/vtysh.c
> @@ -785,6 +785,15 @@ vtysh_end (void)
> }
>
> DEFUNSH (VTYSH_ALL,
> + vtysh_test_all,
> + vtysh_test_all_cmd,
> + "test .LINE",
> + "test commands\ntest commands\n")
> +{
> + return CMD_SUCCESS;
> +}
> +
> +DEFUNSH (VTYSH_ALL,
> vtysh_end_all,
> vtysh_end_all_cmd,
> "end",
> @@ -2450,6 +2459,8 @@ vtysh_init_vty (void)
> install_element (VTY_NODE, &vtysh_exit_line_vty_cmd);
> install_element (VTY_NODE, &vtysh_quit_line_vty_cmd);
>
> + install_element (ENABLE_NODE, &vtysh_test_all_cmd);
> +
> /* "end" command. */
> install_element (CONFIG_NODE, &vtysh_end_all_cmd);
> install_element (ENABLE_NODE, &vtysh_end_all_cmd);
>
Neat. If I understand correctly, the daemon has to dispatch the individual
test functions manually here, though.
> > > My opinion on this is:
> > > - the DEV_BUILD macro is a bad idea. Test code should always be
> > > compiled, or it will fall into bit-rot. We have that problem with
> the
> > > code in tests/ already, people don't update it when changing some
> > > structure and it breaks.
> >
> > Bit rot should not be a problem if a CI system runs the tests.
>
> Yes, but it's still better if this also happens on every developer's
> machine, not only the CI system.
>
If developers are not using a development build, then we've failed ;-).
Jokes apart, this is a fair point. We should consider adding a
'commit-check' script that should be run before submitting code. This would
also be useful for checking other breakage (for instance, 'make dist', and
common configure flags).
> > There is a different argument for separating out the test code, i.e.
> > > not having it in the installed binaries. The programs in tests/ have
> > > that automatically since they're independent programs. For extra
> code
> > > bits in the daemons, the nicest way would be to have them in separate
> > > files which for example get linked into a second binary (like
> > > "zebra/testzebra"). This doesn't work for anything "static" in
> source
> > > files, unfortunately... not sure what the best way there would be.
> > >
> >
> > A CI system can do multiple out-of-source builds: one for production and
> > another for testing. Thereafter, the same automation that is used to run
> > quagga today can be used create various scenarios and invoke test
> > functions. This requires no changes to the makefiles/build system or the
> > quagga automation.
>
> True, but if the same can be done without the extra switch that's IMHO
> preferrable...
>
> And... I'm not even so sure the test code really needs to be disabled.
>
We should plan for success -- in other words, lots of test code!
> There's probably quite a few situations where extra tools are useful on
> user installations when a bug is found. Maybe hidden behind a "_debug
> enable". (depends on what the test function does, of course)
>
This sounds fine if code size is not a problem.
Anyhow, if you guys still have reservations about the vty invoke patches,
let's table them and wait for a better proposal.
Cheers,
Avneesh
> Cheers,
>
>
> -David
>
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
