Minor fixes and more unit tests for ReleaseNotesUrlReader

src/lib/y2packager/release_notes_url_reader.rb

@@ -17,6 +17,7 @@
 
 Yast.import "Pkg"
 Yast.import "Proxy"
+Yast.import "String"
 
 module Y2Packager
   # This class reads release notes from the relnotes_url product property
@@ -27,6 +28,13 @@ class ReleaseNotesUrlReader
     include Yast::Logger
 
     class << self
+      # Enable downloading release notes
+      #
+      # This method is intended to be used during testing.
+      def enable!
+        @enabled = true
+      end
+
       # Disable downloading release notes due to communication errors
       def disable!
         @enabled = false
@@ -37,7 +45,8 @@ def disable!
       # @return [Boolean]
       # @see disable!
       def enabled?
-        @enabled.nil? ? true : @enabled
+        return true if @enabled.nil? # default value
+        @enabled
       end
 
       # Blacklist of URLs that failed to download and should not be retried
@@ -76,7 +85,6 @@ def clear_blacklist
       28 => "Operation timeout."
     }.freeze
 
-
     # Product to get release notes for
     attr_reader :product
 
@@ -93,11 +101,12 @@ def initialize(product)
     # release notes for a language "xx_XX" are not found, it will fallback to
     # "xx".
     #
-    # @param user_lang [String] Release notes language (falling back to "en")
-    # @param format    [Symbol] Release notes format (:txt or :rtf)
+    # @param user_lang     [String]  User preferred language (falling back to fallback_lang)
+    # @param format        [Symbol] Release notes format (:txt or :rtf)
+    # @param fallback_lang [String] Release notes fallback language
     # @return [String,nil] Release notes or nil if a release notes were not found
     #   (no package providing release notes or notes not found in the package)
-    def release_notes(user_lang: "en_US", format: :txt)
+    def release_notes(user_lang: "en_US", format: :txt, fallback_lang: "en")
       if !self.class.enabled?
         log.info("Skipping release notes download due to previous download issues")
         return nil
@@ -108,9 +117,12 @@ def release_notes(user_lang: "en_US", format: :txt)
         return nil
       end
 
-      return unless relnotes_url_valid?
+      if !relnotes_url_valid?
+        log.warn("Skipping release notes download for #{product.name}: '#{relnotes_url}'")
+        return nil
+      end
 
-      release_notes = fetch_release_notes(user_lang, format)
+      release_notes = fetch_release_notes(user_lang, format, fallback_lang)
 
       return release_notes if release_notes
 
@@ -124,12 +136,14 @@ def release_notes(user_lang: "en_US", format: :txt)
     #
     # It relies on #release_notes_content to get release notes content.
     #
-    # @param user_lang [String] Release notes language (falling back to "en")
-    # @param format    [Symbol] Release notes format (:txt or :rtf)
+    # @param user_lang     [String] Release notes language (falling back to fallback_lang)
+    # @param format        [Symbol] Release notes format (:txt or :rtf)
+    # @param fallback_lang [String] Release notes fallback language
     # @return [String,nil] Release notes or nil if a release notes were not found
     # @see release_notes_content
-    def fetch_release_notes(user_lang, format)
-      content, lang = release_notes_content(user_lang, format)
+    def fetch_release_notes(user_lang, format, fallback_lang)
+      content, lang = release_notes_content(user_lang, format, fallback_lang)
+      return nil if content.nil?
 
       Y2Packager::ReleaseNotes.new(
         product_name: product.name,
@@ -151,16 +165,17 @@ def latest_version
       :latest
     end
 
-    FALLBACK_LANG = "en".freeze
-
     # Return release notes content
     #
+    # @param user_lang     [String] Release notes language (falling back to fallback_lang)
+    # @param format        [Symbol] Release notes format (:txt or :rtf)
+    # @param fallback_lang [String] Release notes fallback language
     # @return [String,nil] Return release notes content or nil if it release
     #   notes were not found
-    def release_notes_content(user_lang, format)
+    def release_notes_content(user_lang, format, fallback_lang)
       langs = [user_lang]
       langs << user_lang.split("_", 2).first if user_lang.include?("_")
-      langs << FALLBACK_LANG
+      langs << fallback_lang
 
       langs.each do |lang|
         content = release_notes_content_for_lang_and_format(lang, format)
@@ -173,14 +188,15 @@ def release_notes_content(user_lang, format)
     # Return release notes content for a given language
     def release_notes_content_for_lang_and_format(lang, format)
       # If there is an index and the language is not indexed
+      release_notes_index
       return nil unless release_notes_index.empty? || indexed_release_notes_for?(lang, format)
 
+
       # Where we want to store the downloaded release notes
       filename = Yast::Builtins.sformat(
         "%1/relnotes", Yast::SCR.Read(Yast::Path.new(".target.tmpdir"))
       )
 
-      # download release notes now
       return nil unless curl_download(release_notes_file_url(lang, format), filename)
 
       log.info("Release notes downloaded successfully")
@@ -319,12 +335,15 @@ def download_release_notes_index(url_base)
     end
 
     # Download *url* to *filename*
-    # May set InstData.stop_relnotes_download on download failure.
+    #
+    # May disable release notes downloading by calling .disable!.
     #
     # @return [Boolean,nil] true: success, false: failure, nil: failure+dont retry
     def curl_download(url, filename, max_time: 300)
+      return nil unless self.class.enabled?
       cmd = Yast::Builtins.sformat(
-        "/usr/bin/curl --location --verbose --fail --max-time %6 --connect-timeout 15  %1 '%2' --output '%3' > '%4/%5' 2>&1",
+        "/usr/bin/curl --location --verbose --fail --max-time %6 " \
+        "--connect-timeout 15  %1 '%2' --output '%3' > '%4/%5' 2>&1",
         curl_proxy_args,
         url,
         Yast::String.Quote(filename),

test/lib/release_notes_url_reader_test.rb

@@ -10,18 +10,23 @@
   let(:product) { instance_double(Y2Packager::Product, name: "dummy") }
   let(:relnotes_url) { "http://doc.opensuse.org/openSUSE/release-notes-openSUSE.rpm" }
   let(:language) { double("Yast::Language", language: "de_DE") }
-  let(:release_notes_index) { [] }
+  let(:curl_retcode) { 0 }
 
   before do
     allow(Yast::Pkg).to receive(:ResolvableProperties)
       .with(product.name, :product, "").and_return(["relnotes_url" => relnotes_url])
-    allow(reader).to receive(:release_notes_index).and_return(release_notes_index)
-    allow(reader).to receive(:release_notes_index).and_return(release_notes_index)
     allow(Yast::SCR).to receive(:Read).with(Yast::Path.new(".target.tmpdir"))
       .and_return("/tmp")
     allow(Yast::SCR).to receive(:Read).with(Yast::Path.new(".target.string"), /relnotes/)
       .and_return("Release Notes\n")
+    allow(Yast::SCR).to receive(:Execute)
+      .with(Yast::Path.new(".target.bash"), /curl.*directory.yast/)
+      .and_return(1)
+    allow(Yast::SCR).to receive(:Execute)
+      .with(Yast::Path.new(".target.bash"), /curl.*RELEASE-NOTES/)
+      .and_return(curl_retcode)
     described_class.clear_blacklist
+    described_class.enable!
 
     stub_const("Yast::Language", language)
     stub_const("Yast::Proxy", double("proxy"))
@@ -47,6 +52,17 @@
       expect(rn.version).to eq(:latest)
     end
 
+    it "uses cURL to download release notes" do
+      allow(reader).to receive(:curl_proxy_args).and_return("--proxy http://proxy.example.com")
+
+      cmd = "/usr/bin/curl --location --verbose --fail --max-time 300 --connect-timeout 15  " \
+        "--proxy http://proxy.example.com '#{reader.release_notes_file_url("de_DE", :txt)}' " \
+        "--output '/tmp/relnotes' > '/var/log/YaST2/curl_log' 2>&1"
+
+      expect(Yast::SCR).to receive(:Execute).with(Yast::Path.new(".target.bash"), cmd)
+      reader.release_notes(user_lang: "de_DE", format: :txt)
+    end
+
     context "when release notes are not found for the given language" do
       let(:user_lang) { "de_DE" }
 
@@ -92,12 +108,17 @@
     end
 
     context "when release notes index exists" do
+      before do
+        allow(Yast::SCR).to receive(:Execute)
+          .with(Yast::Path.new(".target.bash"), /curl.*directory.yast/)
+          .and_return(0)
+        allow(File).to receive(:read).with(/directory.yast/)
+          .and_return(release_notes_index)
+      end
+
       context "and wanted release notes are registered in that file" do
         let(:release_notes_index) do
-          [
-            "RELEASE-NOTES.de_DE.txt",
-            "RELEASE-NOTES.en_US.txt"
-          ]
+          "RELEASE-NOTES.de_DE.txt\nRELEASE-NOTES.en_US.txt"
         end
 
         it "tries to download release notes" do
@@ -110,9 +131,7 @@
 
       context "and wanted release notes are not registered in that file" do
         let(:release_notes_index) do
-          [
-            "RELEASE-NOTES.en_US.txt"
-          ]
+          "RELEASE-NOTES.en_US.txt"
         end
 
         it "does not try to download release notes" do
@@ -122,6 +141,86 @@
         end
       end
     end
+
+    context "when release notes are not found" do
+      let(:curl_retcode) { 1 }
+
+      it "blacklists the URL" do
+        expect(described_class).to receive(:add_to_blacklist).with(reader.relnotes_url)
+        reader.release_notes
+      end
+    end
+
+    context "when a connection problem happens" do
+      let(:curl_retcode) { 5 }
+
+      it "disables downloading release notes via relnotes_url" do
+        expect(described_class).to receive(:disable!).and_call_original
+        reader.release_notes
+      end
+    end
+
+    context "when release notes URL is not valid" do
+      let(:relnotes_url) { "http" }
+
+      it "returns nil" do
+        expect(reader.release_notes).to be_nil
+      end
+    end
+
+    context "when release notes URL is nil" do
+      let(:relnotes_url) { nil }
+
+      it "returns nil" do
+        expect(reader.release_notes).to be_nil
+      end
+    end
+
+    context "when release notes URL is empty" do
+      let(:relnotes_url) { "" }
+
+      it "returns nil" do
+        expect(reader.release_notes).to be_nil
+      end
+    end
+
+    context "when relnotes_url is blacklisted" do
+      before do
+        described_class.add_to_blacklist(relnotes_url)
+      end
+
+      it "returns nil" do
+        expect(reader.release_notes).to be_nil
+      end
+
+      it "does not tries to download anything" do
+        expect(Yast::SCR).to_not receive(:Execute)
+          .with(Yast::Path.new(".target.bash"), /curl/)
+        reader.release_notes
+      end
+    end
+
+    context "when release notes downloading is disabled" do
+      before do
+        described_class.disable!
+      end
+
+      it "returns nil" do
+        expect(reader.release_notes).to be_nil
+      end
+
+      it "does not tries to download anything" do
+        expect(Yast::SCR).to_not receive(:Execute)
+          .with(Yast::Path.new(".target.bash"), /curl/)
+        reader.release_notes
+      end
+    end
+  end
+
+  describe "#latest_version" do
+    it "returns :latest" do
+      expect(reader.latest_version).to eq(:latest)
+    end
   end
 
   describe "#curl_proxy_args" do