chore: add unit testcases for files in Text format. #2274

Electronic-Waste · 2024-03-05T14:50:01Z

What this PR does / why we need it:

Add unit test cases for files in Text format in file-metricscollector_test.go, including:

"Positive case for logs in TEXT format" test case
"Invalid file name" test case
"Invalid file format" test case
"Invalid timestamp for logs in TEXT format" test case
"Missing objective metric in training logs" test case

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1756

Checklist:

Docs included if any changes are user facing

Signed-off-by: Electronic-Waste <2690692950@qq.com>

Electronic-Waste · 2024-03-05T14:56:38Z

The newly added code obeys the rules in Developer Guide.

Electronic-Waste · 2024-03-05T15:06:18Z

All unit test cases passed.

tenzen-y · 2024-03-05T15:07:33Z

/cc @tenzen-y

Signed-off-by: Electronic-Waste <2690692950@qq.com>

tenzen-y

My first pass.

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go

Signed-off-by: Electronic-Waste <2690692950@qq.com>

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go

tenzen-y · 2024-03-09T15:15:15Z

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go

+		"Positive case for logs in TEXT format": {
+			filePath:   filepath.Join(testTextDataPath, "good.log"),
+			metrics:    []string{"accuracy", "loss"},
+			filters:    []string{"{metricName: ([\\w|-]+), metricValue: ((-?\\d+)(\\.\\d+)?)}"},


As we mentioned in the issue, our motivation is to verify if this filter works fine to avoid bugs like this: #1754

We should add unit test for our File Metrics Collector to verify log parsing.
That will help to avoid issues similar to: #1754

So, could you add more than valid and invalid cases?

Yes, I'll do it later.

Some new test cases similar to bugs in #1754 have been added to the test file.

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go

Signed-off-by: Electronic-Waste <2690692950@qq.com>

tenzen-y

LGTM
Thanks!

/lgtm
/approve
/hold
for Ci

@kubeflow/wg-automl-leads Could you approve CI?

Electronic-Waste · 2024-03-10T02:28:54Z

It seems that CI does not start yet...Is it manipulated by someone?

andreyvelich · 2024-03-10T02:40:24Z

Thank you for this @Electronic-Waste, I will review PR on Monday. I triggered CI.
/assign @andreyvelich

Electronic-Waste · 2024-03-10T02:43:14Z

Thank you for this @Electronic-Waste, I will review PR on Monday. I triggered CI. /assign @andreyvelich

@andreyvelich Okay, thx for your clarification.

Signed-off-by: Electronic-Waste <2690692950@qq.com>

tenzen-y · 2024-03-12T01:03:41Z

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go

+	if err := generateJSONTestFiles(); err != nil {
+		t.Fatal(err)
+	}
+	if err := generateTEXTTestFiles(); err != nil {


Did you push the commit? I can still see 2 separate functions.

Yes, I pushed the commit. I just integrated dir creation & deletion operations into functions. Test file generation is not included since it's not necessary.

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go

Signed-off-by: Electronic-Waste <2690692950@qq.com>

Electronic-Waste · 2024-03-12T12:51:13Z

@andreyvelich Is there anything else wrong with this PR? I can modify them soon.

andreyvelich

Thank you for the updates @Electronic-Waste!
Congratulation with your first contribution to Kubeflow 🎉

/lgtm
/assign @tenzen-y @johnugeorge

andreyvelich · 2024-03-12T13:00:03Z

/hold for CI/CD and review from @tenzen-y and @johnugeorge

Electronic-Waste · 2024-03-12T13:08:34Z

I'm glad to see that my first PR is going to be merged!
It's a nice experience collaborating with you @andreyvelich @tenzen-y .

andreyvelich · 2024-03-12T13:19:13Z

You might need to fix CI: https://github.com/kubeflow/katib/actions/runs/8249035770/job/22561234937?pr=2274#step:4:1330

Electronic-Waste · 2024-03-12T13:22:48Z

@andreyvelich I see. The new commit is added to this PR.

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go

Signed-off-by: Electronic-Waste <2690692950@qq.com>

Electronic-Waste · 2024-03-12T13:27:32Z

@andreyvelich A new commit is added to the PR. Sorry for my mistake.

andreyvelich · 2024-03-12T14:46:30Z

Tests were succeeded!
/lgtm
/assign @tenzen-y @johnugeorge

tenzen-y

Generally, lgtm

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go

google-oss-prow · 2024-03-12T18:12:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Electronic-Waste, tenzen-y

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tenzen-y]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chore: add unit testcases for files in Text format.

876bdf9

Signed-off-by: Electronic-Waste <2690692950@qq.com>

google-oss-prow bot requested review from andreyvelich, anencore94 and gaocegege March 5, 2024 14:50

google-oss-prow bot added the size/L label Mar 5, 2024

google-oss-prow bot requested a review from tenzen-y March 5, 2024 15:07

chore: adjust file layout using gofmt.

f6b56c4

Signed-off-by: Electronic-Waste <2690692950@qq.com>

tenzen-y reviewed Mar 6, 2024

View reviewed changes

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go Outdated Show resolved Hide resolved

chore: combine test for JSON and TEXT file format.

984862b

Signed-off-by: Electronic-Waste <2690692950@qq.com>

Electronic-Waste requested a review from tenzen-y March 6, 2024 16:04

tenzen-y reviewed Mar 7, 2024

View reviewed changes

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go Outdated Show resolved Hide resolved

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go Show resolved Hide resolved

Electronic-Waste requested a review from tenzen-y March 7, 2024 08:36

tenzen-y reviewed Mar 9, 2024

View reviewed changes

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go Outdated Show resolved Hide resolved

Electronic-Waste added 3 commits March 9, 2024 23:26

fix: rename file-gen functions.

90a096a

Signed-off-by: Electronic-Waste <2690692950@qq.com>

refactor: update cmd.Diff params and log outputs.

4dfc659

Signed-off-by: Electronic-Waste <2690692950@qq.com>

chore: add more valid and invalid testcases for TEXT format.

42f2d45

Signed-off-by: Electronic-Waste <2690692950@qq.com>

Electronic-Waste requested a review from tenzen-y March 9, 2024 16:01

tenzen-y reviewed Mar 9, 2024

View reviewed changes

google-oss-prow bot added the do-not-merge/hold label Mar 9, 2024

google-oss-prow bot assigned tenzen-y Mar 9, 2024

google-oss-prow bot added lgtm approved labels Mar 9, 2024

google-oss-prow bot assigned andreyvelich Mar 10, 2024

chore: compact dir generation & deletion operations into funcs.

c5554f1

Signed-off-by: Electronic-Waste <2690692950@qq.com>

Electronic-Waste requested a review from tenzen-y March 12, 2024 00:59

tenzen-y reviewed Mar 12, 2024

View reviewed changes

Electronic-Waste requested a review from tenzen-y March 12, 2024 02:16

andreyvelich reviewed Mar 12, 2024

View reviewed changes

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go Outdated Show resolved Hide resolved

fix: delete constants used only once.

2496e94

Signed-off-by: Electronic-Waste <2690692950@qq.com>

Electronic-Waste requested a review from andreyvelich March 12, 2024 12:46

andreyvelich reviewed Mar 12, 2024

View reviewed changes

google-oss-prow bot assigned johnugeorge Mar 12, 2024

google-oss-prow bot added the lgtm label Mar 12, 2024

google-oss-prow bot removed the lgtm label Mar 12, 2024

andreyvelich reviewed Mar 12, 2024

View reviewed changes

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go Outdated Show resolved Hide resolved

fix: fix ci error in errorcheck.

82c248f

Signed-off-by: Electronic-Waste <2690692950@qq.com>

Electronic-Waste force-pushed the issue-1756 branch from b60e35c to 82c248f Compare March 12, 2024 13:25

google-oss-prow bot added the lgtm label Mar 12, 2024

tenzen-y reviewed Mar 12, 2024

View reviewed changes

pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go Show resolved Hide resolved

google-oss-prow bot removed the do-not-merge/hold label Mar 12, 2024

google-oss-prow bot merged commit 5837b8a into kubeflow:master Mar 12, 2024
59 checks passed

tariq-hasan mentioned this pull request Mar 12, 2024

Replaced reflect.DeepEqual with cmp.Diff in Go tests #2281

Closed

1 task

Electronic-Waste deleted the issue-1756 branch March 14, 2024 02:00

Electronic-Waste mentioned this pull request Mar 14, 2024

fix: clean up UTs for file metrics collector #2285

Merged

1 task

Electronic-Waste mentioned this pull request Apr 8, 2024

Add Electronic-Waste as member. kubeflow/internal-acls#669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add unit testcases for files in Text format. #2274

chore: add unit testcases for files in Text format. #2274

Electronic-Waste commented Mar 5, 2024

Electronic-Waste commented Mar 5, 2024 •

edited

Loading

Electronic-Waste commented Mar 5, 2024

tenzen-y commented Mar 5, 2024

tenzen-y left a comment

tenzen-y Mar 9, 2024

Electronic-Waste Mar 9, 2024

Electronic-Waste Mar 9, 2024

tenzen-y left a comment

Electronic-Waste commented Mar 10, 2024 •

edited

Loading

andreyvelich commented Mar 10, 2024

Electronic-Waste commented Mar 10, 2024

tenzen-y Mar 12, 2024

Electronic-Waste Mar 12, 2024 •

edited

Loading

Electronic-Waste commented Mar 12, 2024

andreyvelich left a comment

andreyvelich commented Mar 12, 2024 •

edited

Loading

Electronic-Waste commented Mar 12, 2024

andreyvelich commented Mar 12, 2024

Electronic-Waste commented Mar 12, 2024

Electronic-Waste commented Mar 12, 2024

andreyvelich commented Mar 12, 2024 •

edited

Loading

tenzen-y left a comment

google-oss-prow bot commented Mar 12, 2024

chore: add unit testcases for files in Text format. #2274

chore: add unit testcases for files in Text format. #2274

Conversation

Electronic-Waste commented Mar 5, 2024

Electronic-Waste commented Mar 5, 2024 • edited Loading

Electronic-Waste commented Mar 5, 2024

tenzen-y commented Mar 5, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y Mar 9, 2024

Choose a reason for hiding this comment

Electronic-Waste Mar 9, 2024

Choose a reason for hiding this comment

Electronic-Waste Mar 9, 2024

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

Electronic-Waste commented Mar 10, 2024 • edited Loading

andreyvelich commented Mar 10, 2024

Electronic-Waste commented Mar 10, 2024

tenzen-y Mar 12, 2024

Choose a reason for hiding this comment

Electronic-Waste Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Electronic-Waste commented Mar 12, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

andreyvelich commented Mar 12, 2024 • edited Loading

Electronic-Waste commented Mar 12, 2024

andreyvelich commented Mar 12, 2024

Electronic-Waste commented Mar 12, 2024

Electronic-Waste commented Mar 12, 2024

andreyvelich commented Mar 12, 2024 • edited Loading

tenzen-y left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Mar 12, 2024

Electronic-Waste commented Mar 5, 2024 •

edited

Loading

Electronic-Waste commented Mar 10, 2024 •

edited

Loading

Electronic-Waste Mar 12, 2024 •

edited

Loading

andreyvelich commented Mar 12, 2024 •

edited

Loading

andreyvelich commented Mar 12, 2024 •

edited

Loading