CI: set -coverpkg when calling go test in nightly builds

.codecov.yml

@@ -1,10 +1,15 @@
 codecov:
   require_ci_to_pass: yes
 
+flags:
+  full_coverage:
+    joined: false
+
 ignore:
   - "**/*_gen.go"
   - "**/*_gen_test.go"
   - "**/generated"
+  - "**/*_string.go"  # ignore stringer-generated code
 
 coverage:
   status:

.github/workflows/ci-nightly.yml

@@ -103,13 +103,15 @@ jobs:
         run: |
           ./scripts/configure_dev.sh
           PACKAGES="$(go list ./... | grep -v /go-algorand/test/)"
-          export PACKAGE_NAMES=$(echo $PACKAGES | tr -d '\n')          
+          export PACKAGE_NAMES=$(echo $PACKAGES | tr -d '\n')
+          COVERPACKAGES="$(go list ./... | grep -E -v '/go-algorand/(test|debug|cmd|config/defaultsGenerator|tools)' | grep -E -v '(test|testing|mock|mocks)$' | paste -sd ',' -)"
           mkdir -p test_results/${{ matrix.platform }}_test_nightly/${PARTITION_ID}
           go tool -modfile=tool.mod gotestsum --format standard-quiet \
             --junitfile ~/test_results/${{ matrix.platform }}_test_nightly/${PARTITION_ID}/results.xml \
             --jsonfile ~/test_results/${{ matrix.platform }}_test_nightly/${PARTITION_ID}/testresults.json \
             -- --tags "sqlite_unlock_notify sqlite_omit_load_extension" \
             -race -timeout 1h -coverprofile=coverage.txt -covermode=atomic -p 1 \
+            -coverpkg=$COVERPACKAGES \
             $PACKAGE_NAMES
       - name: Notify Slack on failure
         if: failure()
@@ -142,6 +144,7 @@ jobs:
         with:
           token: ${{ env.CODECOV_TOKEN }}
           file: ./coverage.txt
+          flags: full_coverage
           fail_ci_if_error: false
       - name: Upload test results to Codecov
         if: ${{ !cancelled() }}

.github/workflows/ci-pr.yml

@@ -4,6 +4,13 @@ on:
     branches:
       - master
       - 'rel/**'
+  workflow_dispatch:
+    inputs:
+      full_coverage:
+        description: 'Run with -coverpkg to measure cross-package coverage (slower)'
+        required: false
+        type: boolean
+        default: false
 
 env:
   CODECOV_TOKEN: "8b4a1f91-f154-4c26-b84c-c9aaa90159c6"  # Same public token from CircleCI config
@@ -49,13 +56,19 @@ jobs:
       - name: Run tests
         run: |
           PACKAGES="$(go list ./... | grep -v /go-algorand/test/)"
-          export PACKAGE_NAMES=$(echo $PACKAGES | tr -d '\n')          
+          export PACKAGE_NAMES=$(echo $PACKAGES | tr -d '\n')
+          COVERPKG_FLAG=""
+          if [[ "${{ inputs.full_coverage }}" == "true" ]]; then
+            COVERPACKAGES="$(go list ./... | grep -E -v '/go-algorand/(test|debug|cmd|config/defaultsGenerator|tools)' | grep -E -v '(test|testing|mock|mocks)$' | paste -sd ',' -)"
+            COVERPKG_FLAG="-coverpkg=$COVERPACKAGES"
+          fi
           mkdir -p test_results/${{ matrix.platform }}_test/${PARTITION_ID}
           go tool -modfile=tool.mod gotestsum --format standard-quiet \
             --junitfile ~/test_results/${{ matrix.platform }}_test/${PARTITION_ID}/results.xml \
             --jsonfile ~/test_results/${{ matrix.platform }}_test/${PARTITION_ID}/testresults.json \
             -- --tags "sqlite_unlock_notify sqlite_omit_load_extension" $SHORTTEST \
             -race -timeout 1h -coverprofile=coverage.txt -covermode=atomic -p 4 \
+            $COVERPKG_FLAG \
             $PACKAGE_NAMES
       - name: Notify Slack on failure
         if: failure()
@@ -89,6 +102,7 @@ jobs:
         with:
           token: ${{ env.CODECOV_TOKEN }}
           file: ./coverage.txt
+          flags: ${{ inputs.full_coverage && 'full_coverage' || '' }}
           fail_ci_if_error: false
       - name: Upload test results to Codecov
         if: ${{ !cancelled() }}

Makefile

@@ -84,6 +84,7 @@ GOLDFLAGS := $(GOLDFLAGS_BASE) \
 		 -X github.com/algorand/go-algorand/config.Channel=$(CHANNEL)
 
 UNIT_TEST_SOURCES := $(sort $(shell GOPATH=$(GOPATH) && GO111MODULE=off && go list ./... | grep -v /go-algorand/test/ ))
+COVERPKG_PACKAGES := $(sort $(shell GOPATH=$(GOPATH) && GO111MODULE=off && go list ./... | egrep -v '/go-algorand/(test|debug|cmd|config/defaultsGenerator|tools)' | egrep -v '(test|testing|mocks|mock)$$' ))
 ALGOD_API_PACKAGES := $(sort $(shell GOPATH=$(GOPATH) && GO111MODULE=off && cd daemon/algod/api; go list ./... ))
 
 GOMOD_DIRS := ./tools/block-generator ./tools/x-repo-types
@@ -132,8 +133,19 @@ check_shell:
 
 sanity: fix lint fmt tidy modernize
 
+# "make cover" runs all tests, and collects full coverage across all go-algorand packages by setting -coverpkg.
+# Without setting -coverpkg, coverage reports only measure lines of code exercised within the same package as the tests.
+#
+# "make cover PACKAGE=X" runs all tests in package github.com/algorand/go-algorand/X/... and collects full coverage
+# across all packages that are dependencies of that package.
 cover:
-	go test $(GOTAGS) -coverprofile=cover.out $(UNIT_TEST_SOURCES)
+ifeq ($(PACKAGE),)
+	$(GOTESTCOMMAND) $(GOTAGS) -coverprofile=cover.out $(UNIT_TEST_SOURCES) -covermode=atomic -coverpkg=$(shell echo $(COVERPKG_PACKAGES) | sed 's/ /,/g')
+else
+	cd $(PACKAGE); \
+	$(GOTESTCOMMAND) $(GOTAGS) -coverprofile=cover.out ./... -covermode=atomic -coverpkg=$$( (go list -f '{{ join .Deps "\n" }}' ./...; go list -f '{{ join .TestImports "\n" }}' ./...) | grep 'github.com/algorand/go-algorand' | egrep -v '/go-algorand/(test|debug|cmd|config/defaultsGenerator|tools)' | egrep -v '(test|testing|mocks|mock)$$' | sort | uniq | paste -sd ',' -); \
+	go tool cover -html cover.out
+endif
 
 prof:
 	cd node && go test $(GOTAGS) -cpuprofile=cpu.out -memprofile=mem.out -mutexprofile=mutex.out

ledger/metrics.go

@@ -31,31 +31,38 @@ type metricsTracker struct {
 	ledgerRewardClaimsTotal *metrics.Counter
 	ledgerRound             *metrics.Gauge
 	ledgerDBRound           *metrics.Gauge
+	registry                *metrics.Registry
 }
 
 func (mt *metricsTracker) loadFromDisk(l ledgerForTracker, _ basics.Round) error {
-	mt.ledgerTransactionsTotal = metrics.MakeCounter(metrics.LedgerTransactionsTotal)
-	mt.ledgerRewardClaimsTotal = metrics.MakeCounter(metrics.LedgerRewardClaimsTotal)
-	mt.ledgerRound = metrics.MakeGauge(metrics.LedgerRound)
-	mt.ledgerDBRound = metrics.MakeGauge(metrics.LedgerDBRound)
+	reg := mt.registry
+	mt.ledgerTransactionsTotal = metrics.MakeCounterUnregistered(metrics.LedgerTransactionsTotal)
+	mt.ledgerTransactionsTotal.Register(reg)
+	mt.ledgerRewardClaimsTotal = metrics.MakeCounterUnregistered(metrics.LedgerRewardClaimsTotal)
+	mt.ledgerRewardClaimsTotal.Register(reg)
+	mt.ledgerRound = metrics.MakeGaugeUnregistered(metrics.LedgerRound)
+	mt.ledgerRound.Register(reg)
+	mt.ledgerDBRound = metrics.MakeGaugeUnregistered(metrics.LedgerDBRound)
+	mt.ledgerDBRound.Register(reg)
 	return nil
 }
 
 func (mt *metricsTracker) close() {
+	reg := mt.registry
 	if mt.ledgerTransactionsTotal != nil {
-		mt.ledgerTransactionsTotal.Deregister(nil)
+		mt.ledgerTransactionsTotal.Deregister(reg)
 		mt.ledgerTransactionsTotal = nil
 	}
 	if mt.ledgerRewardClaimsTotal != nil {
-		mt.ledgerRewardClaimsTotal.Deregister(nil)
+		mt.ledgerRewardClaimsTotal.Deregister(reg)
 		mt.ledgerRewardClaimsTotal = nil
 	}
 	if mt.ledgerRound != nil {
-		mt.ledgerRound.Deregister(nil)
+		mt.ledgerRound.Deregister(reg)
 		mt.ledgerRound = nil
 	}
 	if mt.ledgerDBRound != nil {
-		mt.ledgerDBRound.Deregister(nil)
+		mt.ledgerDBRound.Deregister(reg)
 		mt.ledgerDBRound = nil
 	}
 }

ledger/metrics_test.go

@@ -34,7 +34,11 @@ import (
 func TestMetricsReload(t *testing.T) {
 	partitiontest.PartitionTest(t)
 
-	mt := metricsTracker{}
+	// Use a non-default registry so the test is not affected by metrics
+	// registered by other packages when running with -coverpkg.
+	registry := metrics.MakeRegistry()
+
+	mt := metricsTracker{registry: registry}
 	accts := ledgertesting.RandomAccounts(1, true)
 	ml := makeMockLedgerForTracker(t, true, 1, protocol.ConsensusCurrentVersion, []map[basics.Address]basics.AccountData{accts})
 
@@ -48,7 +52,7 @@ func TestMetricsReload(t *testing.T) {
 	mt.newBlock(blk, ledgercore.StateDelta{})
 
 	var buf strings.Builder
-	metrics.DefaultRegistry().WriteMetrics(&buf, "")
+	registry.WriteMetrics(&buf, "")
 	lines := strings.Split(buf.String(), "\n")
 	txCount := 0
 	rcCount := 0

scripts/travis/upload_coverage.sh

@@ -3,7 +3,7 @@
 set -eo pipefail
 
 if [[ -z "$CODECOV_TOKEN" ]]; then
-  /usr/bin/env bash scripts/travis/codecov
+  /usr/bin/env bash scripts/travis/codecov "$@"
 else
-  /usr/bin/env bash scripts/travis/codecov -t $CODECOV_TOKEN
+  /usr/bin/env bash scripts/travis/codecov -t $CODECOV_TOKEN "$@"
 fi

util/metrics/counter.go

@@ -28,14 +28,14 @@ type Counter struct {
 
 // MakeCounter create a new counter with the provided name and description.
 func MakeCounter(metric MetricName) *Counter {
-	c := makeCounter(metric)
+	c := MakeCounterUnregistered(metric)
 	c.Register(nil)
 	return c
 }
 
-// makeCounter create a new counter with the provided name and description
+// MakeCounterUnregistered creates a new counter with the provided name and description
 // but does not register it with the default registry.
-func makeCounter(metric MetricName) *Counter {
+func MakeCounterUnregistered(metric MetricName) *Counter {
 	c := &Counter{c: couge{
 		values:        make([]*cougeValues, 0),
 		description:   metric.Description,

util/metrics/counter_test.go

@@ -38,6 +38,9 @@ func TestMetricCounter(t *testing.T) {
 		MetricTest: NewMetricTest(),
 	}
 
+	// create a non-default registry for the metrics in this test
+	registry := MakeRegistry()
+
 	// create a http listener.
 	port := test.createListener("127.0.0.1:0")
 
@@ -46,10 +49,12 @@ func TestMetricCounter(t *testing.T) {
 		Labels: map[string]string{
 			"host_name":  "host_one",
 			"session_id": "AFX-229"},
+		registry: registry,
 	})
 	metricService.Start(context.Background())
 
-	counter := MakeCounter(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"})
+	counter := MakeCounterUnregistered(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"})
+	counter.Register(registry)
 
 	for i := 0; i < 20; i++ {
 		counter.Inc(map[string]string{"pid": "123", "data_host": fmt.Sprintf("host%d", i%5)})
@@ -61,7 +66,7 @@ func TestMetricCounter(t *testing.T) {
 
 	metricService.Shutdown()
 
-	counter.Deregister(nil)
+	counter.Deregister(registry)
 	// test the metrics values.
 
 	test.Lock()
@@ -84,6 +89,9 @@ func TestMetricCounterFastInts(t *testing.T) {
 		MetricTest: NewMetricTest(),
 	}
 
+	// create a non-default registry for the metrics in this test
+	registry := MakeRegistry()
+
 	// create a http listener.
 	port := test.createListener("127.0.0.1:0")
 
@@ -92,10 +100,12 @@ func TestMetricCounterFastInts(t *testing.T) {
 		Labels: map[string]string{
 			"host_name":  "host_one",
 			"session_id": "AFX-229"},
+		registry: registry,
 	})
 	metricService.Start(context.Background())
 
-	counter := MakeCounter(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"})
+	counter := MakeCounterUnregistered(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"})
+	counter.Register(registry)
 
 	for i := 0; i < 20; i++ {
 		counter.Inc(nil)
@@ -108,7 +118,7 @@ func TestMetricCounterFastInts(t *testing.T) {
 
 	metricService.Shutdown()
 
-	counter.Deregister(nil)
+	counter.Deregister(registry)
 	// test the metrics values.
 
 	test.Lock()
@@ -131,6 +141,9 @@ func TestMetricCounterMixed(t *testing.T) {
 		MetricTest: NewMetricTest(),
 	}
 
+	// create a non-default registry for the metrics in this test
+	registry := MakeRegistry()
+
 	// create a http listener.
 	port := test.createListener("127.0.0.1:0")
 
@@ -139,10 +152,12 @@ func TestMetricCounterMixed(t *testing.T) {
 		Labels: map[string]string{
 			"host_name":  "host_one",
 			"session_id": "AFX-229"},
+		registry: registry,
 	})
 	metricService.Start(context.Background())
 
-	counter := MakeCounter(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"})
+	counter := MakeCounterUnregistered(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"})
+	counter.Register(registry)
 
 	counter.AddUint64(5, nil)
 	counter.AddUint64(8, map[string]string{})
@@ -157,7 +172,7 @@ func TestMetricCounterMixed(t *testing.T) {
 
 	metricService.Shutdown()
 
-	counter.Deregister(nil)
+	counter.Deregister(registry)
 	// test the metrics values.
 
 	test.Lock()

util/metrics/gauge.go

@@ -27,14 +27,14 @@ type Gauge struct {
 
 // MakeGauge create a new gauge with the provided name and description.
 func MakeGauge(metric MetricName) *Gauge {
-	c := makeGauge(metric)
+	c := MakeGaugeUnregistered(metric)
 	c.Register(nil)
 	return c
 }
 
-// makeGauge create a new gauge with the provided name and description
+// MakeGaugeUnregistered creates a new gauge with the provided name and description
 // but does not register it with the default registry.
-func makeGauge(metric MetricName) *Gauge {
+func MakeGaugeUnregistered(metric MetricName) *Gauge {
 	c := &Gauge{g: couge{
 		values:        make([]*cougeValues, 0),
 		description:   metric.Description,

util/metrics/gauge_test.go

@@ -37,6 +37,10 @@ func TestMetricGauge(t *testing.T) {
 	test := &GaugeTest{
 		MetricTest: NewMetricTest(),
 	}
+
+	// create a non-default registry for the metrics in this test
+	registry := MakeRegistry()
+
 	// create a http listener.
 	port := test.createListener("127.0.0.1:0")
 
@@ -45,11 +49,13 @@ func TestMetricGauge(t *testing.T) {
 		Labels: map[string]string{
 			"host_name":  "host_one",
 			"session_id": "AFX-229"},
+		registry: registry,
 	})
 	metricService.Start(context.Background())
 	gauges := make([]*Gauge, 3)
 	for i := 0; i < 3; i++ {
-		gauges[i] = MakeGauge(MetricName{Name: fmt.Sprintf("gauge_%d", i), Description: "this is the metric test for gauge object"})
+		gauges[i] = MakeGaugeUnregistered(MetricName{Name: fmt.Sprintf("gauge_%d", i), Description: "this is the metric test for gauge object"})
+		gauges[i].Register(registry)
 	}
 	for i := 0; i < 9; i++ {
 		gauges[i%3].Set(uint64(i*100 + i))
@@ -62,7 +68,7 @@ func TestMetricGauge(t *testing.T) {
 
 	metricService.Shutdown()
 	for _, gauge := range gauges {
-		gauge.Deregister(nil)
+		gauge.Deregister(registry)
 	}
 	// test the metrics values.
 

util/metrics/opencensus.go

@@ -78,7 +78,7 @@ type statCounter struct {
 // WriteMetric outputs Prometheus metrics for all labels/values in statCounter
 func (st *statCounter) WriteMetric(buf *strings.Builder, parentLabels string) {
 	name := sanitizePrometheusName(st.name)
-	counter := makeCounter(MetricName{name, st.description})
+	counter := MakeCounterUnregistered(MetricName{name, st.description})
 	for i := 0; i < len(st.labels); i++ {
 		counter.AddUint64(uint64(st.values[i]), st.labels[i])
 	}
@@ -87,7 +87,7 @@ func (st *statCounter) WriteMetric(buf *strings.Builder, parentLabels string) {
 
 // AddMetric outputs all statCounter's labels/values into a map
 func (st *statCounter) AddMetric(values map[string]float64) {
-	counter := makeCounter(MetricName{st.name, st.description})
+	counter := MakeCounterUnregistered(MetricName{st.name, st.description})
 	for i := 0; i < len(st.labels); i++ {
 		counter.AddUint64(uint64(st.values[i]), st.labels[i])
 	}
@@ -105,7 +105,7 @@ type statDistribution struct {
 // WriteMetric outputs Prometheus metrics for all labels/values in statCounter
 func (st *statDistribution) WriteMetric(buf *strings.Builder, parentLabels string) {
 	name := sanitizePrometheusName(st.name)
-	gauge := makeGauge(MetricName{name, st.description})
+	gauge := MakeGaugeUnregistered(MetricName{name, st.description})
 	for i := 0; i < len(st.labels); i++ {
 		gauge.SetLabels(uint64(st.values[i]), st.labels[i])
 	}
@@ -114,7 +114,7 @@ func (st *statDistribution) WriteMetric(buf *strings.Builder, parentLabels strin
 
 // AddMetric outputs all statCounter's labels/values into a map
 func (st *statDistribution) AddMetric(values map[string]float64) {
-	gauge := makeGauge(MetricName{st.name, st.description})
+	gauge := MakeGaugeUnregistered(MetricName{st.name, st.description})
 	for i := 0; i < len(st.labels); i++ {
 		gauge.SetLabels(uint64(st.values[i]), st.labels[i])
 	}