Separate systemsettings test for KDE4-based and KF5-based #960
Conversation
nilxam
force-pushed
the
go_systemsettings5
branch
from
54f44b4
to
0d54be3
Compare
nilxam
changed the title
|
looks ok but don't want to approve by myself as I don't know the use of the variables good enough. Please wait for others for their feedback |
| my $self = shift; | ||
| x11_start_program("systemsettings5", 6, {valid => 1}); | ||
| if (get_var("LIVETEST")) { | ||
| assert_screen 'test-systemsettings-1', 15; |
There was a problem hiding this comment.
what if here would be just assert_screen with default timeout ?
There was a problem hiding this comment.
but is it wroth 30s here? especially in old systemsettings test that non-LIVETEST block is use 3s...
There was a problem hiding this comment.
No idea what's the point of this 'if' actually. If the test fails in 3 or 30 seconds is not so important - it shouldn't fail at all.
There was a problem hiding this comment.
if there is needle match, it will continue after 1 second
btw default timeout is In today ;)
There was a problem hiding this comment.
You know, as @coolo pointed in some other PR. When people start copy-pasting existing code IMO its better with defaults rather then arbitrary options valid only for some distant test.
Personally I think for assert_screen always use default unless you need to wait longer. Shorter timeout will not speed up the test.
For check_screen that's different and I see there are use cases for shorter timeouts and ideally there should be accompanying comment explaining why shorter timeout (\me going full @okurz ) :)
There was a problem hiding this comment.
that's the road to madness :)
There was a problem hiding this comment.
well, as I said above, I don't mind to use default timeout. and I don't know what is the current agreement/rule to treat timeout with assert_screen and check_screen.
oh, and yes, when I copy the new test from the old one I haven't aware I should change the content besides program name :)
There was a problem hiding this comment.
@nilxam sysrich explained it pretty well in his PR #945 that is why I suggested to improve the documentation in https://progress.opensuse.org/issues/10380 so it will be made more clear.
that's the road to madness :)
@coolo according to your statements in the near past it seems there are many roads that lead to madness ;-)
There was a problem hiding this comment.
that's true - mad, isn't it?
nilxam
force-pushed
the
go_systemsettings5
branch
from
0d54be3
to
da9f225
Compare
In update test, there might have old KDE4 systemsettings as another candidate in krunner via auto-completion, therefore, separate systemsettings test to systemsettings(KDE4-based) and systemsettings5(KF5-based) test. openSUSE version less than or equal to 13.2 have to set KDE4 variable as 1, thus PLASMA5 variable won't be sets.
nilxam
force-pushed
the
go_systemsettings5
branch
from
da9f225
to
6e86558
Compare
|
LGTM |
Separate systemsettings test for KDE4-based and KF5-based
In update test, there might have old KDE4 systemsettings as another candidate in krunner via auto-completion, therefore, separate systemsettings test to systemsettings(KDE4-based) and systemsettings5(KF5-based) test.
openSUSE version less than or equal to 13.2 have to set KDE4 variable as 1, thus PLASMA5 variable won't be sets.