Merge pull request #8101 from yyforyongyu/fix-web-fee-unit-test

chainfee: Fix test flake in `TestWebAPIFeeEstimator`

lnwallet/chainfee/estimator.go

@@ -35,6 +35,14 @@ const (
 	// maxFeeUpdateTimeout represents the maximum interval in which a
 	// WebAPIEstimator will request fresh fees from its API.
 	maxFeeUpdateTimeout = 20 * time.Minute
+
+	// WebAPIConnectionTimeout specifies the timeout value for connecting
+	// to the api source.
+	WebAPIConnectionTimeout = 5 * time.Second
+
+	// WebAPIResponseTimeout specifies the timeout value for receiving a
+	// fee response from the api source.
+	WebAPIResponseTimeout = 10 * time.Second
 )
 
 var (
@@ -493,15 +501,9 @@ var _ Estimator = (*BitcoindEstimator)(nil)
 // implementation of this interface in order to allow the WebAPIEstimator to
 // be fully generic in its logic.
 type WebAPIFeeSource interface {
-	// GenQueryURL generates the full query URL. The value returned by this
-	// method should be able to be used directly as a path for an HTTP GET
-	// request.
-	GenQueryURL() string
-
-	// ParseResponse attempts to parse the body of the response generated
-	// by the above query URL. Typically this will be JSON, but the
-	// specifics are left to the WebAPIFeeSource implementation.
-	ParseResponse(r io.Reader) (map[uint32]uint32, error)
+	// GetFeeMap will query the web API, parse the response and return a
+	// map of confirmation targets to sat/kw fees.
+	GetFeeMap() (map[uint32]uint32, error)
 }
 
 // SparseConfFeeSource is an implementation of the WebAPIFeeSource that utilizes
@@ -513,21 +515,12 @@ type SparseConfFeeSource struct {
 	URL string
 }
 
-// GenQueryURL generates the full query URL. The value returned by this
-// method should be able to be used directly as a path for an HTTP GET
-// request.
-//
-// NOTE: Part of the WebAPIFeeSource interface.
-func (s SparseConfFeeSource) GenQueryURL() string {
-	return s.URL
-}
-
-// ParseResponse attempts to parse the body of the response generated by the
+// parseResponse attempts to parse the body of the response generated by the
 // above query URL. Typically this will be JSON, but the specifics are left to
 // the WebAPIFeeSource implementation.
-//
-// NOTE: Part of the WebAPIFeeSource interface.
-func (s SparseConfFeeSource) ParseResponse(r io.Reader) (map[uint32]uint32, error) {
+func (s SparseConfFeeSource) parseResponse(r io.Reader) (
+	map[uint32]uint32, error) {
+
 	type jsonResp struct {
 		FeeByBlockTarget map[uint32]uint32 `json:"fee_by_block_target"`
 	}
@@ -543,6 +536,47 @@ func (s SparseConfFeeSource) ParseResponse(r io.Reader) (map[uint32]uint32, erro
 	return resp.FeeByBlockTarget, nil
 }
 
+// GetFeeMap will query the web API, parse the response and return a map of
+// confirmation targets to sat/kw fees.
+func (s SparseConfFeeSource) GetFeeMap() (map[uint32]uint32, error) {
+	// Rather than use the default http.Client, we'll make a custom one
+	// which will allow us to control how long we'll wait to read the
+	// response from the service. This way, if the service is down or
+	// overloaded, we can exit early and use our default fee.
+	netTransport := &http.Transport{
+		Dial: (&net.Dialer{
+			Timeout: WebAPIConnectionTimeout,
+		}).Dial,
+		TLSHandshakeTimeout: WebAPIConnectionTimeout,
+	}
+	netClient := &http.Client{
+		Timeout:   WebAPIResponseTimeout,
+		Transport: netTransport,
+	}
+
+	// With the client created, we'll query the API source to fetch the URL
+	// that we should use to query for the fee estimation.
+	targetURL := s.URL
+	resp, err := netClient.Get(targetURL)
+	if err != nil {
+		log.Errorf("unable to query web api for fee response: %v",
+			err)
+		return nil, err
+	}
+	defer resp.Body.Close()
+
+	// Once we've obtained the response, we'll instruct the WebAPIFeeSource
+	// to parse out the body to obtain our final result.
+	feesByBlockTarget, err := s.parseResponse(resp.Body)
+	if err != nil {
+		log.Errorf("unable to parse fee api response: %v", err)
+
+		return nil, err
+	}
+
+	return feesByBlockTarget, nil
+}
+
 // A compile-time assertion to ensure that SparseConfFeeSource implements the
 // WebAPIFeeSource interface.
 var _ WebAPIFeeSource = (*SparseConfFeeSource)(nil)
@@ -762,38 +796,11 @@ func (w *WebAPIEstimator) getCachedFee(numBlocks uint32) (uint32, error) {
 
 // updateFeeEstimates re-queries the API for fresh fees and caches them.
 func (w *WebAPIEstimator) updateFeeEstimates() {
-	// Rather than use the default http.Client, we'll make a custom one
-	// which will allow us to control how long we'll wait to read the
-	// response from the service. This way, if the service is down or
-	// overloaded, we can exit early and use our default fee.
-	netTransport := &http.Transport{
-		Dial: (&net.Dialer{
-			Timeout: 5 * time.Second,
-		}).Dial,
-		TLSHandshakeTimeout: 5 * time.Second,
-	}
-	netClient := &http.Client{
-		Timeout:   time.Second * 10,
-		Transport: netTransport,
-	}
-
-	// With the client created, we'll query the API source to fetch the URL
-	// that we should use to query for the fee estimation.
-	targetURL := w.apiSource.GenQueryURL()
-	resp, err := netClient.Get(targetURL)
-	if err != nil {
-		log.Errorf("unable to query web api for fee response: %v",
-			err)
-		return
-	}
-	defer resp.Body.Close()
-
 	// Once we've obtained the response, we'll instruct the WebAPIFeeSource
 	// to parse out the body to obtain our final result.
-	feesByBlockTarget, err := w.apiSource.ParseResponse(resp.Body)
+	feesByBlockTarget, err := w.apiSource.GetFeeMap()
 	if err != nil {
-		log.Errorf("unable to query web api for fee response: %v",
-			err)
+		log.Errorf("unable to get fee response: %v", err)
 		return
 	}
 

lnwallet/chainfee/estimator_test.go

@@ -3,27 +3,12 @@ package chainfee
 import (
 	"bytes"
 	"encoding/json"
-	"io"
-	"reflect"
 	"testing"
 
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/stretchr/testify/require"
 )
 
-type mockSparseConfFeeSource struct {
-	url  string
-	fees map[uint32]uint32
-}
-
-func (e mockSparseConfFeeSource) GenQueryURL() string {
-	return e.url
-}
-
-func (e mockSparseConfFeeSource) ParseResponse(r io.Reader) (map[uint32]uint32, error) {
-	return e.fees, nil
-}
-
 // TestFeeRateTypes checks that converting fee rates between the
 // different types that represent fee rates and calculating fees
 // work as expected.
@@ -113,10 +98,6 @@ func TestSparseConfFeeSource(t *testing.T) {
 	// Test that GenQueryURL returns the URL as is.
 	url := "test"
 	feeSource := SparseConfFeeSource{URL: url}
-	queryURL := feeSource.GenQueryURL()
-	if queryURL != url {
-		t.Fatalf("expected query URL of %v, got %v", url, queryURL)
-	}
 
 	// Test parsing a properly formatted JSON API response.
 	// First, create the response as a bytes.Reader.
@@ -125,17 +106,17 @@ func TestSparseConfFeeSource(t *testing.T) {
 		2: 42,
 		3: 54321,
 	}
-	testJSON := map[string]map[uint32]uint32{"fee_by_block_target": testFees}
+	testJSON := map[string]map[uint32]uint32{
+		"fee_by_block_target": testFees,
+	}
 	jsonResp, err := json.Marshal(testJSON)
 	require.NoError(t, err, "unable to marshal JSON API response")
 	reader := bytes.NewReader(jsonResp)
 
 	// Finally, ensure the expected map is returned without error.
-	fees, err := feeSource.ParseResponse(reader)
+	fees, err := feeSource.parseResponse(reader)
 	require.NoError(t, err, "unable to parse API response")
-	if !reflect.DeepEqual(fees, testFees) {
-		t.Fatalf("expected %v, got %v", testFees, fees)
-	}
+	require.Equal(t, testFees, fees, "unexpected fee map returned")
 
 	// Test parsing an improperly formatted JSON API response.
 	badFees := map[string]uint32{"hi": 12345, "hello": 42, "satoshi": 54321}
@@ -145,10 +126,8 @@ func TestSparseConfFeeSource(t *testing.T) {
 	reader = bytes.NewReader(jsonResp)
 
 	// Finally, ensure the improperly formatted fees error.
-	_, err = feeSource.ParseResponse(reader)
-	if err == nil {
-		t.Fatalf("expected ParseResponse to fail")
-	}
+	_, err = feeSource.parseResponse(reader)
+	require.Error(t, err, "expected error when parsing bad JSON")
 }
 
 // TestWebAPIFeeEstimator checks that the WebAPIFeeEstimator returns fee rates
@@ -216,10 +195,9 @@ func TestWebAPIFeeEstimator(t *testing.T) {
 		maxTarget: minFeeRate,
 	}
 
-	feeSource := mockSparseConfFeeSource{
-		url:  "https://www.github.com",
-		fees: feeRateResp,
-	}
+	// Create a mock fee source and mock its returned map.
+	feeSource := &mockFeeSource{}
+	feeSource.On("GetFeeMap").Return(feeRateResp, nil)
 
 	estimator := NewWebAPIEstimator(feeSource, false)
 
@@ -250,12 +228,15 @@ func TestWebAPIFeeEstimator(t *testing.T) {
 
 			exp := SatPerKVByte(tc.expectedFeeRate).FeePerKWeight()
 			require.Equalf(t, exp, est, "target %v failed, fee "+
-				"map is %v", tc.target, feeSource.fees)
+				"map is %v", tc.target, feeRateResp)
 		})
 	}
 
 	// Stop the estimator when test ends.
 	require.NoError(t, estimator.Stop(), "unable to stop fee estimator")
+
+	// Assert the mocked fee source is called as expected.
+	feeSource.AssertExpectations(t)
 }
 
 // TestGetCachedFee checks that the fee caching logic works as expected.

lnwallet/chainfee/mocks.go

@@ -0,0 +1,17 @@
+package chainfee
+
+import "github.com/stretchr/testify/mock"
+
+type mockFeeSource struct {
+	mock.Mock
+}
+
+// A compile-time assertion to ensure that mockFeeSource implements the
+// WebAPIFeeSource interface.
+var _ WebAPIFeeSource = (*mockFeeSource)(nil)
+
+func (m *mockFeeSource) GetFeeMap() (map[uint32]uint32, error) {
+	args := m.Called()
+
+	return args.Get(0).(map[uint32]uint32), args.Error(1)
+}