Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cnf network: sriov metrics exporter test cases #183

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

ajaggapa
Copy link
Collaborator

No description provided.

@ajaggapa ajaggapa force-pushed the metricsfeature branch 2 times, most recently from c7c57ed to 5a37445 Compare August 28, 2024 10:15
Copy link
Collaborator

@evgenLevin evgenLevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, Just severall comments

tests/cnf/core/network/sriov/tests/metricsExporter.go Outdated Show resolved Hide resolved
tests/cnf/core/network/sriov/tests/metricsExporter.go Outdated Show resolved Hide resolved
tests/cnf/core/network/sriov/tests/metricsExporter.go Outdated Show resolved Hide resolved
tests/cnf/core/network/sriov/tests/metricsExporter.go Outdated Show resolved Hide resolved
tests/cnf/core/network/sriov/tests/metricsExporter.go Outdated Show resolved Hide resolved
tests/cnf/core/network/sriov/tests/metricsExporter.go Outdated Show resolved Hide resolved
tests/cnf/core/network/sriov/tests/metricsExporter.go Outdated Show resolved Hide resolved
tests/cnf/core/network/sriov/tests/metricsExporter.go Outdated Show resolved Hide resolved
@ajaggapa ajaggapa force-pushed the metricsfeature branch 2 times, most recently from dd1dc08 to 693760e Compare August 30, 2024 11:43
@ajaggapa ajaggapa changed the title cnf network: sriov metrics exporter : netdevice to netdevice test cases cnf network: sriov metrics exporter test cases Aug 30, 2024
@ajaggapa ajaggapa force-pushed the metricsfeature branch 3 times, most recently from 9e83ac3 to caf9da1 Compare August 30, 2024 14:07
Copy link
Collaborator

@evgenLevin evgenLevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments

})

Context("Sriov Metrics Exporter - Netdevice to Netdevice", func() {
It("Sriov Metrics Exporter - Netdevice to Netdevice - Same PF", reportxml.ID("74762"), func() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change the test names to:

Same PF
Different PF
Different Worker

Otherwrise the full name will be:
SriovMetricsExporter Sriov Metrics Exporter - Netdevice to Netdevice Sriov Metrics Exporter - Netdevice to Netdevice - Same PF

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

})
})

Context("Sriov Metrics Exporter - Netdevice to Vfiopci", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also test naming issue

tests/cnf/core/network/sriov/tests/metricsExporter.go Outdated Show resolved Hide resolved
WithResourceRequest("1Gi", "1Gi", 4).
WithEnvVar("RUN_TYPE", "testcmd").
GetContainerCfg()
Expect(err).ToNot(HaveOccurred())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please a failure comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

NetConfig.PrometheusOperatorNamespace, metav1.ListOptions{LabelSelector: "prometheus=k8s"})
Expect(err).ToNot(HaveOccurred(), "Failed to get prometheus pods")

glog.V(90).Infof(fmt.Sprintf("Running PromQL query: %s", query))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same, please replace glog by By

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Collaborator

@evgenLevin evgenLevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Just one super minor comment

}

func definePod(role, devType, worker string) *pod.Builder {
var podbuild *pod.Builder
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls remove and use :=

@ajaggapa ajaggapa force-pushed the metricsfeature branch 2 times, most recently from 16dfb17 to d1b3311 Compare September 11, 2024 16:03
Copy link
Collaborator

@evgenLevin evgenLevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

Copy link
Collaborator

@gkopels gkopels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two really small comments. Lots good!

})
})

Context("Sriov Metrics Exporter - Netdevice to Vfiopci - ", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need a space after - ? Is a space automatically added between Context and It for Polarion?
Same for above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how it works with Polarion when it merges Context and It descriptions. So added space just in case it goes otherwise.

Copy link
Collaborator

@gkopels gkopels Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked on an existing test it adds a space automatically between the context and It you can remove the space

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i tested it today. So removed spaces now 👍

},
90*time.Second, 30*time.Second).Should(BeTrue(), "PromQL output does not contain server pod metrics")

By("Verify RX and TX packets counters are >0")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NitPicking: > 0"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@gkopels
Copy link
Collaborator

gkopels commented Sep 13, 2024

lgtm

@gkopels gkopels merged commit 9943f43 into openshift-kni:main Sep 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants