[FAB-4322] Improve UT coverage of orderer/multichain

This patch also changes tests to use `assert`, as well as
some minor tweaks of comments and log message.

Change-Id: I681731f16f0ab8e7465654891043881ccc47fc6d
Signed-off-by: Jay Guo <[email protected]>

Author: guoger

orderer/multichain/chainsupport.go

@@ -34,7 +34,7 @@ import (
 
 // Consenter defines the backing ordering mechanism
 type Consenter interface {
-	// HandleChain should create a return a reference to a Chain for the given set of resources
+	// HandleChain should create and return a reference to a Chain for the given set of resources
 	// It will only be invoked for a given chain once per process.  In general, errors will be treated
 	// as irrecoverable and cause system shutdown.  See the description of Chain for more details
 	// The second argument to HandleChain is a pointer to the metadata stored on the `ORDERER` slot of

orderer/multichain/chainsupport_test.go

@@ -17,16 +17,17 @@ limitations under the License.
 package multichain
 
 import (
-	"reflect"
 	"testing"
 
 	"github.com/golang/protobuf/proto"
 	mockconfigtx "github.com/hyperledger/fabric/common/mocks/configtx"
+	"github.com/hyperledger/fabric/common/mocks/crypto"
 	"github.com/hyperledger/fabric/orderer/common/filter"
 	"github.com/hyperledger/fabric/orderer/ledger"
 	cb "github.com/hyperledger/fabric/protos/common"
 	ab "github.com/hyperledger/fabric/protos/orderer"
 	"github.com/hyperledger/fabric/protos/utils"
+	"github.com/stretchr/testify/assert"
 )
 
 type mockLedgerReadWriter struct {
@@ -66,24 +67,23 @@ func TestCommitConfig(t *testing.T) {
 	ml := &mockLedgerReadWriter{}
 	cm := &mockconfigtx.Manager{}
 	cs := &chainSupport{ledgerResources: &ledgerResources{configResources: &configResources{Manager: cm}, ledger: ml}, signer: mockCrypto()}
+	assert.Equal(t, uint64(0), cs.Height(), "Should has height of 0")
+
 	txs := []*cb.Envelope{makeNormalTx("foo", 0), makeNormalTx("bar", 1)}
 	committers := []filter.Committer{&mockCommitter{}, &mockCommitter{}}
 	block := cs.CreateNextBlock(txs)
 	cs.WriteBlock(block, committers, nil)
+	assert.Equal(t, uint64(1), cs.Height(), "Should has height of 1")
 
 	blockTXs := make([]*cb.Envelope, len(ml.data))
 	for i := range ml.data {
 		blockTXs[i] = utils.UnmarshalEnvelopeOrPanic(ml.data[i])
 	}
 
-	if !reflect.DeepEqual(blockTXs, txs) {
-		t.Errorf("Should have written input data to ledger but did not")
-	}
+	assert.Equal(t, txs, blockTXs, "Should have written input data to ledger but did not")
 
 	for _, c := range committers {
-		if c.(*mockCommitter).committed != 1 {
-			t.Errorf("Expected exactly 1 commits but got %d", c.(*mockCommitter).committed)
-		}
+		assert.EqualValues(t, 1, c.(*mockCommitter).committed, "Expected exactly 1 commits but got %d", c.(*mockCommitter).committed)
 	}
 }
 
@@ -92,9 +92,8 @@ func TestWriteBlockSignatures(t *testing.T) {
 	cm := &mockconfigtx.Manager{}
 	cs := &chainSupport{ledgerResources: &ledgerResources{configResources: &configResources{Manager: cm}, ledger: ml}, signer: mockCrypto()}
 
-	if utils.GetMetadataFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(0, nil), nil, nil), cb.BlockMetadataIndex_SIGNATURES) == nil {
-		t.Fatalf("Block should have block signature")
-	}
+	actual := utils.GetMetadataFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(0, nil), nil, nil), cb.BlockMetadataIndex_SIGNATURES)
+	assert.NotNil(t, actual, "Block should have block signature")
 }
 
 func TestWriteBlockOrdererMetadata(t *testing.T) {
@@ -105,14 +104,22 @@ func TestWriteBlockOrdererMetadata(t *testing.T) {
 	value := []byte("foo")
 	expected := &cb.Metadata{Value: value}
 	actual := utils.GetMetadataFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(0, nil), nil, value), cb.BlockMetadataIndex_ORDERER)
+	assert.NotNil(t, actual, "Block should have orderer metadata written")
+	assert.True(t, proto.Equal(expected, actual), "Orderer metadata not written to block correctly")
+}
 
-	if actual == nil {
-		t.Fatalf("Block should have orderer metadata written")
-	}
-	if !proto.Equal(expected, actual) {
-		t.Fatalf("Orderer metadata not written to block correctly")
-	}
+func TestSignature(t *testing.T) {
+	ml := &mockLedgerReadWriter{}
+	cm := &mockconfigtx.Manager{}
+	cs := &chainSupport{ledgerResources: &ledgerResources{configResources: &configResources{Manager: cm}, ledger: ml}, signer: mockCrypto()}
+
+	message := []byte("Darth Vader")
+	signed, _ := cs.Sign(message)
+	assert.Equal(t, message, signed, "Should sign the message")
 
+	signatureHeader, _ := cs.NewSignatureHeader()
+	assert.Equal(t, crypto.FakeLocalSigner.Identity, signatureHeader.Creator)
+	assert.Equal(t, crypto.FakeLocalSigner.Nonce, signatureHeader.Nonce)
 }
 
 func TestWriteLastConfig(t *testing.T) {
@@ -121,39 +128,28 @@ func TestWriteLastConfig(t *testing.T) {
 	cs := &chainSupport{ledgerResources: &ledgerResources{configResources: &configResources{Manager: cm}, ledger: ml}, signer: mockCrypto()}
 
 	expected := uint64(0)
-
-	if lc := utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(0, nil), nil, nil)); lc != expected {
-		t.Fatalf("First block should have config block index of %d, but got %d", expected, lc)
-	}
-	if lc := utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(1, nil), nil, nil)); lc != expected {
-		t.Fatalf("Second block should have config block index of %d, but got %d", expected, lc)
-	}
+	lc := utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(0, nil), nil, nil))
+	assert.Equal(t, expected, lc, "First block should have config block index of %d, but got %d", expected, lc)
+	lc = utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(1, nil), nil, nil))
+	assert.Equal(t, expected, lc, "Second block should have config block index of %d, but got %d", expected, lc)
 
 	cm.SequenceVal = 1
 	expected = uint64(2)
-	if lc := utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(2, nil), nil, nil)); lc != expected {
-		t.Fatalf("Second block should have config block index of %d, but got %d", expected, lc)
-	}
+	lc = utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(2, nil), nil, nil))
+	assert.Equal(t, expected, lc, "Second block should have config block index of %d, but got %d", expected, lc)
 
-	if lc := utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(3, nil), nil, nil)); lc != expected {
-		t.Fatalf("Second block should have config block index of %d, but got %d", expected, lc)
-	}
+	lc = utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(3, nil), nil, nil))
+	assert.Equal(t, expected, lc, "Second block should have config block index of %d, but got %d", expected, lc)
 
 	t.Run("ResetChainSupport", func(t *testing.T) {
-
 		cm.SequenceVal = 2
 		expected = uint64(4)
 
 		cs = &chainSupport{ledgerResources: &ledgerResources{configResources: &configResources{Manager: cm}, ledger: ml}, signer: mockCrypto()}
+		lc := utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(4, nil), nil, nil))
+		assert.Equal(t, expected, lc, "Second block should have config block index of %d, but got %d", expected, lc)
 
-		if lc := utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(4, nil), nil, nil)); lc != expected {
-			t.Fatalf("Second block should have config block index of %d, but got %d", expected, lc)
-		}
-
-		if lc := utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(5, nil), nil, nil)); lc != expected {
-			t.Fatalf("Second block should have config block index of %d, but got %d", expected, lc)
-		}
-
+		lc = utils.GetLastConfigIndexFromBlockOrPanic(cs.WriteBlock(cb.NewBlock(5, nil), nil, nil))
+		assert.Equal(t, expected, lc, "Second block should have config block index of %d, but got %d")
 	})
-
 }

orderer/multichain/manager.go

@@ -109,7 +109,7 @@ func NewManagerImpl(ledgerFactory ledger.Factory, consenters map[string]Consente
 		}
 		configTx := getConfigTx(rl)
 		if configTx == nil {
-			logger.Panicf("Could not find config transaction for chain %s", chainID)
+			logger.Panic("Programming error, configTx should never be nil here")
 		}
 		ledgerResources := ml.newLedgerResources(configTx)
 		chainID := ledgerResources.ChainID()
@@ -123,7 +123,7 @@ func NewManagerImpl(ledgerFactory ledger.Factory, consenters map[string]Consente
 				consenters,
 				signer)
 			logger.Infof("Starting with system channel %s and orderer type %s", chainID, chain.SharedConfig().ConsensusType())
-			ml.chains[string(chainID)] = chain
+			ml.chains[chainID] = chain
 			ml.systemChannelID = chainID
 			ml.systemChannel = chain
 			// We delay starting this chain, as it might try to copy and replace the chains map via newChain before the map is fully built
@@ -134,7 +134,7 @@ func NewManagerImpl(ledgerFactory ledger.Factory, consenters map[string]Consente
 				ledgerResources,
 				consenters,
 				signer)
-			ml.chains[string(chainID)] = chain
+			ml.chains[chainID] = chain
 			chain.start()
 		}
 

orderer/multichain/manager_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package multichain
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 	"time"
@@ -39,19 +40,24 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-var conf, singleMSPConf *genesisconfig.Profile
-var genesisBlock, singleMSPGenesisBlock *cb.Block
+var conf, singleMSPConf, noConsortiumConf *genesisconfig.Profile
+var genesisBlock, singleMSPGenesisBlock, noConsortiumGenesisBlock *cb.Block
 var mockSigningIdentity msp.SigningIdentity
 
+const NoConsortiumChain = "NoConsortiumChain"
+
 func init() {
-	conf = genesisconfig.Load(genesisconfig.SampleInsecureProfile)
 	logging.SetLevel(logging.DEBUG, "")
-	genesisBlock = provisional.New(conf).GenesisBlock()
 	mockSigningIdentity, _ = mmsp.NewNoopMsp().GetDefaultSigningIdentity()
 
+	conf = genesisconfig.Load(genesisconfig.SampleInsecureProfile)
+	genesisBlock = provisional.New(conf).GenesisBlock()
+
 	singleMSPConf = genesisconfig.Load(genesisconfig.SampleSingleMSPSoloProfile)
-	logging.SetLevel(logging.DEBUG, "")
 	singleMSPGenesisBlock = provisional.New(singleMSPConf).GenesisBlock()
+
+	noConsortiumConf = genesisconfig.Load("SampleNoConsortium")
+	noConsortiumGenesisBlock = provisional.New(noConsortiumConf).GenesisBlockForChannel(NoConsortiumChain)
 }
 
 func mockCrypto() *mockCryptoHelper {
@@ -113,10 +119,7 @@ func TestGetConfigTx(t *testing.T) {
 	rl.Append(block)
 
 	pctx := getConfigTx(rl)
-
-	if !reflect.DeepEqual(ctx, pctx) {
-		t.Fatalf("Did not select most recent config transaction")
-	}
+	assert.Equal(t, pctx, ctx, "Did not select most recent config transaction")
 }
 
 // Tests a chain which contains blocks with multi-transactions mixed with config txs, and a single tx which is not a config tx, none count as config blocks so nil should return
@@ -129,29 +132,21 @@ func TestGetConfigTxFailure(t *testing.T) {
 		}))
 	}
 	rl.Append(ledger.CreateNextBlock(rl, []*cb.Envelope{makeNormalTx(provisional.TestChainID, 11)}))
-	defer func() {
-		if recover() == nil {
-			t.Fatalf("Should have panic-ed because there was no config tx")
-		}
-	}()
-	getConfigTx(rl)
+	assert.Panics(t, func() { getConfigTx(rl) }, "Should have panicked because there was no config tx")
 
+	block := ledger.CreateNextBlock(rl, []*cb.Envelope{makeNormalTx(provisional.TestChainID, 12)})
+	block.Metadata.Metadata[cb.BlockMetadataIndex_LAST_CONFIG] = []byte("bad metadata")
+	assert.Panics(t, func() { getConfigTx(rl) }, "Should have panicked because of bad last config metadata")
 }
 
 // This test checks to make sure the orderer refuses to come up if it cannot find a system channel
 func TestNoSystemChain(t *testing.T) {
-	defer func() {
-		if recover() == nil {
-			t.Fatalf("Should have panicked when starting without a system chain")
-		}
-	}()
-
 	lf := ramledger.New(10)
 
 	consenters := make(map[string]Consenter)
 	consenters[conf.Orderer.OrdererType] = &mockConsenter{}
 
-	NewManagerImpl(lf, consenters, mockCrypto())
+	assert.Panics(t, func() { NewManagerImpl(lf, consenters, mockCrypto()) }, "Should have panicked when starting without a system chain")
 }
 
 // This test checks to make sure that the orderer refuses to come up if there are multiple system channels
@@ -172,6 +167,74 @@ func TestMultiSystemChannel(t *testing.T) {
 	assert.Panics(t, func() { NewManagerImpl(lf, consenters, mockCrypto()) }, "Two system channels should have caused panic")
 }
 
+// This test checks to make sure that the orderer creates different type of filters given different type of channel
+func TestFilterCreation(t *testing.T) {
+	lf := ramledger.New(10)
+	rl, err := lf.GetOrCreate(provisional.TestChainID)
+	if err != nil {
+		panic(err)
+	}
+	err = rl.Append(genesisBlock)
+	if err != nil {
+		panic(err)
+	}
+
+	// Creating a non-system chain to test that NewManagerImpl could handle the diversity
+	rl, err = lf.GetOrCreate(NoConsortiumChain)
+	if err != nil {
+		panic(err)
+	}
+	err = rl.Append(noConsortiumGenesisBlock)
+	if err != nil {
+		panic(err)
+	}
+
+	consenters := make(map[string]Consenter)
+	consenters[conf.Orderer.OrdererType] = &mockConsenter{}
+
+	manager := NewManagerImpl(lf, consenters, mockCrypto())
+
+	_, ok := manager.GetChain(provisional.TestChainID)
+	assert.True(t, ok, "Should have found chain: %d", provisional.TestChainID)
+
+	chainSupport, ok := manager.GetChain(NoConsortiumChain)
+	assert.True(t, ok, "Should have retrieved chain: %d", NoConsortiumChain)
+
+	messages := make([]*cb.Envelope, conf.Orderer.BatchSize.MaxMessageCount)
+	for i := 0; i < int(conf.Orderer.BatchSize.MaxMessageCount); i++ {
+		messages[i] = &cb.Envelope{
+			Payload: utils.MarshalOrPanic(&cb.Payload{
+				Header: &cb.Header{
+					ChannelHeader: utils.MarshalOrPanic(&cb.ChannelHeader{
+						// For testing purpose, we are injecting configTx into non-system channel.
+						// Set Type to HeaderType_ORDERER_TRANSACTION to verify this message is NOT
+						// filtered by SystemChainFilter, so we know we are creating correct type
+						// of filter for the chain.
+						Type:      int32(cb.HeaderType_ORDERER_TRANSACTION),
+						ChannelId: NoConsortiumChain,
+					}),
+					SignatureHeader: utils.MarshalOrPanic(&cb.SignatureHeader{}),
+				},
+				Data: []byte(fmt.Sprintf("%d", i)),
+			}),
+		}
+
+		assert.True(t, chainSupport.Enqueue(messages[i]), "Should have successfully enqueued message")
+	}
+
+	it, _ := rl.Iterator(&ab.SeekPosition{Type: &ab.SeekPosition_Specified{Specified: &ab.SeekSpecified{Number: 1}}})
+	select {
+	case <-it.ReadyChan():
+		block, status := it.Next()
+		assert.Equal(t, cb.Status_SUCCESS, status, "Could not retrieve block")
+		for i := 0; i < int(conf.Orderer.BatchSize.MaxMessageCount); i++ {
+			assert.Equal(t, messages[i], utils.ExtractEnvelopeOrPanic(block, i), "Block contents wrong at index %d", i)
+		}
+	case <-time.After(time.Second):
+		t.Fatalf("Block 1 not produced after timeout")
+	}
+}
+
 // This test essentially brings the entire system up and is ultimately what main.go will replicate
 func TestManagerImpl(t *testing.T) {
 	lf, rl := NewRAMLedgerAndFactory(10)
@@ -182,15 +245,10 @@ func TestManagerImpl(t *testing.T) {
 	manager := NewManagerImpl(lf, consenters, mockCrypto())
 
 	_, ok := manager.GetChain("Fake")
-	if ok {
-		t.Errorf("Should not have found a chain that was not created")
-	}
+	assert.False(t, ok, "Should not have found a chain that was not created")
 
 	chainSupport, ok := manager.GetChain(provisional.TestChainID)
-
-	if !ok {
-		t.Fatalf("Should have gotten chain which was initialized by ramledger")
-	}
+	assert.True(t, ok, "Should have gotten chain which was initialized by ramledger")
 
 	messages := make([]*cb.Envelope, conf.Orderer.BatchSize.MaxMessageCount)
 	for i := 0; i < int(conf.Orderer.BatchSize.MaxMessageCount); i++ {
@@ -205,21 +263,16 @@ func TestManagerImpl(t *testing.T) {
 	select {
 	case <-it.ReadyChan():
 		block, status := it.Next()
-		if status != cb.Status_SUCCESS {
-			t.Fatalf("Could not retrieve block")
-		}
+		assert.Equal(t, cb.Status_SUCCESS, status, "Could not retrieve block")
 		for i := 0; i < int(conf.Orderer.BatchSize.MaxMessageCount); i++ {
-			if !reflect.DeepEqual(utils.ExtractEnvelopeOrPanic(block, i), messages[i]) {
-				t.Errorf("Block contents wrong at index %d", i)
-			}
+			assert.Equal(t, messages[i], utils.ExtractEnvelopeOrPanic(block, i), "Block contents wrong at index %d", i)
 		}
 	case <-time.After(time.Second):
 		t.Fatalf("Block 1 not produced after timeout")
 	}
 }
 
 func TestNewChannelConfig(t *testing.T) {
-
 	lf, _ := NewRAMLedgerAndFactoryWithMSP()
 
 	consenters := make(map[string]Consenter)
@@ -228,7 +281,7 @@ func TestNewChannelConfig(t *testing.T) {
 
 	t.Run("BadPayload", func(t *testing.T) {
 		_, err := manager.NewChannelConfig(&cb.Envelope{Payload: []byte("bad payload")})
-		assert.Error(t, err, "Should bot be able to create new channel config from bad payload.")
+		assert.Error(t, err, "Should not be able to create new channel config from bad payload.")
 	})
 
 	for _, tc := range []struct {

orderer/multichain/systemchain_test.go

@@ -95,9 +95,7 @@ func TestGoodProposal(t *testing.T) {
 		),
 		configtx.NewChainCreationTemplate("SampleConsortium", []string{}),
 	).Envelope(newChainID)
-	if err != nil {
-		t.Fatalf("Error constructing configtx")
-	}
+	assert.Nil(t, err, "Error constructing configtx")
 
 	ingressTx := makeConfigTxFromConfigUpdateEnvelope(newChainID, configEnv)
 	wrapped := wrapConfigTx(ingressTx)