Re: [PATCH 27/27] avocado: Convert basic pasta tests

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 35957 bytes --]

On Fri, Jul 07, 2023 at 07:42:39PM +0200, Stefano Brivio wrote:
> On Wed, 5 Jul 2023 13:27:17 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jul 05, 2023 at 02:30:48AM +0200, Stefano Brivio wrote:
> > > Sorry for the delay, some (partial) feedback and a few ideas:
> > > 
> > > On Tue, 27 Jun 2023 12:54:28 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > Convert the old-style tests for pasta (DHCP, NDP, TCP and UDP transfers)
> > > > to using avocado.  There are a few differences in what we test, but this
> > > > should generally improve coverage:
> > > > 
> > > >  * We run in a constructed network environment, so we no longer depend on
> > > >    the real host's networking configuration
> > > >  * We do independent setup for each individual test
> > > >  * We add explicit tests for --config-net, which we use to accelerate that
> > > >    setup for the TCP and UDP tests
> > > >  * The TCP and UDP tests now test transfers between the guest and a
> > > >    (simulated) remote site that's on a different network from the simulated
> > > >    pasta host.  This better matches the typical passt/pasta usecase  
> > > 
> > > ...this is not necessarily true -- it really depends, but sure, it's
> > > important to have this too.  
> > 
> > Yeah, I guess not.  My point here is that given that we're generally
> > trying to avoid NAT, it seems to me we should primarily test the
> > no-NAT case, then have specific tests for the NAT cases.  I'll try to
> > find a better way to phrase that.
> 
> I don't think it's really needed, I just wanted to make sure we have a
> common understanding.

Ok.

> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  oldtest/run                   |  14 ++--
> > > >  test/Makefile                 |   1 +
> > > >  test/avocado/pasta.py         | 129 ++++++++++++++++++++++++++++++++++
> > > >  test/lib/layout               |  31 --------
> > > >  test/lib/setup                |  40 -----------
> > > >  test/pasta/dhcp               |  46 ------------
> > > >  test/pasta/ndp                |  33 ---------
> > > >  test/pasta/tcp                |  96 -------------------------
> > > >  test/pasta/udp                |  59 ----------------
> > > >  test/run                      |   8 ---
> > > >  test/tasst/dhcpv6.py          |   4 +-
> > > >  test/tasst/pasta.py           |  42 +++++++++++
> > > >  test/tasst/scenario/simple.py |  44 ++++++------
> > > >  13 files changed, 203 insertions(+), 344 deletions(-)
> > > >  create mode 100644 test/avocado/pasta.py
> > > >  delete mode 100644 test/pasta/dhcp
> > > >  delete mode 100644 test/pasta/ndp
> > > >  delete mode 100644 test/pasta/tcp
> > > >  delete mode 100644 test/pasta/udp
> > > >  create mode 100644 test/tasst/pasta.py
> > > > 
> > > > diff --git a/oldtest/run b/oldtest/run
> > > > index a16bc49b..f1157f90 100755
> > > > --- a/oldtest/run
> > > > +++ b/oldtest/run
> > > > @@ -70,13 +70,13 @@ run() {
> > > >  	test build/clang_tidy
> > > >  	teardown build
> > > >  
> > > > -#	setup pasta
> > > > -#	test pasta/ndp
> > > > -#	test pasta/dhcp
> > > > -#	test pasta/tcp
> > > > -#	test pasta/udp
> > > > -#	test passt/shutdown
> > > > -#	teardown pasta
> > > > +	setup pasta
> > > > +	test pasta/ndp
> > > > +	test pasta/dhcp
> > > > +	test pasta/tcp
> > > > +	test pasta/udp
> > > > +	test passt/shutdown
> > > > +	teardown pasta
> > > >  
> > > >  #	setup pasta_options
> > > >  #	test pasta_options/log_to_file
> > > > diff --git a/test/Makefile b/test/Makefile
> > > > index 953eacf2..9c3114c2 100644
> > > > --- a/test/Makefile
> > > > +++ b/test/Makefile
> > > > @@ -227,6 +227,7 @@ $(VENV):
> > > >  
> > > >  .PHONY: avocado-assets
> > > >  avocado-assets: nstool small.bin medium.bin big.bin
> > > > +	$(MAKE) -C .. pasta
> > > >  
> > > >  .PHONY: avocado
> > > >  avocado: avocado-assets $(VENV)
> > > > diff --git a/test/avocado/pasta.py b/test/avocado/pasta.py
> > > > new file mode 100644
> > > > index 00000000..891313f5
> > > > --- /dev/null
> > > > +++ b/test/avocado/pasta.py  
> > > 
> > > I'm just focusing on this for the moment, as this is the part I'm mostly
> > > concerned about (writing tests), with a particular accent on what makes
> > > this (still) read-only _code_ for me, and the gaps with the kind of
> > > things I would have naturally expected to write.
> > > 
> > > That is, I'm mostly isolating negative aspects here.
> > > 
> > > I'm stressing "code" because I also would have the natural expectation
> > > that it should/must be simpler to _write_ (not necessarily design) tests
> > > rather than the code that end users use, because we can use whatever
> > > language with no strict (only some) constraints on resources and speed.
> > > So in my opinion it doesn't necessarily need to be "code" (and a
> > > general feeling I have from this is that it really is code).  
> > 
> > Right, I see/share your concern.  To some extent there's an
> > unavoidable trade-off here.  Avoiding lots of repetition in the test
> > cases, leads to "code" in some sense: we need something resembling
> > function calls and loops at least to re-invoke standard test pieces.
> 
> True, even though there's also the approach, quite commonly implemented
> in test suites, of mixing actual code with custom directives or
> annotations (e.g. bats does that) to keep things a bit simpler or more
> descriptive. But I can't come up with a reasonably good way here, and
> it's probably doable to also add this later on, maybe in form of
> pre-processing.

Right... there's two aspects to that too.  In bats we have directives
giving the structure of the testsuite - that is listing which test is
which, then the contents of each test is more or less just code.  It
would also be possible to have helper directives within the test,
although in that case it doesn't really differ than much from just
being a function call in whatever language it is.

Is it one of those cases specifically you're looking at?  It would be
not that complex to redesign the "avocado-classless" plugin thingy to
be more declarative in terms of defining tests, which might make it
more appealing to you.  I think the major trade-off of doing so is
that that would force the tests to occupy a different file from
e.g. test library code.  That's no real change for the "real" tests of
passt & pasta, but would make things a bit more clunky for the tests
of the test library.

> > That's not to say we can't make it better than it is in this draft.
> > 
> > > > @@ -0,0 +1,129 @@
> > > > +#! /usr/bin/env avocado-runner-avocado-classless
> > > > +
> > > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > > +#
> > > > +# Copyright Red Hat
> > > > +# Author: David Gibson <david@gibson.dropbear.id.au>
> > > > +
> > > > +"""
> > > > +avocado/pasta.py - Basic tests for pasta mode
> > > > +"""
> > > > +
> > > > +import contextlib
> > > > +import ipaddress
> > > > +import os
> > > > +import tempfile  
> > > 
> > > So far so good, I probably don't need to read up about every single
> > > import.
> > >   
> > > > +from avocado_classless.test import assert_eq, assert_eq_unordered, test  
> > > 
> > > Probably a future source of (needless) copy-and-paste.  
> > 
> > Fair point.  I was considering making the idiom here:
> > 	from avocado_classless.test import *
> > Which will import "all" (in fact a curated list) of things from the
> > module for use here.  I didn't go that way because pylint whinged
> > about it, but we could suppress that easily enough.
> 
> Ah, that's nicer. One would still copy and paste it, but without
> thinking too much.

Ok, I've started moving towards that for the next draft.

> > > Of course,
> > > assert_eq_unordered is not available in the current tests, and we need
> > > it, plus a syntax to represent that.  
> > 
> > I'm not quite sure what you're getting at here.
> 
> I'm just saying that either we "magically" import things by
> pre-processing test scripts or suchlike, or we have an explicit import,
> but no matter what, we need that.
> 
> The current framework 1. doesn't implement anything like that and 2. it
> wouldn't need an "import" anyway because it's a domain-specific
> language. But here, we need that, and probably more, and I guess it's
> reasonable.
> 
> > > Still, conceptually speaking, we shouldn't need this kind of import if
> > > we are writing _a test for passt in the test suite for passt itself_,
> > > and we want to check that the list of interfaces matches "lo" -- it's
> > > something we'll need basically everywhere anyway.
> > >   
> > > > +from tasst.address import LOOPBACK4, LOOPBACK6  
> > > 
> > > On one hand, probably more readable than a 127.0.0.1 here and there
> > > (even though longer than "::1"), on the other hand there are 256
> > > loopback addresses in IPv4. And there is only one way of writing "::1",
> > > but many ways of remembering/misspelling LOOPBACK6.  
> > 
> > Right... so "LOOPBACK6" is longer than "::1", but not than
> > "ipaddress.ip_address('::1')".  That is, these helpers produce
> > something that's in an actual IP address type, rather than a bare
> > string.  I could try moving to just using strings for IP addresses
> > throughout the tests, but that might cause some different messes.
> 
> To me treating it as string doesn't sound like a bad idea, except when
> we want to do calculations/masks (I guess that would be the most
> problematic part?)... but then we could probably convert to "addresses"
> on the fly as needed?

I'll see what I can do along these lines.  One thing that might get
messy is the question of which things want a bare address (127.0.0.1),
and which things what an address+netmask (127.0.0.1/8).  Although,
TBH, that's already kinda messy.

> > > > +from tasst.dhcp import test_dhcp
> > > > +from tasst.dhcpv6 import test_dhcpv6
> > > > +from tasst.ndp import test_ndp
> > > > +from tasst.nstool import unshare_site
> > > > +from tasst.pasta import Pasta
> > > > +from tasst.scenario.simple import (
> > > > +    simple_net, IFNAME, HOST_NET6, IP4, IP6, GW_IP4, REMOTE_IP4, REMOTE_IP6
> > > > +)
> > > > +from tasst.transfer import test_transfers  
> > > 
> > > Similar for this -- it would be great to have an infrastructure where
> > > we can just use what we need without looking it up. Rather minor
> > > though.  
> > 
> > Right, I could introduce "from foo import *" conventions for some of
> > these too.
> > 
> > > > +
> > > > +
> > > > +IN_FWD_PORT = 10002
> > > > +SPLICE_FWD_PORT = 10003
> > > > +FWD_OPTS = f'-t {IN_FWD_PORT} -u {IN_FWD_PORT} \
> > > > +             -T {SPLICE_FWD_PORT} -U {SPLICE_FWD_PORT}'  
> > > 
> > > I think clear enough.
> > >   
> > > > +
> > > > +@contextlib.contextmanager  
> > > 
> > > About this decorator: I now have a vague idea what it means after
> > > reading the full series and some Avocado documentation... but I don't
> > > find that very descriptive.  
> > 
> > Yeah.  I agree that this is uncomfortably cryptic, however this one I
> > don't see an easy way to remove.  I heavily use this to conveniently
> > modify and extend context managers.  It allows two things which are
> > both very convenient:
> > 
> > 1) You can do:
> > 
> > 	@contextlib.contextmanager
> > 	def foo(...):
> > 	    setup()
> > 
> > 	    yield whatever   # <== actual test runs here
> > 
> > 	    cleanup()
> > 
> > this will do the cleanup() even if the test blows up.
> > 
> > 2) You can do
> > 
> > 	@contextlib.contextmanager
> > 	def bar(...):
> > 	    with other_context_1():
> > 	        with other_context_2():
> > 		    yield whatever    # <=== actual test runs here
> > 
> > This effectively makes this a combination of the two existing
> > contexts, again ensuring that both their cleanup happens if the test
> > blows up.
> > 
> > It's certainly possible to do this with direct implementations of the
> > contextmanager protocol but it's much more verbose, and not
> > significantly less esoteric.
> 
> Right -- maybe a comment could help, then.

Hrm.. where, though.  The point is that this is a technique I'm using
heavily throughout, not just in a particular place.

> > > > +def pasta_unconfigured(opts=FWD_OPTS):  
> > > 
> > > Clear.
> > >   
> > > > +    with simple_net() as simnet:  
> > > 
> > > Also clear.
> > >   
> > > > +        with unshare_site('pastans', '-Ucnpf --mount-proc',
> > > > +                          parent=simnet.simhost, sudo=True) as guestns:  
> > > 
> > > If I ignore for a moment the implementation detail, it's not very
> > > intuitive why this is nested in the "with" statement before.  
> > 
> > Hmm.. I'm not sure what's throwing you on this specifically.  We need
> > both pieces of setup for our test, so we need to nest them within each
> > other.  They have to go in this order because the parameters to
> > unshare_site() need something from the simnet.
> 
> Oh, I didn't realise that unshare_site() needs stuff from simple_net().
> -- then this makes sense.

Right.  This (so far) is the case where we explicitly create a guest
ns for pasta, rather than having pasta make one for us.  We want that
to be a child of the ns which is simulating the "host" for pasta, and
that "simhost" ns is created as part of simple_net().

> And yes, this itself would be clearer in my opinion if we then had
> "tempfile.TemporaryDirectory()" on the same level as you mention later,
> because otherwise I'm reading it as something nested, because the
> language wants us to do that. It's not the case, these two are
> conceptually nested.

So, in this case I nested it because I hadn't hit the bits that needed
the tmpdir until this point.  I probably could get both the simple_net
and the tmpdir as part of the same outermost 'with'.  I'll take a look.

> I still think it would be easier to read this nesting in a YAML file or
> suchlike. But of course this part of a different (and larger) problem,
> so maybe we can just live with this for the moment...
> 
> > > Those parameters require a complete understanding of unshare_site(),  
> > 
> > Hm, ok.  I could require that this takes keyword arguments, so it
> > would be something like:
> >     unshare_site(name='pastans', unshare_opts='-Ucnpf --mount-proc',
> >                  ..)
> > 
> > Would that help?	
> 
> Yes, definitely, a bit more to type, but absolutely understandable.

Ok.  I'll move towards using keyword arguments everywhere.

> > > and... I now know what "sudo=True" means in Avocado, but I don't
> > > even have sudo on my machine, so at a distracted look I would think I
> > > need to install it and configure sudoers (which would make me as a
> > > developer skip the tests).  
> > 
> > Yeah.  And it's arguably not entirely accurate either.  I could rework
> > the 'site' stuff so that I call this, say, 'capable' or 'keep_caps'
> > instead.  Would that help?
> 
> 'keep_caps' does what it says, that would definitely help.

Ok, I'll have a look at this.

> > > So far, what this says is:
> > > 
> > > - use addressing from simple_net()  
> > 
> > Hrm, it's more than that: simple_net() is actually *creating* the
> > simulated simple network (and cleaning it up once the with clause
> > ends).
> 
> Yes, okay, fair point.
> 
> > > - detach user and network namespace, remounting proc in it, preserve (I
> > >   guess?) credentials  
> > 
> > Well, if by "preserve credentials" you mean "keep the capabilities you
> > have because you own the namespace", yes (this is equivalent to
> > "nsenter --keep-caps", not "nsenter --preserve-credentials").
> 
> Right, sorry, I meant capabilities. While at it, would it perhaps make
> sense to build unshare(1) options in unshare_site() itself?

You mean take some sort of Python description of what we want to
unshare, then generate the unshare(1) options from that?  I guess we
could, I'm just a bit dubious about the worth of doing that when we
don't actually need to do anything other than pass some options
verbatim to unshare(1).

> > > > +            with tempfile.TemporaryDirectory() as tmpdir:  
> > > 
> > > ...even less intuitive why this statement (tmpdir="$(mktemp -d)") is
> > > nested.  
> > 
> > So it's not quite equivalent to tmpdir=$(mktemp -d): it does that at
> > the start of the with, but it also removes the directory at the end of
> > the with.
> 
> Okay, fair, POSIX shell doesn't have that (it has traps, but you need
> to pop/push stuff from them, which means it's less terse anyway).

Right, and it's at the granularity of a whole script/process.  'with'
gives us full control of the scope in which we need the setup.

> > We need the network constructed, the namespace created and the
> > temporary directory available all at the same time.  So the scope of
> > each 'with' context needs to overlap - in other words to be nested.
> > In this case the order of nesting doesn't matter, we could put the
> > temporary directory 'with' on the outside.
> > 
> > >   
> > > > +                pidfile = os.path.join(tmpdir, 'pasta.pid')
> > > > +
> > > > +                with Pasta(simnet.simhost, pidfile, opts, ns=guestns) as pasta:
> > > > +                    yield simnet, pasta.ns  
> > > 
> > > ...and this finally says: start pasta in that (user? network?)
> > > namespace.  
> > 
> > Yes, specifically, start pasta with it's "host" being simnet.simhost,
> > and its "guest" being guestns.  Again I could enforce keyword
> > parameters to make the meaning of the arguments a bit clearer.
> > 
> > [Aside, I plan to extent Pasta() so that if you don't pass a guest ns
> > it will default to having pasta create its own - that ns will still be
> > available as 'pasta.ns', so the rest of the stuff can remain the same]
> > 
> > Then the "yield" essentially means "now that we've done the setup,
> > here is where to run the actual specific test".
> 
> This last part is quite hard for me to grasp. But maybe an
> over-commented test "template" would fix it.
> 
> > > So, from my expectation, pseudocode:
> > > 
> > > setup:
> > >   net: simple_net
> > >   pasta_options: -t 10002 -T 10003 -u 10002 -U 10003
> > >   start pasta  
> > 
> > Hrm, that pseudocode assumes a pretty specific set of possible setups,
> > and how they relate to each other.  For example I can't see how it
> > would extend to running two different pasta instances on different
> > simulated hosts.
> 
> setup:
>    net: not_so_simple_net
>    site_a:
>      pasta_options: -t 10002 -T 10003 -u 10002 -U 10003
>      start pasta
>    site_b:
>      pasta_options: -t 10012 -T 10013 -u 10012 -U 10013
>      start pasta
> 
> ?
> 
> >  That's possible with the approach I'm using here.
> 
> Sure, and it's less effort compared to what I'm describing, I know.

Right.  Basically I feel like something processing this would more or
less be translating a heirarchy of thingies in yaml to a heirarchy of
function calls to do the setup.  That seems like a fair bit of
boilerplate hassle to achieve a pretty small improvement in syntax
readability over just making those function calls directly.

> > > and from my understanding of what this is replacing:
> > > 
> > > setup_pasta() {
> > > 	context_setup_host unshare
> > > 
> > > 	context_run_bg unshare "unshare -rUnpf ${NSTOOL} hold ${STATESETUP}/ns.hold"
> > > 
> > > 	context_setup_nstool ns ${STATESETUP}/ns.hold
> > > 
> > > 	# Ports:
> > > 	#
> > > 	#                 ns        |         host
> > > 	#         ------------------|---------------------
> > > 	#  10002      as server     |    spliced to ns
> > > 	#  10003   spliced to init  |      as server
> > > 
> > > 	context_run_bg passt "./pasta -f -t 10002 -T 10003 -u 10002 -U 10003 -P ${STATESETUP}/passt.pid $(${NSTOOL} info -pw ${STATESETUP}/ns.hold)"
> > > }  
> > 
> > It's replacing not just that, but also the matching teardown function,
> > and ensures that the teardown runs if there's an exception in the test
> > itself (including running the teardowns for the outer contexts if the
> > setup of the inner contexts failed).
> 
> Still, I think the equivalent in POSIX shell (we kind of have it
> already) would be clearer. Obvious downside: a lot more to write, and
> it's bug-prone. Maybe we could solve this particular issue by a very
> verbosely documented example.
> 
> > > which I actually find less elegant but much clearer, especially before
> > > reading the whole series, I wonder if we could have either a
> > > configuration format, or directives... but, disappointingly, I couldn't
> > > decide on a more detailed proposal yet.
> > > 
> > > Still, I would aim at something resembling that
> > > pseudocode/configuration above. Of course, with no details it looks
> > > neat, so the comparison is unfair, but I'm under the impression that
> > > even after adding details (that's what I'm trying to figure
> > > out/articulate) it should still be much clearer.  
> > 
> > I'm just not convinced that it would be, without losing a great deal
> > of the flexibility in this system.  Or at least, not clearer than a
> > more polished version of this proposal.
> > 
> > > > +
> > > > +
> > > > +@test
> > > > +def test_ifname():  
> > > 
> > > I guess fine (even though it's missing a descriptive name _inline_).  
> > 
> > We could put a description in a docstring easily enough.
> 
> I guess needed.

I'm not sure what you mean by that.

> > > > +    with pasta_unconfigured() as (_, ns):  
> > > 
> > > Ouch... a bit hard to grasp for people not familiar with Python or
> > > Go... plus, conceptually, we _just_ want to say either:  
> > 
> > Yeah.  One of the bits I'm least happy with is what to do in cases
> > where the information the setup needs to pass to the tests proper is
> > quite complex: so here it has both the simulated network which is
> > hosting the pasta instance, and also the namespace within pasta.  This
> > specific test doesn't need the former, hence the _.  We could use a
> > suitably named dummy variable which might be a bit more readable,
> > though it would need a lint suppression (unused variable).
> > 
> > I've considered another approach, though I'm not sure it's an
> > improvement, which is to use more intermediate structs/classes.  So,
> > in that approach pasta_unconfigured() would give you a
> > 'PastaTestScenario' instance with fields for the guest ns, host ns and
> > whatever else seems useful.
> 
> I'm not entirely sure what that entails, but I guess it might be more
> readable overall (also because we'll probably need more fields...? How
> do you explicitly get a reference to a PID, for example?)
> 
> > > test: "pasta namespace has the interface name we configured"
> > >   enter namespace
> > >   list of interfaces includes $NAME
> > > 
> > > or:
> > > 
> > > test: "pasta namespace has a loopback interface"
> > >   list of interfaces in namespace includes $NAME  
> > 
> > It's more than that though: we're also requesting a very specific
> > (simulated) environment for the test, which the pseudocode above just
> > kind of assumes.  'pasta_unconfigured()' (name certainly negotiable)
> > is handling the setup and teardown of everything we need for a certain
> > class of pasta tests.  That has 3 namespaces, a veth, and a pasta
> > instance, all configured in a particular way.
> > 
> > > ...and yes, that requires that we start pasta first, but I wonder if it
> > > makes sense to mix the two notions (i.e. whether an object-oriented
> > > approach makes sense at all, actually).  
> > 
> > So the OO approach works ok for pretty simple setups - probably why it
> > became popular with jUnit etc: the class handles the setup, the
> > methods do the specific tests.  But it breaks down pretty quickly:
> >    - If you want to use multiple "setup" pieces at once you need to
> >      use multiple inheritence, which is a nightmare
> 
> Argh. Yes.
> 
> >    - If you want to parameterize parts of the setup, you need to do it
> >      at the whole class level which involves wierd non-local
> >      interactions
> >    - If you want to use multiple setup pieces which are actually the
> >      same piece repeated with different parameters, you're pretty much
> >      out of luck
> > 
> > I realize that context managers aren't the most obvious thing, but
> > they have the huge advantage that every test can directly and
> > reasonably tersely declare what setup they need to run:
> > 
> >     def test():
> > 	with setup_i_need():
> > 	    with other_setup_i_need():
> > 	       with setup_i_need(param='non default'):
> > 	           # actually do the test
> > 
> > This also handles the necessary teardown at each level, and it still
> > works if some of the inner setups need paramaters that are derived
> > fromt the outer setups.
> 
> Yes yes definitely.
> 
> > > > +        assert_eq_unordered(ns.ifs(), ('lo', IFNAME))  
> > > 
> > > Clear (probably clearer with a "list includes" operator).  
> > 
> > It's not the same as list includes (that's assert_in()).  This checks
> > that the lists (strictly, iterables) are identical ignoring order.  So
> > in this case ['lo', IFNAME] and [IFNAME, 'lo'] would be ok, but ['lo',
> > 'eth99', IFNAME] would not be.
> > 
> > > > +
> > > > +
> > > > +@test_ndp(IFNAME, HOST_NET6)
> > > > +@contextlib.contextmanager
> > > > +def pasta_ndp():
> > > > +    with pasta_unconfigured() as (simnet, guestns):
> > > > +        guestns.ifup(IFNAME)
> > > > +        yield guestns, simnet.gw_ip6_ll.ip  
> > > 
> > > This really hides what we are checking. Other than that (but it's a big
> > > issue in my opinion) I find it terse and elegant.  
> > 
> > Yeah, I'm not super happy with this one.  In particular I dislike the
> > inconsistency: in simple tests like the above we have, loosely the
> > setup, followed by the test.  In these composed examples we have the
> > tests then the setup.  So combining a few options I've thought of, I
> > could do something like this:
> > 
> > 	@contextlib.contextmanager
> > 	def pasta_ndp():
> > 	    with pasta_unconfigured() as (simnet, guestns):
> > 	        guestns.ifup(IFNAME)
> > 	        yield NdpTestScenario(ifname=IFNAME, net=HOST_NET6,
> > 		                      router=simnet.gw_ip6_ll.ip,
> > 				      ndpclient=guestns)
> > 
> > 	compose_matrix([pasta_ndp], NDP_TESTS)
> > 
> > Here NDP_TESTS would come from the ndp module and have the list of
> > individual ndp test cases, each of which takes an NdpTestScenario as
> > parameter.
> 
> Probably easier to understand, but it doesn't solve the problem that
> "yield" doesn't mean "we're checking this". Maybe it could be worked
> around with naming though, I haven't tried.

Hm, can't really do that - "yield" is a keyword, not a function or
identifier.

> > > > +
> > > > +@test_dhcp(IFNAME, IP4.ip, GW_IP4.ip, 65520)  
> > > 
> > > The test_ndp decorator is probably usable, this one not so much. As we
> > > have more parameters, it would probably be better to have something
> > > more descriptive (even if necessarily more verbose).  
> > 
> > Right.  So would the treatment above help?  Enough?  It would look
> > something like:
> > 
> > 	@contextlib.contextmanager
> > 	def pasta_dhcp():
> > 	    ....
> > 	    yield DhcpTestScenario(ifname=IFNAME, client_ip4=IP4.ip,
> > 	                           server_ip4=GW_IP4.ip, mtu=65520)
> > 
> > 	compose_matrix([pasta_dhcp], DHCP_TESTS)
> 
> Much better, I think.

Ok, I'll move towards that throughout.

> > > > +@test_dhcpv6(IFNAME, IP6.ip)
> > > > +@contextlib.contextmanager
> > > > +def pasta_dhcp():
> > > > +    with pasta_unconfigured() as (_, guestns):
> > > > +        yield guestns
> > > > +
> > > > +
> > > > +@contextlib.contextmanager
> > > > +def pasta_configured():
> > > > +    with pasta_unconfigured(FWD_OPTS + ' --config-net') as (simnet, ns):
> > > > +        # Wait for DAD to complete on the --config-net address  
> > > 
> > > Side note: this shouldn't be needed -- if it is, we should probably fix
> > > something in pasta.  
> > 
> > It is needed, and yes we probably should, I haven't had a chance to
> > look into what, exactly.  We may hit that kernel bug I noticed where
> > permissions to the disable_dad sysctl seem to behave weirdly.
> 
> Ah, right, that rings a bell.
> 
> > > > +        ns.addr_wait(IFNAME, family='inet6', scope='global')
> > > > +        yield simnet, ns  
> > > 
> > > Except for the common points with my observation above, this looks
> > > usable, and:
> > >   
> > > > +
> > > > +
> > > > +@test
> > > > +def test_config_net_addr():
> > > > +    with pasta_configured() as (_, ns):
> > > > +        addrs = ns.addrs(IFNAME, scope='global')
> > > > +        assert_eq_unordered(addrs, [IP4, IP6])
> > > > +
> > > > +
> > > > +@test
> > > > +def test_config_net_route4():
> > > > +    with pasta_configured() as (_, ns):
> > > > +        (defroute,) = ns.routes4(dst='default')
> > > > +        gateway = ipaddress.ip_address(defroute['gateway'])
> > > > +        assert_eq(gateway, GW_IP4.ip)
> > > > +
> > > > +
> > > > +@test
> > > > +def test_config_net_route6():
> > > > +    with pasta_configured() as (simnet, ns):
> > > > +        (defroute,) = ns.routes6(dst='default')
> > > > +        gateway = ipaddress.ip_address(defroute['gateway'])
> > > > +        assert_eq(gateway, simnet.gw_ip6_ll.ip)
> > > > +
> > > > +
> > > > +@test
> > > > +def test_config_net_mtu():
> > > > +    with pasta_configured() as (_, ns):
> > > > +        mtu = ns.mtu(IFNAME)
> > > > +        assert_eq(mtu, 65520)  
> > > 
> > > these all make intuitive sense to me.
> > >   
> > > > +
> > > > +
> > > > +@test_transfers(ip4=REMOTE_IP4.ip, ip6=REMOTE_IP6.ip, port=10000)
> > > > +@contextlib.contextmanager
> > > > +def outward_transfer():
> > > > +    with pasta_configured() as (simnet, ns):
> > > > +        yield ns, simnet.gw  
> > > 
> > > This, however, not. I would naturally expect the function implementing
> > > a template of data transfer test to do something with data.  
> > 
> > I'm not entirely sure what you mean here.  Would the same treatment as
> > for the ndp and dhcp cases above help?
> > 
> > 	@contextlib.contextmanager
> > 	def outward_transfer():
> > 	    with pasta_configured() as (simnet, ns):
> > 	        yield TransferTestScenario(client=ns,
> > 	                                   server=simnet.gw,
> > 	                                   connect_ip4=REMOTE_IP4.ip,
> > 	                                   connect_ip6=REMOTE_IP6.ip,
> > 					   connect_port=10000)
> > 
> > 	# variants for inward transfer, and spliced_transfer
> > 	compose_matrix([outward_transfer, inward_transfer, spliced_transfer],
> > 	               TRANSFER_TESTS)
> 
> Yes, now I actually understand what outward_transfer() does: it calls
> TransferTestScenario(). I really had no idea otherwise.

Well.. sort of.  The idea here is that 'TransferTestScenario' doesn't
itself *do* anything - it's just a container for all the information
relevant to running transfer tests in a specific setup.  All the
individual transfer test functions then use the information in here to
do their thing.

> > > > +
> > > > +
> > > > +@test_transfers(ip4=IP4.ip, ip6=IP6.ip, port=IN_FWD_PORT,
> > > > +                fromip4=REMOTE_IP4.ip, fromip6=REMOTE_IP6.ip)
> > > > +@contextlib.contextmanager
> > > > +def inward_transfer():
> > > > +    with pasta_configured() as (simnet, ns):
> > > > +        yield simnet.gw, ns
> > > > +
> > > > +
> > > > +@test_transfers(ip4=LOOPBACK4, ip6=LOOPBACK6, port=SPLICE_FWD_PORT,
> > > > +                listenip4=LOOPBACK4, listenip6=LOOPBACK6)
> > > > +@contextlib.contextmanager
> > > > +def spliced_transfer():
> > > > +    with pasta_configured() as (simnet, ns):
> > > > +        yield ns, simnet.simhost  
> > > 
> > > I still have to decode these before I can reasonably comment on them.
> > > 
> > > I'll follow up with a more detailed proposal of a possible
> > > configuration format or perhaps something between a domain-specific
> > > language and an annotated Python script (à la bats), but I'm trying to
> > > consider a few more tests on top of these.
> > > 
> > > I started looking at "options" tests next, and realised my proposal was
> > > a bit too simplistic. But also that the current version of those tests,
> > > while somewhat verbose and surely clunky, shows in a very clear way
> > > what we're checking and what we expect... and I'm not sure that the
> > > outcome from extending pasta_configured() would be very usable.  
> > 
> > So, I was only thinking of pasta_configured() and pasta_unconfigured()
> > as useful for the pretty specific pattern of pasta invocations in this
> > batch of tests (which is why they're local to that file).  I've also
> > had at pasta_options (which I'm thinking of as the log-to-file tests,
> > since that's all they test so far).  I was planning a new helper to
> > set up a suitable scenario for that, which would be based on
> > simple_net() and Pasta() with some kind of parameterization for where
> > to place the logfile.
> 
> Hmm, yes, I guess that would be better than the alternative I was afraid
> of (extending pasta_configured()).
> 
> > > All we need for that is:
> > > 
> > > - define options and network model/addressing  
> > 
> > I think that short statement hides a lot of complexity.
> 
> Eh, yes, but if you take this part by itself, you wouldn't naturally
> jump to Python decorators. :)

Indeed, and that's not where the decorators are arising from.  With
the revisions I'm already planning on there will only be two
decorators:

    @test(), which is used to declare a particular function as a
    standalone test (possibly with options like timeout).  This could
    be done instead with explicit "register_test('name', testfn)"
    calls, but the decorator form seems more natural to me.

    @contextlib.contextmanager, which is a way of much more tersely
    writing "context managers", which are essentially a matched pair
    of setup/teardown functions.  I realize that they're kind of
    obscure, but it's a huge reduction in the amount of boilerplate:

Suppose we have an existing setup foo(), and want to extend it with
one extra step.  Using the decorator it's:

@contextlib.contextmanager
def bar(param):
    with foo(param) as f:
       additional_setup(f)
       yield f

To open code it would be roughly:

class Bar(contextlib.AbstractContextManager):
    def __init__(self, param):
       self.foo = foo(param)

    def __enter__(self):
        f = self.foo.__enter__()
	try:
	    additional_setup(f)
	except Exception as e:
	    self.foo.__exit__(... not sure what goes here ...)
	return f

    def __exit__(self, *exc_info):
        self.foo.__exit__(*exc_info)

.. and it just gets worse with more complex examples.

> > > - have a clear syntax of identifying where pasta is starting
> > > 
> > > - a list of commands (which, themselves, are absolutely obvious and
> > >   natural in a shell script, but I see the value of using Python
> > >   features to check assertions, at least).  
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]