katib icon indicating copy to clipboard operation
katib copied to clipboard

Add unit tests for File Metrics Collector

Open andreyvelich opened this issue 4 years ago • 15 comments

/kind feature

We should add unit test for our File Metrics Collector to verify log parsing. That will help to avoid issues similar to: https://github.com/kubeflow/katib/issues/1754

/help /good-first-issue


Love this feature? Give it a 👍 We prioritize the features with the most 👍

andreyvelich avatar Dec 01 '21 21:12 andreyvelich

@andreyvelich: This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-good-first-issue command.

In response to this:

/kind feature

We should add unit test for our File Metrics Collector to verify log parsing. That will help to avoid issues similar to: https://github.com/kubeflow/katib/issues/1754

/help /good-first-issue


Love this feature? Give it a 👍 We prioritize the features with the most 👍

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

google-oss-prow[bot] avatar Dec 01 '21 21:12 google-oss-prow[bot]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Mar 02 '22 09:03 stale[bot]

/lifecycle frozen

tenzen-y avatar Mar 02 '22 16:03 tenzen-y

I want to have a try! It may help me better understand the logics of metrics collecting.

Electronic-Waste avatar Mar 04 '24 15:03 Electronic-Waste

I want to have a try! It may help me better understand the metrics collecting logics.

Sure, feel free to assign this yourself with a "/assign" comment.

tenzen-y avatar Mar 04 '24 15:03 tenzen-y

/assign

Electronic-Waste avatar Mar 04 '24 15:03 Electronic-Waste

@tenzen-y thank you!

Electronic-Waste avatar Mar 04 '24 15:03 Electronic-Waste

@tenzen-y I notice that you left a comment in the unit test file file-metricscollector_test.go:

TODO (tenzen-y): We should add tests for logs in TEXT format.

Does it mean that the additional test cases are supposed to be TEXT format? If so, can you provide an example of TEXT format log file for me? Thanks!

Electronic-Waste avatar Mar 04 '24 16:03 Electronic-Waste

@tenzen-y I notice that you left a comment in the unit test file file-metricscollector_test.go:

TODO (tenzen-y): We should add tests for logs in TEXT format.

Does it mean that the additional test cases are supposed to be TEXT format? If so, can you provide an example of TEXT format log file for me? Thanks!

Yes, that's right. We have UTs only for JSON form. So, we need to add UTs for TEXT form logs. You probably could obtain samples once you run some example Experiments.

tenzen-y avatar Mar 04 '24 16:03 tenzen-y

@andreyvelich I would love to contribute to this issue just wanted to ask that I have to add unit tests for parseLogsInTextFormat and parseLogsInJsonFormat functions and modify test cases in this file, am I right??

kshitijdshah99 avatar Mar 04 '24 17:03 kshitijdshah99

@andreyvelich I would love to contribute to this issue just wanted to ask that I have to add unit tests for parseLogsInTextFormat and parseLogsInJsonFormat functions and modify test cases in this file, am I right??

@kshitijdshah99 This is already taken by @Electronic-Waste. I'd be happy to be taken another issue by you.

tenzen-y avatar Mar 04 '24 17:03 tenzen-y

@tenzen-y Current error comparison in file-metricscollector_test.go is based on boolean rather than error content itself. To make test cases more detailed and accurate, we need to expose error and compare them. I would like to open another PR to fix this for this is a different feat compared with adding test cases for TEXT format log file.

Electronic-Waste avatar Mar 07 '24 15:03 Electronic-Waste

@tenzen-y Current error comparison in file-metricscollector_test.go is based on boolean rather than error content itself. To make test cases more detailed and accurate, we need to expose error and compare them. I would like to open another PR to fix this for this is a different feat compared with adding test cases for TEXT format log file.

SGTM

tenzen-y avatar Mar 09 '24 12:03 tenzen-y

/reopen

We need another followup PR.

tenzen-y avatar Mar 12 '24 18:03 tenzen-y

@tenzen-y: Reopened this issue.

In response to this:

/reopen

We need another followup PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

google-oss-prow[bot] avatar Mar 12 '24 18:03 google-oss-prow[bot]