Add a test for extract_aggregate #106

ronensc · 2022-02-27T13:45:48Z

This is a follow up to #95 as part of #61

codecov-commenter · 2022-02-27T14:02:22Z

Codecov Report

Merging #106 (7d74fce) into main (59852cf) will increase coverage by 1.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
+ Coverage   54.80%   56.05%   +1.25%     
==========================================
  Files          37       45       +8     
  Lines        2332     2501     +169     
==========================================
+ Hits         1278     1402     +124     
- Misses        981     1016      +35     
- Partials       73       83      +10

Flag	Coverage Δ
unittests	`56.05% <ø> (+1.25%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/pipeline/extract/extract.go	`100.00% <0.00%> (ø)`
pkg/api/encode_prom.go	`100.00% <0.00%> (ø)`
cmd/confgenerator/main.go	`0.00% <0.00%> (ø)`
pkg/pipeline/extract/extract_aggregate.go	`56.25% <0.00%> (ø)`
pkg/api/transform_network.go	`100.00% <0.00%> (ø)`
pkg/api/enum.go	`68.00% <0.00%> (ø)`
pkg/api/write_loki.go	`55.55% <0.00%> (ø)`
pkg/api/encode_kafka.go	`100.00% <0.00%> (ø)`
pkg/pipeline/write/write_loki.go	`67.23% <0.00%> (+4.51%)`	⬆️
pkg/pipeline/pipeline.go	`74.35% <0.00%> (+10.25%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59852cf...7d74fce. Read the comment docs.

eranra · 2022-02-27T14:54:40Z

pkg/pipeline/extract/extract_aggregate_test.go

+			"count":           "2",
+		},
+		{
+			"name":            "bandwidth",


@ronensc what is the difference from the previous test? (do we need this test ?)

eranra · 2022-02-27T14:55:22Z

pkg/pipeline/extract/extract_aggregate_test.go

+			"count":                 "2",
+		},
+		{
+			"name":                  "bandwidth_count",


The same question seems that these tests again count. ...

BTW; maybe add also avg test?

eranra · 2022-02-27T14:57:59Z

pkg/pipeline/extract/extract_aggregate_test.go

+	actualAggs2 := extractAggregate.Extract(input2)
+	require.ElementsMatch(t, expectedAggs2, actualAggs2)
+
+}


@ronensc general remark, maybe put the use cases in some for loop and just run them all of them against extractAggregate --- this is very common for tests --- look here for an example b24be79#diff-168486be9e126a8de1f0588534a9bf2fd49261555cbdc3759a5be2b1d2825cd3R26

@eranra The test tests extract_aggregate as a whole. It can be thought of as an integration test between extract_aggregate.go and aggregate.go and aggregates.go.
The test sends flows in 2 batches and verifies the extractor's output after each batch. The output of the 2nd batch depends on the 1st batch. I'll check if the referred pattern is suitable here as well.

eranra · 2022-02-27T14:58:27Z

pkg/pipeline/extract/extract_aggregate_test.go

@@ -0,0 +1,172 @@
+/*
+ * Copyright (C) 2021 IBM, Inc.


we are in 2022 ... 2021 is history :-)

eranra

Looks good to me, some comments but this is good ... waiting for @KalmanMeth to review :-)

ronensc · 2022-02-27T16:03:51Z

Makefile

 .PHONY: test
 test: validate_go ## Test
-	go test -p 1 -race -covermode=atomic -coverprofile=/tmp/coverage.out ./...
+	go test -p 1 -race -coverpkg=./... -covermode=atomic -coverprofile=/tmp/coverage.out ./...


@eranra please note that I added the -coverpkg flag.
I added it because I noticed that the code coverage didn't increase for aggregate.go although this PR tests execution paths that were not tested by the other tests. The reason is that the test in this PR is in a different go package from aggregate.go. Adding the flag solves this. The following repo helped me understand this:
https://github.com/yudai/go_cover

KalmanMeth · 2022-02-28T09:03:14Z

pkg/pipeline/extract/extract_aggregate_test.go

+	input1 := []config.GenericMap{
+		{"service": "http", "bytes": 10.0},
+		{"service": "http", "bytes": 20.0},
+		{"service": "tcp", "bytes": 1.0},


Why are "bytes" floats and not integers?

I'll fix this

KalmanMeth · 2022-02-28T09:04:51Z

pkg/pipeline/extract/extract_aggregate_test.go

+			"value":           "30.000000",
+			"bandwidth_value": "30.000000",
+			"recentRawValues": []float64{10.0, 20.0},
+			"count":           "2",


We now handle integers and not just strings, so "count" should be an integer. Same for "value" and "bandwidth_value".

I used strings because that's the format returned by: GetMetrics()
I admit that I don't understand why it's string...

flowlogs-pipeline/pkg/pipeline/extract/aggregate/aggregate.go

Lines 178 to 189 in 59852cf

metrics = append(metrics, config.GenericMap{

"name": aggregate.Definition.Name,

"operation": aggregate.Definition.Operation,

"record_key": aggregate.Definition.RecordKey,

"by": strings.Join(aggregate.Definition.By, ","),

"aggregate": string(group.normalizedValues),

"value": fmt.Sprintf("%f", group.value),

"recentRawValues": group.RecentRawValues,

"count": fmt.Sprintf("%d", group.count),

aggregate.Definition.Name + "_value": fmt.Sprintf("%f", group.value),

strings.Join(aggregate.Definition.By, "_"): string(group.normalizedValues),

})

KalmanMeth · 2022-02-28T09:31:20Z

pkg/pipeline/extract/extract_aggregate_test.go

+)
+
+func createAgg(name, recordKey, by, agg, op string, value float64, count int, rrv []float64) config.GenericMap {
+	valueString := fmt.Sprintf("%f", value)


Why do we need these to be strings?

Same reason as #106 (comment)

eranra · 2022-03-01T09:41:42Z

@ronensc it looks very-very nice now :-) thanks for putting effort to create those tests :-)

ronensc self-assigned this Feb 27, 2022

ronensc changed the title ~~Add a test for extract_aggregate~~ [WIP] Add a test for extract_aggregate Feb 27, 2022

Add a test for extract_aggregate

2b82192

ronensc force-pushed the test-extract-agg branch from 759c688 to 2b82192 Compare February 27, 2022 13:55

ronensc requested review from KalmanMeth and eranra February 27, 2022 14:04

ronensc changed the title ~~[WIP] Add a test for extract_aggregate~~ Add a test for extract_aggregate Feb 27, 2022

ronensc added 3 commits February 27, 2022 16:19

Add test for count operation

6c20df8

Make test agnostic to order

09b8d5d

Add -coverpkg flag to coverage tests

cc564c4

eranra reviewed Feb 27, 2022

View reviewed changes

eranra approved these changes Feb 27, 2022

View reviewed changes

ronensc commented Feb 27, 2022

View reviewed changes

ronensc added 4 commits February 27, 2022 18:06

Update year in copyright comment

f6ebc9a

Refactor test to use test cases

e63d36b

Refactor test to use util function

00bb41a

Add test for max

04af302

KalmanMeth reviewed Feb 28, 2022

View reviewed changes

KalmanMeth approved these changes Feb 28, 2022

View reviewed changes

ronensc added 2 commits February 28, 2022 11:07

Add test for min

6cc8c71

Add test for avg

b7e27a8

KalmanMeth reviewed Feb 28, 2022

View reviewed changes

ronensc added 2 commits February 28, 2022 11:44

Change type of bytes float->int

f2e2ba8

Fix test

7d74fce

eranra merged commit 510ca74 into netobserv:main Mar 1, 2022

ronensc deleted the test-extract-agg branch March 1, 2022 09:49

	metrics = append(metrics, config.GenericMap{
	"name": aggregate.Definition.Name,
	"operation": aggregate.Definition.Operation,
	"record_key": aggregate.Definition.RecordKey,
	"by": strings.Join(aggregate.Definition.By, ","),
	"aggregate": string(group.normalizedValues),
	"value": fmt.Sprintf("%f", group.value),
	"recentRawValues": group.RecentRawValues,
	"count": fmt.Sprintf("%d", group.count),
	aggregate.Definition.Name + "_value": fmt.Sprintf("%f", group.value),
	strings.Join(aggregate.Definition.By, "_"): string(group.normalizedValues),
	})

Add a test for extract_aggregate #106

Add a test for extract_aggregate #106

Uh oh!

Conversation

ronensc commented Feb 27, 2022

Uh oh!

codecov-commenter commented Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eranra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eranra commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Feb 27, 2022 •

edited

Loading