Michael Goldish wrote:
I think it's very rare to want to let the test continue even if some
command fails.
Can you give examples?
Some commands are not critical, like one that converts the screendumps
from PPM to PNG format. We don't want to fail the test if you don't have
ImageMagick installed.
I think you don't want to run the test at all in that case. But if you
do, you shouldn't invoke ImageMagick if you don't have it installed.
Otherwise you can't distinguish between failure due to a corrupted
source image, out of disk space, or ImageMagick not installed.
Ignoring errors is usually a bug.
A similar function used for remote commands is sometimes used for checking
if a file exists on the guest and getting its size at the same time,
using ls. If the file doesn't exist we should SCP it into the guest, not
fail the test.
Again it doesn't distinguish between real errors and 'file does not
exist'. Use something like 'if test -f file; then ls -l file; else echo
missing; fi'.
You can use rsync to implement 'copy unless exists'.
try:
code
code
code
except e:
raise ChainedException('exception while running migration test',
e)
Instead of checking each code line, you provide a wrapper for the
entire test.
It's not informative to add it for the entire test, because we already know
what test failed -- we don't need the exception string for that.
Instead, we'd have to wrap small sections of the test separately and provide
a different exception string for each. The migration test may be split to
four sections: setup, pre-migration, the migration itself and post-migration.
This may not be so bad, but do you still think it's a good solution?
You can have a 'stage' variable and assign it your current stage, and
use it when reporting errors.
Anything is better than a maze of ifs.
There's also another issue: the utility functions that should raise exceptions
don't know what they're being used for, so there can be exception string
collisions. For example, both run_bg(), used for local processes, and
kvm_spawn.get_command_status_output(), used for SSH commands, would say
something like "command %s failed". It would be incorrect to hardcode a more
specific message into any of them (in my opinion).
The message should explain what happened, no more, no less.
The caller has to test for three possible outcomes. Success, failure,
and 'command is running in the background'. If your callsites don't
check for all three, something's wrong.
When we want the command to succeed on time, we just test for status == 0.
If it's a nonzero integer or None, something went wrong, and we should kill
the process just in case it's still running.
But if it's nonzero it has exited already!
However, I feel I'm not making any progress, so I'll stop. I just wish
I could use exceptions in the code I write.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
