Add post_fail_hook for boot_to_desktop #6370
Conversation
DrMullings
force-pushed
the
boot_to_desktop_postfail
branch
2 times, most recently
from
ca0c4f2
to
658e4d1
Compare
tests/boot/boot_to_desktop.pm
Outdated
|
|
||
| # if we found a shell, we do not need the memory dump | ||
| if (!(match_has_tag('emergency-shell') or match_has_tag('emergency-mode'))) { | ||
| die "save_memory_dump is temporarily unavailable, see https://progress.opensuse.org/issues/19390"; |
There was a problem hiding this comment.
As this die is executed without any condition, the rest of the commands in the if block won't ever get executed.
There was a problem hiding this comment.
You're right, but this code is from first_boot so we should also change it there
There was a problem hiding this comment.
Then we should maybe reuse the code.
|
bump |
DrMullings
force-pushed
the
boot_to_desktop_postfail
branch
from
658e4d1
to
ee909e0
Compare
DrMullings
changed the title
|
Removed dead code |
DrMullings
force-pushed
the
boot_to_desktop_postfail
branch
6 times, most recently
from
f8f4a05
to
8e666f0
Compare
tests/boot/boot_to_desktop.pm
Outdated
| save_memory_dump; | ||
| } | ||
|
|
||
| # crosscheck for text login on tty1 |
There was a problem hiding this comment.
the thought is to check if the system completely fails to boot or just fails booting to the desktop
tests/boot/boot_to_desktop.pm
Outdated
| } | ||
|
|
||
| # crosscheck for text login on tty1 | ||
| select_console 'root-console'; |
There was a problem hiding this comment.
Normally we use "log-console" because the "root-console" might have some clutter on it preventing it to be usable
There was a problem hiding this comment.
since its not the intention to actually use the console this should not make a difference?
There was a problem hiding this comment.
hm, I see. So if the login works then we can collect logs otherwise we are lost anyway?
|
Closing this in the progress of the discussion and now working on a common base with a proper booting post_fail_hook |
DrMullings
force-pushed
the
boot_to_desktop_postfail
branch
from
8e666f0
to
0c6d373
Compare
|
I added a 'bootbasetest' and used it in said modules, please give feedback |
DrMullings
force-pushed
the
boot_to_desktop_postfail
branch
6 times, most recently
from
018d0a0
to
d9c163e
Compare
|
looks nice but still please provide a verification run |
DrMullings
force-pushed
the
boot_to_desktop_postfail
branch
from
d9c163e
to
dfe2485
Compare
|
Verification run: http://pinky.arch.suse.de/tests/81 |
Added a baseclass with a generic post_fail_hook for tests that boot a system
DrMullings
force-pushed
the
boot_to_desktop_postfail
branch
from
dfe2485
to
3c21a54
Compare
|
|
||
| # try to save logs as a last resort | ||
| $self->export_logs(); | ||
| bootbasetest->post_fail_hook(); |
There was a problem hiding this comment.
you might want to export the function, although I didn't know that calling directly with -> could work. The other option that I know is bootbasetest::post_fail_hook but it is more used in our shared code the first one I think.
There was a problem hiding this comment.
Thanks for the hint, I was wondering like an Idiot why it did not work as expected :D
There was a problem hiding this comment.
Hmm after taking a look at y2logstep and opensusebasetest I wonder if it is needed to export the function, none of those baseclasses do it?
There was a problem hiding this comment.
for example here is calling the parent because is overriding the function in base but you use it as utility, not as base. As a base you can also call it sometime with $self, it depends on the case.
As a utility library I would say that you can use this approach to export it.
There was a problem hiding this comment.
I wouldn't export it. That makes accidental calls to that function more likely.
Not sure why the :: syntax doesn't work but -> should be fine, too (since the function does not evaluate any arguments anyways).
There was a problem hiding this comment.
As an alternative I think you could use multiple inheritance, e.g. use base qw(foo bar)
There was a problem hiding this comment.
@okurz I saw the same alternative, and would actually create a ticket or suggest @DrMullings to come with this approach, but the pr has been hanging here for so long... that it started to get sad and feel forgotten...
There was a problem hiding this comment.
As I already told @foursixnine multiple inheritance sounds like opening the gates to the hell of ambiguity, we should not do it as long as we have a working state
|
Are we good to go here? |
foursixnine
dismissed
okurz’s stale review
No further comments from @okurz so far
|
If it breaks it breaks :) There are few improvements that we can do as a followup for this |
| @@ -15,6 +15,7 @@ | |||
|
|
|||
| use strict; | |||
| use base "y2logsstep"; | |||
| use bootbasetest; | |||
There was a problem hiding this comment.
@DrMullings Looking at this again... changing this line to use base "bootbasetest' and moving it above y2logsstep should do the trick too. (use base qw(bootbasetest y2logsstep) should also do the trick and call the post_fail_hook from bootbasetest.
Added a basic post_fail_hook for boot_to_desktop
Opened for discussion what other steps we could introduce here