Merge pull request #223 from buildkite/ming/pie-2813

Handle http response errors during upoad.

lib/buildkite/test_collector/http_client.rb

@@ -38,7 +38,13 @@ def post_json(data)
 
       contact.body = compressed_body.string
 
-      http.request(contact)
+      response = http.request(contact)
+
+      if response.is_a?(Net::HTTPSuccess)
+        response
+      else
+        raise "HTTP Request Failed: #{response.code} #{response.message}"
+      end
     end
 
     def metadata

lib/buildkite/test_collector/uploader.rb

@@ -25,7 +25,8 @@ def self.traces
       OpenSSL::SSL::SSLError,
       OpenSSL::SSL::SSLErrorWaitReadable,
       EOFError,
-      Errno::ETIMEDOUT
+      Errno::ETIMEDOUT,
+      # TODO: some retries for server-side error would be great.
     ]
 
     def self.tracer
@@ -38,7 +39,7 @@ def self.upload(data)
       http = Buildkite::TestCollector::HTTPClient.new(Buildkite::TestCollector.url)
 
       Thread.new do
-        response = begin
+        begin
           upload_attempts ||= 0
           http.post_json(data)
         rescue *Buildkite::TestCollector::Uploader::RETRYABLE_UPLOAD_ERRORS => e

spec/test_collector/http_client_spec.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'ostruct'
+
 RSpec.describe Buildkite::TestCollector::HTTPClient do
   subject { described_class.new("buildkite.localhost") }
 
@@ -19,6 +21,7 @@
 
   let(:http_double) { double("Net::HTTP_double") }
   let(:post_double) { double("Net::HTTP::Post") }
+  let(:response) { double("Net::HTTPResponse") }
 
   let(:request_body) do
     {
@@ -72,9 +75,19 @@
 
   describe "#post_json" do
     it "sends the right data" do
+      allow(response).to receive(:is_a?).with(Net::HTTPSuccess).and_return(true)
       expect(post_double).to receive(:body=).with(compressed_body)
-      expect(http_double).to receive(:request).with(post_double)
+      expect(http_double).to receive(:request).with(post_double).and_return(response)
       subject.post_json([trace])
     end
+
+    it "throw error in a server error" do
+      allow(response).to receive(:is_a?).with(Net::HTTPSuccess).and_return(false)
+      allow(response).to receive(:code).and_return("500")
+      allow(response).to receive(:message).and_return("Internal Server Error")
+      expect(post_double).to receive(:body=).with(compressed_body)
+      expect(http_double).to receive(:request).with(post_double).and_return(response)
+      expect { subject.post_json([trace]) }.to raise_error(RuntimeError, "HTTP Request Failed: 500 Internal Server Error")
+    end
   end
 end

spec/test_collector/uploader_spec.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+RSpec.describe Buildkite::TestCollector::Uploader do
+  let(:http_client_double) { instance_double(Buildkite::TestCollector::HTTPClient) }
+  let(:api_token) { 'fake_api_token' }
+
+  before do
+    allow(Buildkite::TestCollector::HTTPClient).to receive(:new).and_return(http_client_double)
+    allow(Thread).to receive(:new).and_yield
+    allow(Buildkite::TestCollector).to receive(:api_token).and_return(api_token)
+    allow(Buildkite::TestCollector).to receive(:url).and_return('https://fake-url.com')
+  end
+
+  describe '.upload' do
+    it 'posts data to the HTTP client' do
+      expect(http_client_double).to receive(:post_json).with([{some: 'data'}])
+      described_class.upload([{some: 'data'}])
+    end
+
+    context 'when there is RuntimeError' do
+      before do
+        allow(http_client_double).to receive(:post_json).and_raise(RuntimeError)
+        allow($stderr).to receive(:puts)
+      end
+
+      it 'logs an error message' do
+        expect($stderr).to receive(:puts).with(include("experienced an error when sending your data"))
+        described_class.upload([{some: 'data'}])
+      end
+    end
+  end
+end