Bug 1257062

Thanks for the bug, Iveta. I'll prepare the updated skeleton.

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

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?

(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.

(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).

(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.

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...

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.

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.

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?

Updated the patch as agreed and pushed to gerrit:
http://gerrit.beaker-project.org/#/c/4373/

This bug was accidentally moved from POST to MODIFIED via an error in automation, please see mmccune with any questions

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/

This patch was merged to the release-22 branch but the next release will be 23.0.

Beaker 23.0 has been released.