Improve unit test of flowexporter package

[heanlan]

Add more test cases for pkg/agent/flowexporter package and
its subpackages: connections, exporter, priorityqueue.

For #4142

Signed-off-by: heanlan hanlan@vmware.com

[codecov]

Codecov Report

Merging #4182 (7b03299) into main (ca6e64c) will increase coverage by 3.39%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4182      +/-   ##
==========================================
+ Coverage   57.25%   60.65%   +3.39%     
==========================================
  Files         380      384       +4     
  Lines       53791    54375     +584     
==========================================
+ Hits        30799    32981    +2182     
+ Misses      20655    18961    -1694     
- Partials     2337     2433      +96     

Flag	Coverage Δ
integration-tests	34.78% <ø> (-0.17%)	⬇️
kind-e2e-tests	48.59% <ø> (+7.16%)	⬆️
unit-tests	42.03% <ø> (+0.99%)	⬆️

Impacted Files	Coverage Δ
pkg/agent/flowexporter/exporter/certificate.go	27.77% <0.00%> (-22.23%)	⬇️
...lowaggregator/clickhouseclient/clickhouseclient.go	73.59% <0.00%> (-9.13%)	⬇️
pkg/flowaggregator/options/options.go	31.25% <0.00%> (-5.49%)	⬇️
...trollers/multicluster/resourceexport_controller.go	75.13% <0.00%> (-3.56%)	⬇️
pkg/controller/ipam/antrea_ipam_controller.go	76.25% <0.00%> (-3.02%)	⬇️
...gent/controller/networkpolicy/status_controller.go	78.99% <0.00%> (-2.53%)	⬇️
pkg/flowaggregator/flowaggregator.go	67.56% <0.00%> (-2.49%)	⬇️
...gent/controller/noderoute/node_route_controller.go	57.54% <0.00%> (-1.87%)	⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go	51.08% <0.00%> (-1.74%)	⬇️
pkg/ipam/ipallocator/allocator.go	88.14% <0.00%> (-1.55%)	⬇️
... and 89 more

[heanlan]

Hi @tnqn , I found the coverage of pkg/agent/flowexporter is already over 78%. I looked into the individual files, didn't find much I can add. Many of the uncovered lines are just error logging. So I only fixed a small error in one of the test, and added one single test case. Do you think it will be ok for now?

[tnqn]

Hi @tnqn , I found the coverage of pkg/agent/flowexporter is already over 78%. I looked into the individual files, didn't find much I can add. Many of the uncovered lines are just error logging. So I only fixed a small error in one of the test, and added one single test case. Do you think it will be ok for now?

@heanlan I listed the package because unit test report show around 50% coverage for several sub-packages of it and I took a quick look at the source code and thought some of them may be error prone without an unit test, e.g.

antrea/pkg/agent/flowexporter/priorityqueue/priorityqueue.go

Lines 113 to 127 in 7a3f6e2

	func (pq *ExpirePriorityQueue) GetExpiryFromExpirePriorityQueue() time.Duration {
	currTime := time.Now()
	if pq.Len() > 0 {
	// Get the minExpireTime of the top item in ExpirePriorityQueue.
	expiryDuration := minExpiryTime + pq.minExpireTime(0).Sub(currTime)
	if expiryDuration < 0 {
	return minExpiryTime
	}
	return expiryDuration
	}
	if pq.ActiveFlowTimeout < pq.IdleFlowTimeout {
	return pq.ActiveFlowTimeout
	}
	return pq.IdleFlowTimeout
	}

As explained in #4142 (comment), I guess the reason why its overall coverage looks fine is because the major code of this package will be executed when the program runs e2e to e2e, but does it mean the code work as expected in various situation? For example, if I remove L107 from the following code, would any test fail? If not, it would be a memory leak but the coverage still looks good.

antrea/pkg/agent/flowexporter/priorityqueue/priorityqueue.go

Lines 100 to 109 in 7a3f6e2

	func (pq *ExpirePriorityQueue) Remove(connKey flowexporter.ConnectionKey) *flowexporter.ItemToExpire {
	item, exists := pq.KeyToItem[connKey]
	if !exists {
	return nil
	}
	removedItem := heap.Remove(pq, item.Index)
	delete(pq.KeyToItem, connKey)
	return removedItem.(*flowexporter.ItemToExpire)
	}

For a tool like ExpirePriorityQueue, I feel it should have a good unit test coverage to cover as many as situations as it can, instead of relying the tool's consumer (the flow aggregator)'s e2e test to ensure its functionality's correctness. However, I'm not familar this package so maybe I'm wrong. If so, feel free to go with what makes most sense to you.

[heanlan]

Thanks @tnqn , I think I got your point. I previously didn't realize the coverage report does not only include UT coverage. I will re-evaluate the coverage in your suggested way.

I listed the package because unit test report show around 50% coverage for several sub-packages

Could you show me where to find the coverage report for exact unit test? In https://app.codecov.io/gh/antrea-io/antrea/tree/main/pkg/agent/flowexporter, I only see the coverage for file flowexporter/exporter/certificate.go is 50%. And flowexporter/connections/conntrack_windows.go is 0%. The others are all above 70%. Not sure if I'm looking at the correct stats.

For a tool like ExpirePriorityQueue, I feel it should have a good unit test coverage to cover as many as situations as it can

For priorityqueue pkg, yes, we don't have UT for GetExpiryFromExpirePriorityQueue, I'll add that. While in https://github.com/antrea-io/antrea/blob/7a3f6e25f4226f5cb93778e7bd5759982478962d/pkg/agent/flowexporter/priorityqueue/priorityqueue_test.go, I think we already cover most of the operations of PQ. For example, if we comment L107 as you suggested, the current test will fail.

[tnqn]

Could you show me where to find the coverage report for exact unit test? In https://app.codecov.io/gh/antrea-io/antrea/tree/main/pkg/agent/flowexporter, I only see the coverage for file flowexporter/exporter/certificate.go is 50%. And flowexporter/connections/conntrack_windows.go is 0%. The others are all above 70%. Not sure if I'm looking at the correct stats.

You could run make test-unit to generate the unit test coverage profile, then convert it to a html file to check:

$ make test-unit
$ go tool cover -html=.coverage/coverage-unit.txt
HTML output written to /tmp/cover3567790458/coverage.html

[dreamtalen]

LGTM

[yuntanghsu]

LGTM

[heanlan]

squashed the commits

[heanlan]

Hi @tnqn , would you like to take a look at the PR? Thanks

[heanlan]

Found I missed to git add a new test file... It includes the unit tests of pkg/agent/flowexporter/utils.go. The code itself is not very error-prone, and I can remove it if the reviewers believe it is unnecessary.

[tnqn]

LGTM, thanks

[tnqn]

/skip-all