Bug 1257062 - New 'conditional' skeleton for beaker-wizard
New 'conditional' skeleton for beaker-wizard
Status: CLOSED CURRENTRELEASE
Product: Beaker
Classification: Community
Component: command line (Show other bugs)
21
Unspecified Unspecified
medium Severity medium (vote)
: 23.0
: ---
Assigned To: Dan Callaghan
tools-bugs
: FutureFeature, Patch
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-08-26 04:11 EDT by Iveta Wiedermann
Modified: 2016-07-07 19:09 EDT (History)
10 users (show)

See Also:
Fixed In Version:
Doc Type: Enhancement
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-07-07 19:09:46 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Iveta Wiedermann 2015-08-26 04:11:11 EDT
Description of problem:
After some discussions we (BaseOS QE) agreed that setup phase should be as reliable as possible and therefor if it fails a test phase doesn't have prepared environment correctly and should be skipped. For this purpose rlGetTestState function can be used and the default skeleton might look like:

rlPhaseStartSetup
    rlAssertRpm $PACKAGE || rlDie "Sorry, nothing to be tested"
    ...
rlPhaseEnd

rlGetTestState && {
    rlPhaseStartTest
       ...
    rlPhaseEnd
}

rlPhaseStartCleanup
   ...
rlPhaseEnd
Comment 1 Petr Šplíchal 2015-08-26 04:17:35 EDT
Thanks for the bug, Iveta. I'll prepare the updated skeleton.
Comment 2 Petr Šplíchal 2015-08-31 03:52:46 EDT
Updated skeleton submitted for review:
http://gerrit.beaker-project.org/4373

> wizard: Update the default skeleton [BZ#1257062]
> 
> Use rlDie to show a clear example how to bail out from the test if
> something goes terribly wrong. Skip the main test phase if the setup
> phase fails (the environment is not correctly set up for testing).
> 
> Change-Id: I7e677bba263038332b73c200e771f1f021d825ec
Comment 3 David Kutálek 2015-10-01 05:23:37 EDT
It seems to me this brings even more boiler plate code.
Harder to read, creating variants through the test suites...

Why not make rlPhaseStartTest itself do a check?
Comment 4 Karel Srot 2015-10-01 05:40:40 EDT
(In reply to David Kutálek from comment #3)
> Why not make rlPhaseStartTest itself do a check?

Having tests with multiple test phases is quite common scenario and I do not want to abort a test phase just because a previous one failed. Moreover, I personally prefer to avoid using rlGetTestState entirely.
Comment 5 Ales Zelinka 2015-10-01 05:54:30 EDT
(In reply to David Kutálek from comment #3)

> Why not make rlPhaseStartTest itself do a check?

A phase isn't a block of code, rlPhaseStartTest has no way of telling bash to skip all subsequent commands up to rlPhaseEnd. (If possible it would have to be configurable via a parameter, otherwise it would introduce a major change in default behaviour = big no-no).
Comment 6 David Kutálek 2015-10-01 07:08:43 EDT
(In reply to Karel Srot from comment #4)
> (In reply to David Kutálek from comment #3)
> > Why not make rlPhaseStartTest itself do a check?
> 
> Having tests with multiple test phases is quite common scenario and I do not
> want to abort a test phase just because a previous one failed. 

Initial proposal does not mention how to deal more phases,
so I expected just including all of them into the conditional block,
making them all depend only on Setup phase.

In this way I expected that all Test phases would be skipped
only in case Setup phase failed.

> Moreover, I personally prefer to avoid using rlGetTestState entirely.

Understand, the behaviour would have to be somehow optional.

(In reply to Ales Zelinka from comment #5)
> (In reply to David Kutálek from comment #3)
> 
> > Why not make rlPhaseStartTest itself do a check?
> 
> A phase isn't a block of code, rlPhaseStartTest has no way of telling bash
> to skip all subsequent commands up to rlPhaseEnd. 

Ah, correct. It would have to be probably a little bit hackish
(at least script can read its own code, so could eg. eval Cleanup phase...).

> (If possible it would have
> to be configurable via a parameter, otherwise it would introduce a major
> change in default behaviour = big no-no).

Yes.
Comment 7 Robin Hack 2015-10-02 01:44:04 EDT
Because we moved discussion here.

Context from previous discussion on mailing list:

amohdal:
    .. some random words ...

    if rlGetTestState;
    then

        rlPhaseStartTest foo
            ...
        rlPhaseEnd

    else

        rlLogError "setup failed; skipping tests"

    fi

azelinka:
I do like (and use myself) the else-branch that leaves a log of what happened. Encountering a log with a phase missing, without any hint on what caused it is quite confusing.

rhack:
I personally dislike if-else in case of long test phase... I really don't want to search for ending fi somewhere...

Is here wayt how to do something like:

rlPhaseSetup
 rlFail
rlPhaseEnd

# When setup fase fails, test phase will be skipped
if !rlGetTestState:
  SkipPhase or go to CleanupPhase
fi

rlPhaseTest
 rlPass
rlPhaseEnd

What do you think? Maybe it's worst of them all :).
Maybe I'm too deformed from C ;).

It's just my few dollars...
Comment 8 Petr Šplíchal 2015-10-15 06:00:34 EDT
The main motivation for the skeleton update was to give a hint
that skipping test phase after unsuccessful setup is possible and
is recommended as it saves machine time (no point of running the
test phase if the environment is not ready for testing).

I believe the proposed patch is not very invasive. I've attempted
to minimize changes necessary to the minimal skeleton, at the same
time allowing to easily remove the stuff if, for particular use
case, skipping the test phase is not desired.

I think it's better to do the conditional explicitly: This helps
to clearly see what's happening. Although I have to agree that the
code is not that beautiful as it could be. Another option could be
to revisit the idea of conditional phases (bug 508650):

    rlPhaseStartTest && {
        rlRun "touch foo" 0 "Creating the foo test file"
        rlAssertExists "foo"
        rlRun "ls -l foo" 0 "Listing the foo test file"
    rlPhaseEnd; }

This would look a bit more concise/aligned. But we would probably
have to rethink and rediscuss the original idea as there were some
implementation problems if I remember correctly. Inviting Dalibor
to the discussion.
Comment 9 Petr Šplíchal 2015-10-26 09:00:28 EDT
Here's another option how the updated skeleton could look like
(result of today's brainstorm of several BaseOS QE folks):

    rlGetTestState && { # Skip test phases if setup failed

    rlPhaseStartTest
        rlRun "touch foo" 0 "Creating the foo test file"
        rlAssertExists "foo"
        rlRun "ls -l foo" 0 "Listing the foo test file"
    rlPhaseEnd

    }

However, after rather long discussion we were not able to agree on
which of the options presented so far is the best. Each one has
some substantial disadvantages and none is clean/clear enough.
Comment 10 Petr Šplíchal 2015-12-09 05:26:49 EST
Summary from today's discussion: As there is no clear consensus
about the update of the default skeleton let's offer the variant
as another optional skeleton called 'conditional'. Objections?
Comment 11 Petr Šplíchal 2016-03-02 04:11:05 EST
Updated the patch as agreed and pushed to gerrit:
http://gerrit.beaker-project.org/#/c/4373/
Comment 12 Mike McCune 2016-03-28 19:23:30 EDT
This bug was accidentally moved from POST to MODIFIED via an error in automation, please see mmccune@redhat.com with any questions
Comment 13 Dan Callaghan 2016-04-08 02:45:02 EDT
This bug fix is included in beaker-client-22.4-0.git.6.5613dcf which is currently available for download here:

https://beaker-project.org/nightlies/release-22/
Comment 16 Dan Callaghan 2016-06-09 21:40:22 EDT
This patch was merged to the release-22 branch but the next release will be 23.0.
Comment 17 Dan Callaghan 2016-07-07 19:09:46 EDT
Beaker 23.0 has been released.

Note You need to log in before you can comment on or make changes to this bug.