report warning for none existing targets #187

schubi2 · 2019-02-08T09:48:46Z

No description provided.

coveralls · 2019-02-08T09:52:35Z

Coverage increased (+0.1%) to 63.084% when pulling 43152a1 on master-bsc-1046083 into 1b9392b on master.

dgdavid

Codewise looks good to me. Just a np/suggestion :)

dgdavid · 2019-02-08T10:52:03Z

src/modules/services_manager_target.rb

      log.info('Saving default target...')
-      Yast2::Systemd::Target.set_default(default_target)
+      unless Yast2::Systemd::Target.find(self.default_target)


NP: matter of taste, I guess. I'd extract this to its own method, something like

def save return true unless modified? ensure_target log.info("saving...") Yast2::Systemd::Target.set_default(self.default_target) end def ensure_target return if Yast2::Systemd::Target.find(self.default_target) log.info("#{self.default_target} not found, using mode fallback instead") Report.Warning( _("...") ) end

joseivanlopez

Some comments and a general question. Are we allowing to select targets that are not available in the system?

joseivanlopez · 2019-02-08T11:26:58Z

test/services_manager_target_test.rb

        end
      end

-      context "when the default target has been changed" do
+      context "when the default target has not been changed" do
        let(:target) { "multi-user" }

        it "returns true" do


Please, fix text: "returns false"

joseivanlopez · 2019-02-08T11:27:21Z

test/services_manager_target_test.rb

@@ -136,19 +154,19 @@ class Template < Struct.new(
        subject.default_target = target
      end

-      context "when the default target has not been changed" do
+      context "when the default target has been changed" do
        let(:target) { "graphical" }

        it "returns false" do


Please, fix text: "returns true"

joseivanlopez · 2019-02-08T11:35:08Z

src/modules/services_manager_target.rb

      log.info('Saving default target...')
-      Yast2::Systemd::Target.set_default(default_target)
+      unless Yast2::Systemd::Target.find(self.default_target)


Please, don't use unless for multiliner if. Otherwise, rubocop complains with "Favor modifier unless usage when having a single-line body". Yes, maybe this package is not using rubocop at all, but even though we should follow the standard.

joseivanlopez · 2019-02-08T11:44:26Z

src/modules/services_manager_target.rb

-      Yast2::Systemd::Target.set_default(default_target)
+      unless Yast2::Systemd::Target.find(self.default_target)
+        # TRANSLATORS: error popup, %s is the default target e.g. graphical
+         Report.Warning(_("Cannot find default target '%s' which is not available," \


The warning sounds a bit weird to me. Why not something like:

Report.Warning(_("Target '%s' is not available. Using text mode as default target.") % default_target)

Note that self is not required to get the default_target value.

joseivanlopez · 2019-02-08T11:45:19Z

src/modules/services_manager_target.rb

+                          "using the text mode fallback.") % self.default_target)
+         self.default_target = BaseTargets::MULTIUSER
+      end
+      Yast2::Systemd::Target.set_default(self.default_target)


Again, self is not required:

Yast2::Systemd::Target.set_default(default_target)

joseivanlopez · 2019-02-08T11:52:32Z

test/services_manager_target_test.rb


-        it "saves the changes in the underlying system" do
+        it "reports a warning and sets to multi-user" do
+          expect(Yast2::Systemd::Target).to receive(:find).and_return(nil)


I guess you are using this line to make the target unavailable. If so, I prefer to do it in a before block, that is:

Suggested change

expect(Yast2::Systemd::Target).to receive(:find).and_return(nil)

context "when default target is not available" do

let(:target) { "graphical" }

before do

allow(Yast2::Systemd::Target).to receive(:find).with(target).and_return(nil)

end

it "reports a warning" do

expect(Yast::Report).to receive(:Warning)

subject.save

end

it "sets 'multi-user' as default target" do

expect(Yast2::Systemd::Target).to receive(:set_default).with("multi-user")

subject.save

end

end

That does not work because you are calling subject.save twice and after the first call you have already set the default_target to mutli-user. I must admit, my first try has also looked like that :-)

Ah, I see. If this happens again in the future, maybe a solution is to reset the default_target inside the before block.

schubi2 · 2019-02-08T13:28:55Z

Some comments and a general question. Are we allowing to select targets that are not available in the system?
Yes, in AutoYaST

schubi2 · 2019-02-08T13:44:10Z

src/modules/services_manager_target.rb

+      # TRANSLATORS: error popup, %s is the default target e.g. graphical
+      Report.Warning(_("Target '%s' is not available." \
+                       " Using text mode as default target.") % default_target)
+      self.default_target = BaseTargets::MULTIUSER


The self is needed because ruby is taking default_target as a local variable here. That's why I have set the rest to self.... too in order to be consequent. I have no idea why ruby is so here. Just try it out in the testcase :-)

As commented in IRC, here it is needed because in the setter is not clear if you want to invoke the method or to define a new local variable (ruby stuff xD).

joseivanlopez

LGTM

yast-bot · 2019-02-08T14:05:27Z

❌ Public Jenkins job #23 failed

yast-bot · 2019-02-08T14:11:16Z

✔️ Internal Jenkins job #22 successfully finished
✔️ Created IBS submit request #183977

yast-bot · 2019-02-12T13:26:05Z

✔️ Public Jenkins job #24 successfully finished
✔️ Created OBS submit request #673922

report error for none existing targets

551a5d2

schubi2 changed the title ~~report error for none existing targets~~ WIP report error for none existing targets Feb 8, 2019

schubi2 added 4 commits February 8, 2019 11:08

removed unneeded logging

1670b67

packaging

6df9c07

spelling

ea73b08

spelling

ec604b1

schubi2 changed the title ~~WIP report error for none existing targets~~ report warning for none existing targets Feb 8, 2019

schubi2 mentioned this pull request Feb 8, 2019

[WIP] AutoYaST X11 test #125

Closed

dgdavid reviewed Feb 8, 2019

View reviewed changes

dgdavid approved these changes Feb 8, 2019

View reviewed changes

joseivanlopez reviewed Feb 8, 2019

View reviewed changes

adapted suggestions

43152a1

schubi2 commented Feb 8, 2019

View reviewed changes

joseivanlopez approved these changes Feb 8, 2019

View reviewed changes

schubi2 merged commit 967720e into master Feb 8, 2019

schubi2 deleted the master-bsc-1046083 branch February 8, 2019 14:05

schubi2 mentioned this pull request Feb 13, 2019

Upgrade: Do not write default target if it has not been set #188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report warning for none existing targets #187

report warning for none existing targets #187

schubi2 commented Feb 8, 2019

coveralls commented Feb 8, 2019 •

edited

dgdavid left a comment

dgdavid Feb 8, 2019 •

edited

joseivanlopez left a comment

joseivanlopez Feb 8, 2019

joseivanlopez Feb 8, 2019

joseivanlopez Feb 8, 2019

joseivanlopez Feb 8, 2019

joseivanlopez Feb 8, 2019

joseivanlopez Feb 8, 2019

schubi2 Feb 8, 2019

joseivanlopez Feb 8, 2019

schubi2 commented Feb 8, 2019

schubi2 Feb 8, 2019

joseivanlopez Feb 8, 2019

joseivanlopez left a comment

yast-bot commented Feb 8, 2019

yast-bot commented Feb 8, 2019

yast-bot commented Feb 12, 2019

-          expect(Yast2::Systemd::Target).to receive(:find).and_return(nil)
+context "when default target is not available" do
+  let(:target) { "graphical" }
+  before do
+    allow(Yast2::Systemd::Target).to receive(:find).with(target).and_return(nil)
+  end
+  it "reports a warning" do
+    expect(Yast::Report).to receive(:Warning)
+    subject.save
+  end
+  it "sets 'multi-user' as default target" do
+    expect(Yast2::Systemd::Target).to receive(:set_default).with("multi-user")
+    subject.save
+  end
+end

report warning for none existing targets #187

report warning for none existing targets #187

Conversation

schubi2 commented Feb 8, 2019

coveralls commented Feb 8, 2019 • edited

dgdavid left a comment

Choose a reason for hiding this comment

dgdavid Feb 8, 2019 • edited

Choose a reason for hiding this comment

joseivanlopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schubi2 commented Feb 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseivanlopez left a comment

Choose a reason for hiding this comment

yast-bot commented Feb 8, 2019

yast-bot commented Feb 8, 2019

yast-bot commented Feb 12, 2019

coveralls commented Feb 8, 2019 •

edited

dgdavid Feb 8, 2019 •

edited