[needs-restarting] add opt using config file (RhBug:1810123) #814
Conversation
inknos
force-pushed
the
ns-needs-restarting-list
branch
2 times, most recently
from
5554f0f
to
5fc88aa
Compare
inknos
force-pushed
the
ns-needs-restarting-list
branch
from
5fc88aa
to
19cfe26
Compare
| Given I enable plugin "needs_restarting" | ||
| And I use repository "dnf-ci-fedora" | ||
| And I move the clock backward to "before boot-up" | ||
| And I execute dnf with args "install lame kernel basesystem glibc wget abcde" |
There was a problem hiding this comment.
Looks like this list of packages is unnecessary, you're only ever using wget and abcde from the list?
While not necessary, I'd prefer if you set up a single repository with the 3 packages you're using and used only that in the feature file, to make it a bit cleaner... I'm not requiring it though.
| Scenario: restarting needed after updating package in conf files | ||
| Given I execute dnf with args "upgrade wget" | ||
| When I execute dnf with args "needs-restarting -r" | ||
| Then the exit code is 1 |
There was a problem hiding this comment.
Unrelated to your PR, so consider this a rhetorical question 🙂 but is it really returning a non-zero exit status when there are packages to restart? That doesn't make sense...
dnf-behave-tests/features/plugins-core/needs-restarting-conf-files.feature
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,21 @@ | |||
| Name: test | |||
| Version: 1.0.0 | |||
| Release: 1 | |||
There was a problem hiding this comment.
Nitpick: the release actually doesn't contain "fc29", which you have in the filename, you could remove it, but whatever... 🙂
dnf-behave-tests/features/plugins-core/needs-restarting-conf-files.feature
Outdated
Show resolved
Hide resolved
dnf-behave-tests/features/plugins-core/needs-restarting-conf-files.feature
Outdated
Show resolved
Hide resolved
inknos
force-pushed
the
ns-needs-restarting-list
branch
from
19cfe26
to
2c1349b
Compare
dnf-behave-tests/features/plugins-core/needs-restarting-conf-files.feature
Outdated
Show resolved
Hide resolved
| Given I create and substitute file "/etc/dnf/plugins/needs-restarting.d/abcde.conf" with | ||
| """ | ||
| abcde | ||
| """ |
There was a problem hiding this comment.
I'm not really sure at what indent we are putting the text blocks, but this is inconsistent with the other text blocks in this file, please unify it (more occurrences below).
| abcde | ||
| """ | ||
| And I execute dnf with args "upgrade wget abcde" | ||
| Then the exit code is 0 |
There was a problem hiding this comment.
This actually is a setup step to test the needs-restarting functionality. You should use this instead of the two above lines:
Given I successfully execute dnf with args "upgrade wget abcde"
(Or, in this case Given is the active step so you should use And)
More occurrences of this below.
| @@ -0,0 +1,22 @@ | |||
| Name: abcde | |||
| Version: 2.9.3 | |||
There was a problem hiding this comment.
I prefer to use simple versions for the test packages, like 1.0, so that you don't need to squint when you have multiple versions of a package like 3.14.1.1 and 3.14.7.1 and try to figure out which is which 😎 But feel free to keep it here, not much of an issue in this case.
| BuildArch: noarch | ||
| Requires: wget | ||
| Recommends: flac | ||
| Suggests: lame |
There was a problem hiding this comment.
There are attributes on the packages like the above which are irrelevant to the test and therefore superfluous (also the lengthy %descriptions). Again, not hurting anything and they are not too long, so feel free to keep them but in general it's good practice to keep the test data minimal to the test case.
inknos
force-pushed
the
ns-needs-restarting-list
branch
from
2c1349b
to
a964d48
Compare
inknos
force-pushed
the
ns-needs-restarting-list
branch
from
a964d48
to
d31c506
Compare
dnf-plugins-core: rpm-software-management/dnf-plugins-core#391
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1810123