From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 886145A026D for ; Fri, 7 Jul 2023 19:42:52 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688751771; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3vAxcJbuBSDwVPBXHAuCTBUNK6lWS29qAyd49R0RxPc=; b=P69trbvWXlOqwGXi9xwq4/NmVGP/cGnONC+sLmHTECewMJxxyBGCSGrhT0jKTeeaG6Swsx MFW77jbNNl4dzDPalwXwod2Iyu3GDk3AivDZ6Ga+C1qIemUmwG+VUp7k4NzUuvM6TVA7MB wc6rQdChw2kNT09axe0fZ7HKoP6PDsw= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-464-MGz7bl-_NvCNN5Vpqf1iUQ-1; Fri, 07 Jul 2023 13:42:46 -0400 X-MC-Unique: MGz7bl-_NvCNN5Vpqf1iUQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id A001F80269A; Fri, 7 Jul 2023 17:42:46 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.30]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5C1CCF6431; Fri, 7 Jul 2023 17:42:44 +0000 (UTC) Date: Fri, 7 Jul 2023 19:42:39 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 27/27] avocado: Convert basic pasta tests Message-ID: <20230707194239.2ae67345@elisabeth> In-Reply-To: References: <20230627025429.2209702-1-david@gibson.dropbear.id.au> <20230627025429.2209702-28-david@gibson.dropbear.id.au> <20230705023048.108f3c46@elisabeth> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 37B56UNKWD6AQ7KEHNIUCKJY7XPNN6FU X-Message-ID-Hash: 37B56UNKWD6AQ7KEHNIUCKJY7XPNN6FU X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, crosa@redhat.com, jarichte@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, 5 Jul 2023 13:27:17 +1000 David Gibson 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: > >=20 > > On Tue, 27 Jun 2023 12:54:28 +1000 > > David Gibson wrote: > > =20 > > > Convert the old-style tests for pasta (DHCP, NDP, TCP and UDP transfe= rs) > > > to using avocado. There are a few differences in what we test, but t= his > > > should generally improve coverage: > > >=20 > > > * We run in a constructed network environment, so we no longer depen= d 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 sim= ulated > > > pasta host. This better matches the typical passt/pasta usecase = =20 > >=20 > > ...this is not necessarily true -- it really depends, but sure, it's > > important to have this too. =20 >=20 > 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. > > > Signed-off-by: David Gibson > > > --- > > > 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 > > >=20 > > > diff --git a/oldtest/run b/oldtest/run > > > index a16bc49b..f1157f90 100755 > > > --- a/oldtest/run > > > +++ b/oldtest/run > > > @@ -70,13 +70,13 @@ run() { > > > =09test build/clang_tidy > > > =09teardown build > > > =20 > > > -#=09setup pasta > > > -#=09test pasta/ndp > > > -#=09test pasta/dhcp > > > -#=09test pasta/tcp > > > -#=09test pasta/udp > > > -#=09test passt/shutdown > > > -#=09teardown pasta > > > +=09setup pasta > > > +=09test pasta/ndp > > > +=09test pasta/dhcp > > > +=09test pasta/tcp > > > +=09test pasta/udp > > > +=09test passt/shutdown > > > +=09teardown pasta > > > =20 > > > #=09setup pasta_options > > > #=09test 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): > > > =20 > > > .PHONY: avocado-assets > > > avocado-assets: nstool small.bin medium.bin big.bin > > > +=09$(MAKE) -C .. pasta > > > =20 > > > .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 =20 > >=20 > > I'm just focusing on this for the moment, as this is the part I'm mostl= y > > 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. > >=20 > > That is, I'm mostly isolating negative aspects here. > >=20 > > I'm stressing "code" because I also would have the natural expectation > > that it should/must be simpler to _write_ (not necessarily design) test= s > > 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). =20 >=20 > 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. > That's not to say we can't make it better than it is in this draft. >=20 > > > @@ -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 > > > + > > > +""" > > > +avocado/pasta.py - Basic tests for pasta mode > > > +""" > > > + > > > +import contextlib > > > +import ipaddress > > > +import os > > > +import tempfile =20 > >=20 > > So far so good, I probably don't need to read up about every single > > import. > > =20 > > > +from avocado_classless.test import assert_eq, assert_eq_unordered, t= est =20 > >=20 > > Probably a future source of (needless) copy-and-paste. =20 >=20 > Fair point. I was considering making the idiom here: > =09from 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. > > Of course, > > assert_eq_unordered is not available in the current tests, and we need > > it, plus a syntax to represent that. =20 >=20 > 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. > > =20 > > > +from tasst.address import LOOPBACK4, LOOPBACK6 =20 > >=20 > > 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. =20 >=20 > 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? > > > +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, REM= OTE_IP6 > > > +) > > > +from tasst.transfer import test_transfers =20 > >=20 > > 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. =20 >=20 > Right, I could introduce "from foo import *" conventions for some of > these too. >=20 > > > + > > > + > > > +IN_FWD_PORT =3D 10002 > > > +SPLICE_FWD_PORT =3D 10003 > > > +FWD_OPTS =3D f'-t {IN_FWD_PORT} -u {IN_FWD_PORT} \ > > > + -T {SPLICE_FWD_PORT} -U {SPLICE_FWD_PORT}' =20 > >=20 > > I think clear enough. > > =20 > > > + > > > +@contextlib.contextmanager =20 > >=20 > > 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. =20 >=20 > 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: >=20 > 1) You can do: >=20 > =09@contextlib.contextmanager > =09def foo(...): > =09 setup() >=20 > =09 yield whatever # <=3D=3D actual test runs here >=20 > =09 cleanup() >=20 > this will do the cleanup() even if the test blows up. >=20 > 2) You can do >=20 > =09@contextlib.contextmanager > =09def bar(...): > =09 with other_context_1(): > =09 with other_context_2(): > =09=09 yield whatever # <=3D=3D=3D actual test runs here >=20 > This effectively makes this a combination of the two existing > contexts, again ensuring that both their cleanup happens if the test > blows up. >=20 > 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. > > > +def pasta_unconfigured(opts=3DFWD_OPTS): =20 > >=20 > > Clear. > > =20 > > > + with simple_net() as simnet: =20 > >=20 > > Also clear. > > =20 > > > + with unshare_site('pastans', '-Ucnpf --mount-proc', > > > + parent=3Dsimnet.simhost, sudo=3DTrue) as g= uestns: =20 > >=20 > > If I ignore for a moment the implementation detail, it's not very > > intuitive why this is nested in the "with" statement before. =20 >=20 > 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. 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. 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(), = =20 >=20 > Hm, ok. I could require that this takes keyword arguments, so it > would be something like: > unshare_site(name=3D'pastans', unshare_opts=3D'-Ucnpf --mount-proc', > ..) >=20 > Would that help?=09 Yes, definitely, a bit more to type, but absolutely understandable. > > and... I now know what "sudo=3DTrue" 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). =20 >=20 > 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. > > So far, what this says is: > >=20 > > - use addressing from simple_net() =20 >=20 > 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 =20 >=20 > 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? > > > + with tempfile.TemporaryDirectory() as tmpdir: =20 > >=20 > > ...even less intuitive why this statement (tmpdir=3D"$(mktemp -d)") is > > nested. =20 >=20 > So it's not quite equivalent to tmpdir=3D$(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). > 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. >=20 > > =20 > > > + pidfile =3D os.path.join(tmpdir, 'pasta.pid') > > > + > > > + with Pasta(simnet.simhost, pidfile, opts, ns=3Dguest= ns) as pasta: > > > + yield simnet, pasta.ns =20 > >=20 > > ...and this finally says: start pasta in that (user? network?) > > namespace. =20 >=20 > 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. >=20 > [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] >=20 > 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: > >=20 > > setup: > > net: simple_net > > pasta_options: -t 10002 -T 10003 -u 10002 -U 10003 > > start pasta =20 >=20 > 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. > > and from my understanding of what this is replacing: > >=20 > > setup_pasta() { > > =09context_setup_host unshare > >=20 > > =09context_run_bg unshare "unshare -rUnpf ${NSTOOL} hold ${STATESETUP}/= ns.hold" > >=20 > > =09context_setup_nstool ns ${STATESETUP}/ns.hold > >=20 > > =09# Ports: > > =09# > > =09# ns | host > > =09# ------------------|--------------------- > > =09# 10002 as server | spliced to ns > > =09# 10003 spliced to init | as server > >=20 > > =09context_run_bg passt "./pasta -f -t 10002 -T 10003 -u 10002 -U 10003= -P ${STATESETUP}/passt.pid $(${NSTOOL} info -pw ${STATESETUP}/ns.hold)" > > } =20 >=20 > 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. > >=20 > > 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. =20 >=20 > 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. >=20 > > > + > > > + > > > +@test > > > +def test_ifname(): =20 > >=20 > > I guess fine (even though it's missing a descriptive name _inline_). = =20 >=20 > We could put a description in a docstring easily enough. I guess needed. > > > + with pasta_unconfigured() as (_, ns): =20 > >=20 > > Ouch... a bit hard to grasp for people not familiar with Python or > > Go... plus, conceptually, we _just_ want to say either: =20 >=20 > 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). >=20 > 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 > >=20 > > or: > >=20 > > test: "pasta namespace has a loopback interface" > > list of interfaces in namespace includes $NAME =20 >=20 > 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. >=20 > > ...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). =20 >=20 > 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 >=20 > 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: >=20 > def test(): > =09with setup_i_need(): > =09 with other_setup_i_need(): > =09 with setup_i_need(param=3D'non default'): > =09 # actually do the test >=20 > 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)) =20 > >=20 > > Clear (probably clearer with a "list includes" operator). =20 >=20 > 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. >=20 > > > + > > > + > > > +@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 =20 > >=20 > > 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. =20 >=20 > 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: >=20 > =09@contextlib.contextmanager > =09def pasta_ndp(): > =09 with pasta_unconfigured() as (simnet, guestns): > =09 guestns.ifup(IFNAME) > =09 yield NdpTestScenario(ifname=3DIFNAME, net=3DHOST_NET6, > =09=09 router=3Dsimnet.gw_ip6_ll.ip, > =09=09=09=09 ndpclient=3Dguestns) >=20 > =09compose_matrix([pasta_ndp], NDP_TESTS) >=20 > 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. > > > + > > > +@test_dhcp(IFNAME, IP4.ip, GW_IP4.ip, 65520) =20 > >=20 > > 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). =20 >=20 > Right. So would the treatment above help? Enough? It would look > something like: >=20 > =09@contextlib.contextmanager > =09def pasta_dhcp(): > =09 .... > =09 yield DhcpTestScenario(ifname=3DIFNAME, client_ip4=3DIP4.ip, > =09 server_ip4=3DGW_IP4.ip, mtu=3D65520) >=20 > =09compose_matrix([pasta_dhcp], DHCP_TESTS) Much better, I think. > > > +@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 =20 > >=20 > > Side note: this shouldn't be needed -- if it is, we should probably fix > > something in pasta. =20 >=20 > 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=3D'inet6', scope=3D'global') > > > + yield simnet, ns =20 > >=20 > > Except for the common points with my observation above, this looks > > usable, and: > > =20 > > > + > > > + > > > +@test > > > +def test_config_net_addr(): > > > + with pasta_configured() as (_, ns): > > > + addrs =3D ns.addrs(IFNAME, scope=3D'global') > > > + assert_eq_unordered(addrs, [IP4, IP6]) > > > + > > > + > > > +@test > > > +def test_config_net_route4(): > > > + with pasta_configured() as (_, ns): > > > + (defroute,) =3D ns.routes4(dst=3D'default') > > > + gateway =3D ipaddress.ip_address(defroute['gateway']) > > > + assert_eq(gateway, GW_IP4.ip) > > > + > > > + > > > +@test > > > +def test_config_net_route6(): > > > + with pasta_configured() as (simnet, ns): > > > + (defroute,) =3D ns.routes6(dst=3D'default') > > > + gateway =3D 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 =3D ns.mtu(IFNAME) > > > + assert_eq(mtu, 65520) =20 > >=20 > > these all make intuitive sense to me. > > =20 > > > + > > > + > > > +@test_transfers(ip4=3DREMOTE_IP4.ip, ip6=3DREMOTE_IP6.ip, port=3D100= 00) > > > +@contextlib.contextmanager > > > +def outward_transfer(): > > > + with pasta_configured() as (simnet, ns): > > > + yield ns, simnet.gw =20 > >=20 > > This, however, not. I would naturally expect the function implementing > > a template of data transfer test to do something with data. =20 >=20 > I'm not entirely sure what you mean here. Would the same treatment as > for the ndp and dhcp cases above help? >=20 > =09@contextlib.contextmanager > =09def outward_transfer(): > =09 with pasta_configured() as (simnet, ns): > =09 yield TransferTestScenario(client=3Dns, > =09 server=3Dsimnet.gw, > =09 connect_ip4=3DREMOTE_IP4.ip, > =09 connect_ip6=3DREMOTE_IP6.ip, > =09=09=09=09=09 connect_port=3D10000) >=20 > =09# variants for inward transfer, and spliced_transfer > =09compose_matrix([outward_transfer, inward_transfer, spliced_transfer], > =09 TRANSFER_TESTS) Yes, now I actually understand what outward_transfer() does: it calls TransferTestScenario(). I really had no idea otherwise. > > > + > > > + > > > +@test_transfers(ip4=3DIP4.ip, ip6=3DIP6.ip, port=3DIN_FWD_PORT, > > > + fromip4=3DREMOTE_IP4.ip, fromip6=3DREMOTE_IP6.ip) > > > +@contextlib.contextmanager > > > +def inward_transfer(): > > > + with pasta_configured() as (simnet, ns): > > > + yield simnet.gw, ns > > > + > > > + > > > +@test_transfers(ip4=3DLOOPBACK4, ip6=3DLOOPBACK6, port=3DSPLICE_FWD_= PORT, > > > + listenip4=3DLOOPBACK4, listenip6=3DLOOPBACK6) > > > +@contextlib.contextmanager > > > +def spliced_transfer(): > > > + with pasta_configured() as (simnet, ns): > > > + yield ns, simnet.simhost =20 > >=20 > > I still have to decode these before I can reasonably comment on them. > >=20 > > 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 (=C3=A0 la bats), but I'm tryin= g to > > consider a few more tests on top of these. > >=20 > > 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. =20 >=20 > 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: > >=20 > > - define options and network model/addressing =20 >=20 > 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. :) > > - have a clear syntax of identifying where pasta is starting > >=20 > > - 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). =20 --=20 Stefano