[ovs-dev,repost,1/3] tests: Run OVS_WAIT_WHILE, OVS_WAIT_UNTIL in main shell environment.
diff mbox

Message ID 1443550635-29312-1-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Sept. 29, 2015, 6:17 p.m. UTC
AT_CHECK runs its commands in a subshell.  That means that (among other
effects), any variable assignments within its commands will disappear after
the commands' completion.  That doesn't matter for any of the existing
users, which don't do the sorts of things that affect an outer shell
environment anyhow, but an upcoming user wants to make a shell assignment
that persists.  This commit makes that possible, by using AT_CHECK
(actually AT_FAIL_IF but it's moot) only upon failure instead of bracketing
the entire test.

Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 tests/ovs-macros.at | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Simon Horman Nov. 25, 2015, 7:21 a.m. UTC | #1
Hi Ben,

On Tue, Sep 29, 2015 at 11:17:13AM -0700, Ben Pfaff wrote:
> AT_CHECK runs its commands in a subshell.  That means that (among other
> effects), any variable assignments within its commands will disappear after
> the commands' completion.  That doesn't matter for any of the existing
> users, which don't do the sorts of things that affect an outer shell
> environment anyhow, but an upcoming user wants to make a shell assignment
> that persists.  This commit makes that possible, by using AT_CHECK
> (actually AT_FAIL_IF but it's moot) only upon failure instead of bracketing
> the entire test.
> 
> Signed-off-by: Ben Pfaff <blp@nicira.com>

From my point of view adding support for side effects isn't particularly
desirable, however, I'm not sure that I know a way to cleanly handle the
use-case in the third patch of the series. So I guess its best to give a
little to get a lot.

With the above in mind, the entire series:

Reviewed-by: Simon Horman <simon.horman@netronome.com>
Flavio Leitner Nov. 26, 2015, 5:49 p.m. UTC | #2
On Tue, Sep 29, 2015 at 11:17:13AM -0700, Ben Pfaff wrote:
> AT_CHECK runs its commands in a subshell.  That means that (among other
> effects), any variable assignments within its commands will disappear after
> the commands' completion.  That doesn't matter for any of the existing
> users, which don't do the sorts of things that affect an outer shell
> environment anyhow, but an upcoming user wants to make a shell assignment
> that persists.  This commit makes that possible, by using AT_CHECK
> (actually AT_FAIL_IF but it's moot) only upon failure instead of bracketing
> the entire test.
> 
> Signed-off-by: Ben Pfaff <blp@nicira.com>
> ---

I like the idea of passing variables.
Patch looks good, nothing breaks here though I had to
manually update the couple patches here.

Acked-by: Flavio Leitner <fbl@sysclose.org>
Ben Pfaff Nov. 26, 2015, 9:28 p.m. UTC | #3
On Wed, Nov 25, 2015 at 04:21:22PM +0900, Simon Horman wrote:
> Hi Ben,
> 
> On Tue, Sep 29, 2015 at 11:17:13AM -0700, Ben Pfaff wrote:
> > AT_CHECK runs its commands in a subshell.  That means that (among other
> > effects), any variable assignments within its commands will disappear after
> > the commands' completion.  That doesn't matter for any of the existing
> > users, which don't do the sorts of things that affect an outer shell
> > environment anyhow, but an upcoming user wants to make a shell assignment
> > that persists.  This commit makes that possible, by using AT_CHECK
> > (actually AT_FAIL_IF but it's moot) only upon failure instead of bracketing
> > the entire test.
> > 
> > Signed-off-by: Ben Pfaff <blp@nicira.com>
> 
> From my point of view adding support for side effects isn't particularly
> desirable, however, I'm not sure that I know a way to cleanly handle the
> use-case in the third patch of the series. So I guess its best to give a
> little to get a lot.
> 
> With the above in mind, the entire series:
> 
> Reviewed-by: Simon Horman <simon.horman@netronome.com>

I guess I feel like scoping in a shell environment is more of a surprise
than a feature.

Thanks Simon and Flavio!  I applied these three patches to master.

Patch
diff mbox

diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
index 058830b..535d923 100644
--- a/tests/ovs-macros.at
+++ b/tests/ovs-macros.at
@@ -31,13 +31,14 @@  ovs_wait () {
     # in the normal case.  POSIX doesn't require fractional times to
     # work, so this might not work.
     sleep 0.1
-    ovs_wait_cond && exit 0
+    ovs_wait_cond && return 0
+
     # Then wait up to 10 seconds.
     for d in 0 1 2 3 4 5 6 7 8 9; do
         sleep 1
-        ovs_wait_cond && exit 0
+        ovs_wait_cond && return 0
     done
-    exit 1
+    return 1
 }
 
 # Prints the integers from $1 to $2, increasing by $3 (default 1) on stdout.
@@ -87,11 +88,16 @@  fi
 ]
 m4_divert_pop([PREPARE_TESTS])
 
-m4_define([OVS_WAIT],
-  [AT_CHECK(
-     [ovs_wait_cond () { $1
+m4_define([OVS_WAIT], [dnl
+ovs_wait_cond () {
+    $1
 }
-ovs_wait], [0], [ignore], [ignore], [$2])])
+if ovs_wait; then :
+else
+    $2
+    AT_FAIL_IF([:])
+fi
+])
 m4_define([OVS_WAIT_UNTIL], [OVS_WAIT([$1], [$2])])
 m4_define([OVS_WAIT_WHILE],
   [OVS_WAIT([if $1; then return 1; else return 0; fi], [$2])])