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.

Sign up for GitHub

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

Jump to bottom

Improve unit test coverage #4142

Open
22 of 37 tasks
tnqn opened this issue Aug 22, 2022 · 10 comments
Open
22 of 37 tasks

Improve unit test coverage #4142

tnqn opened this issue Aug 22, 2022 · 10 comments
Labels
area/test Issues or PRs related to unit and integration tests. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@tnqn
Copy link
Member

tnqn commented Aug 22, 2022 •
edited by luolanzone
Loading

Describe the bug

To ensure quality code and features being delivered, code coverage is one of the most important metrics we rely on. With good coverage for regression testing, there is also much less risk of breaking existing functionality when adding or changing features.
Currently Antrea’s entire coverage is 66.02%, with 49.12% e2e test coverage, 44.70% unit test coverage, 35.00% integration test coverage. Even though unit test is the easiest and the most cost efficient one, its coverage is only 44.70%, less than e2e test, which means there is a lot of room for improvement.

This issue tracks packages that should be well unit tested but were not. If you are familiar with or interested in understanding a package's implementation, feel free to pick up the package by adding your name after the package name to indicate you will work on it or comment the issue if you cannot edit it, write unit tests for it and submit your code.

Expected

Unit test coverage greater than 70%

Actual behavior

Unit test coverage less than 70%
@tnqn tnqn added area/test Issues or PRs related to unit and integration tests. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Aug 22, 2022
@antoninbas
Copy link
Contributor

antoninbas commented Aug 22, 2022 •
edited
Loading

@tnqn do you have the link with code coverage info per package?

Edit: found it. For reference of others: https://app.codecov.io/gh/antrea-io/antrea/branch/main

antoninbas added a commit that referenced this issue Aug 23, 2022
@antoninbas
Use standard value type for k8s.v1.cni.cncf.io/networks annotation
931a7d4 
For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/network-attachment-definition-client@v1.3.0/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement

By switching to the standard type:
* we avoid user confusion
* we ensure compatibility with other solutions such as Multus
* we support "edge" cases that were not supported before: 1) using a
  NetworkAttachmentDefinition CR defined in a different Namespace than
  the Pod, 2) providing the NetworkAttachmentDefinition CR name as
  `[namespace/]name[@interface]`
* we can use the standard ParseNetworkAnnotation function and avoid
  potential bugs or inconsistencies.

This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" mode (for the "antrea" network
type).

In addition to this change, we add a good amount of unit tests, in the
context of #4142.

Signed-off-by: Antonin Bas <abas@vmware.com>
antoninbas added a commit to antoninbas/antrea that referenced this issue Aug 23, 2022
@antoninbas
Use standard value type for k8s.v1.cni.cncf.io/networks annotation
80f94e2 
For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/network-attachment-definition-client@v1.3.0/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement

By switching to the standard type:
* we avoid user confusion
* we ensure compatibility with other solutions such as Multus
* we support "edge" cases that were not supported before: 1) using a
  NetworkAttachmentDefinition CR defined in a different Namespace than
  the Pod, 2) providing the NetworkAttachmentDefinition CR name as
  `[namespace/]name[@interface]`
* we can use the standard ParseNetworkAnnotation function and avoid
  potential bugs or inconsistencies.

This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" mode (for the "antrea" network
type).

In addition to this change, we add a good amount of unit tests, in the
context of antrea-io#4142.

Signed-off-by: Antonin Bas <abas@vmware.com>
@antoninbas antoninbas mentioned this issue Aug 23, 2022
Merged
antoninbas added a commit to antoninbas/antrea that referenced this issue Aug 23, 2022
@antoninbas
Use standard value type for k8s.v1.cni.cncf.io/networks annotation
536fc6b 
For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/network-attachment-definition-client@v1.3.0/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement

By switching to the standard type:
* we avoid user confusion
* we ensure compatibility with other solutions such as Multus
* we support "edge" cases that were not supported before: 1) using a
  NetworkAttachmentDefinition CR defined in a different Namespace than
  the Pod, 2) providing the NetworkAttachmentDefinition CR name as
  `[namespace/]name[@interface]`
* we can use the standard ParseNetworkAnnotation function and avoid
  potential bugs or inconsistencies.

This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" mode (for the "antrea" network
type).

In addition to this change, we add a good amount of unit tests, in the
context of antrea-io#4142.

Signed-off-by: Antonin Bas <abas@vmware.com>
antoninbas added a commit to antoninbas/antrea that referenced this issue Aug 23, 2022
@antoninbas
Use standard value type for k8s.v1.cni.cncf.io/networks annotation
5fef5a1 
For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/network-attachment-definition-client@v1.3.0/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement

By switching to the standard type:
* we avoid user confusion
* we ensure compatibility with other solutions such as Multus
* we support "edge" cases that were not supported before: 1) using a
  NetworkAttachmentDefinition CR defined in a different Namespace than
  the Pod, 2) providing the NetworkAttachmentDefinition CR name as
  `[namespace/]name[@interface]`
* we can use the standard ParseNetworkAnnotation function and avoid
  potential bugs or inconsistencies.

This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" mode (for the "antrea" network
type).

In addition to this change, we add a good amount of unit tests, in the
context of antrea-io#4142.

Signed-off-by: Antonin Bas <abas@vmware.com>
antoninbas added a commit to antoninbas/antrea that referenced this issue Aug 23, 2022
@antoninbas
Use standard value type for k8s.v1.cni.cncf.io/networks annotation
aad9225 
For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/network-attachment-definition-client@v1.3.0/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement

By switching to the standard type:
* we avoid user confusion
* we ensure compatibility with other solutions such as Multus
* we support "edge" cases that were not supported before: 1) using a
  NetworkAttachmentDefinition CR defined in a different Namespace than
  the Pod, 2) providing the NetworkAttachmentDefinition CR name as
  `[namespace/]name[@interface]`
* we can use the standard ParseNetworkAnnotation function and avoid
  potential bugs or inconsistencies.

This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" mode (for the "antrea" network
type).

In addition to this change, we add a good amount of unit tests, in the
context of antrea-io#4142.

Signed-off-by: Antonin Bas <abas@vmware.com>
antoninbas added a commit to antoninbas/antrea that referenced this issue Aug 25, 2022
@antoninbas
Use standard value type for k8s.v1.cni.cncf.io/networks annotation
436e8db 
For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/network-attachment-definition-client@v1.3.0/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement

By switching to the standard type:
* we avoid user confusion
* we ensure compatibility with other solutions such as Multus
* we support "edge" cases that were not supported before: 1) using a
  NetworkAttachmentDefinition CR defined in a different Namespace than
  the Pod, 2) providing the NetworkAttachmentDefinition CR name as
  `[namespace/]name[@interface]`
* we can use the standard ParseNetworkAnnotation function and avoid
  potential bugs or inconsistencies.

This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" mode (for the "antrea" network
type).

In addition to this change, we add a good amount of unit tests, in the
context of antrea-io#4142.

Signed-off-by: Antonin Bas <abas@vmware.com>
@wenqiq wenqiq mentioned this issue Aug 25, 2022
Merged
@luolanzone
Copy link
Contributor

luolanzone commented Aug 26, 2022 •
edited
Loading

Hi @tnqn I was trying to take the pkg/apiserver/certificate packages and add unit test, but after checking the codecov data, I feel the coverage is good enough with e2e/integration test data, it might be unnecessary to add more cases for this packages, could you help to check and let me know your opinion? thanks
https://app.codecov.io/gh/antrea-io/antrea/tree/main/pkg/apiserver/certificate
Personally I would suggest to check the overall data coverage and make sure we add the test for those lines which are not covered by any kinds of tests.

antoninbas added a commit to antoninbas/antrea that referenced this issue Aug 26, 2022
@antoninbas
Use standard value type for k8s.v1.cni.cncf.io/networks annotation
f8d41be 
For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/network-attachment-definition-client@v1.3.0/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement

By switching to the standard type:
* we avoid user confusion
* we ensure compatibility with other solutions such as Multus
* we support "edge" cases that were not supported before: 1) using a
  NetworkAttachmentDefinition CR defined in a different Namespace than
  the Pod, 2) providing the NetworkAttachmentDefinition CR name as
  `[namespace/]name[@interface]`
* we can use the standard ParseNetworkAnnotation function and avoid
  potential bugs or inconsistencies.

This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" mode (for the "antrea" network
type).

In addition to this change, we add a good amount of unit tests, in the
context of antrea-io#4142.

Signed-off-by: Antonin Bas <abas@vmware.com>
@luolanzone luolanzone mentioned this issue Aug 29, 2022
Merged
antoninbas added a commit that referenced this issue Aug 29, 2022
@antoninbas
Use standard value type for k8s.v1.cni.cncf.io/networks annotation (#…
222c2b6 
…4146)

For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/network-attachment-definition-client@v1.3.0/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement

By switching to the standard type:
* we avoid user confusion
* we ensure compatibility with other solutions such as Multus
* we support "edge" cases that were not supported before: 1) using a
  NetworkAttachmentDefinition CR defined in a different Namespace than
  the Pod, 2) providing the NetworkAttachmentDefinition CR name as
  `[namespace/]name[@interface]`
* we can use the standard ParseNetworkAnnotation function and avoid
  potential bugs or inconsistencies.

This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" networkType.

In addition to this change, we add a good amount of unit tests, in the
context of #4142.

Signed-off-by: Antonin Bas <abas@vmware.com>
@yanjunz97 yanjunz97 mentioned this issue Aug 30, 2022
Merged
antoninbas added a commit to antoninbas/antrea that referenced this issue Aug 30, 2022
@antoninbas
Add more unit tests for secondarynetwork feature
9c6d60c 
Fix one of the tests (`TestPodControllerAddPod/missing_Status.PodIPs`)
and add a few more unit tests.

For antrea-io#4142

Signed-off-by: Antonin Bas <abas@vmware.com>
@antoninbas antoninbas mentioned this issue Aug 30, 2022
Merged
heanlan added a commit to heanlan/antrea that referenced this issue Aug 30, 2022
@heanlan
Improve unit test of flowexporter package
3ec23f1 
Fix one of the unit test TestFlowExporter_sendFlowRecords, and
adds a testcase to TestFlowExporter_initFlowExporter.

For antrea-io#4142

Signed-off-by: heanlan <hanlan@vmware.com>
@heanlan heanlan mentioned this issue Aug 30, 2022
Merged
@tnqn
Copy link
Member Author

tnqn commented Aug 31, 2022 •
edited
Loading

Hi @tnqn I was trying to take the pkg/apiserver/certificate packages and add unit test, but after checking the codecov data, I feel the coverage is good enough with e2e/integration test data, it might be unnecessary to add more cases for this packages, could you help to check and let me know your opinion? thanks
https://app.codecov.io/gh/antrea-io/antrea/tree/main/pkg/apiserver/certificate
Personally I would suggest to check the overall data coverage and make sure we add the test for those lines which are not covered by any kinds of tests.

@luolanzone I made the list based on the unit test coverage report. It was 30% for this package as there is no unit test for cacert_controller.go. And I understand why its overall coverage looks fine: as long as the program runs end to end, many code in cacert_controller.go will run at least once regardless of whether the e2e test is intended to test it. But running the code once doesn't mean it's well tested, for example, if CACertController.syncConfigMap is well tested,the following situations should be covered:

  1. If the ConfigMap doesn't exist, it will call the Create API
  2. If the ConfigMap exists and the cert matches, it will not call the Update API
  3. If the ConfigMap exists and the cert doesn't match, it will call the Update API
  4. If it fails to check ConfigMap's existence, it will return the error

I think the purpose of improving test coverage is not to make the code to be executed once by any kind of test, but to ensure the code is executed as expected in various situation.

antoninbas added a commit to antoninbas/antrea that referenced this issue Aug 31, 2022
@antoninbas
Add more unit tests for secondarynetwork feature
04c75e4 
Fix one of the tests (`TestPodControllerAddPod/missing_Status.PodIPs`)
and add a few more unit tests.

For antrea-io#4142

Signed-off-by: Antonin Bas <abas@vmware.com>
antoninbas added a commit that referenced this issue Sep 1, 2022
@antoninbas
Add more unit tests for secondarynetwork feature (#4180)
ff15b16 
Fix one of the tests (`TestPodControllerAddPod/missing_Status.PodIPs`)
and add a few more unit tests.

For #4142

Signed-off-by: Antonin Bas <abas@vmware.com>
@yuntanghsu yuntanghsu mentioned this issue Sep 2, 2022
Merged
heanlan added a commit to heanlan/antrea that referenced this issue Sep 2, 2022
@heanlan
Improve unit test of flowexporter package
c4dd278 
Add more test cases for pkg/agent/flowexporter package and
its subpackages: connections, exporter, priorityqueue.

For antrea-io#4142

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue Sep 2, 2022
@heanlan
Improve unit test of flowexporter package
c87a2f6 
Add more test cases for pkg/agent/flowexporter package and
its subpackages: connections, exporter, priorityqueue.

For antrea-io#4142

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue Sep 2, 2022
@heanlan
Improve unit test of flowexporter package
aefef72 
Add more test cases for pkg/agent/flowexporter package and
its subpackages: connections, exporter, priorityqueue.

For antrea-io#4142

Signed-off-by: heanlan <hanlan@vmware.com>
This was referenced Sep 5, 2022
Merged
Merged
heanlan added a commit to heanlan/antrea that referenced this issue Sep 6, 2022
@heanlan
Improve unit test of flowexporter package
7ab0f23 
Add more test cases for pkg/agent/flowexporter package and
its subpackages: connections, exporter, priorityqueue.

For antrea-io#4142

Signed-off-by: heanlan <hanlan@vmware.com>
@urharshitha urharshitha mentioned this issue Dec 7, 2022
Merged
@NamanAg30 NamanAg30 mentioned this issue Dec 23, 2022
Merged
@rajnkamr rajnkamr pinned this issue Jan 3, 2023
@urharshitha urharshitha mentioned this issue Jan 23, 2023
Merged
@Atish-iaf Atish-iaf mentioned this issue Jan 31, 2023
Merged
@urharshitha urharshitha mentioned this issue Feb 1, 2023
Closed
This was referenced Feb 2, 2023
Closed
Merged
@elton-furtado elton-furtado unpinned this issue Feb 13, 2023
@urharshitha urharshitha mentioned this issue Feb 15, 2023
Merged
@NamanAg30 NamanAg30 mentioned this issue Mar 28, 2023
Merged
heanlan pushed a commit to heanlan/antrea that referenced this issue Mar 29, 2023
@antoninbas @heanlan
Use standard value type for k8s.v1.cni.cncf.io/networks annotation (a…
7a7e502 
…ntrea-io#4146)

For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/network-attachment-definition-client@v1.3.0/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement

By switching to the standard type:
* we avoid user confusion
* we ensure compatibility with other solutions such as Multus
* we support "edge" cases that were not supported before: 1) using a
  NetworkAttachmentDefinition CR defined in a different Namespace than
  the Pod, 2) providing the NetworkAttachmentDefinition CR name as
  `[namespace/]name[@interface]`
* we can use the standard ParseNetworkAnnotation function and avoid
  potential bugs or inconsistencies.

This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" networkType.

In addition to this change, we add a good amount of unit tests, in the
context of antrea-io#4142.

Signed-off-by: Antonin Bas <abas@vmware.com>
heanlan pushed a commit to heanlan/antrea that referenced this issue Mar 29, 2023
@antoninbas @heanlan
Add more unit tests for secondarynetwork feature (antrea-io#4180)
cde7c92 
Fix one of the tests (`TestPodControllerAddPod/missing_Status.PodIPs`)
and add a few more unit tests.

For antrea-io#4142

Signed-off-by: Antonin Bas <abas@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue Mar 29, 2023
@heanlan
Improve unit test of flowexporter package (antrea-io#4182)
e6c04f9 
Add more test cases for pkg/agent/flowexporter package and
its subpackages: connections, exporter, priorityqueue.

For antrea-io#4142

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan pushed a commit to heanlan/antrea that referenced this issue Mar 29, 2023
@yuntanghsu @heanlan
Add unit tests for pkg/flowaggregator/exporter (antrea-io#4195)
ab25ecc 
For antrea-io#4142 

Signed-off-by: Yun-Tang Hsu <hsuy@vmware.com>
@KMAnju-2021 KMAnju-2021 mentioned this issue Apr 20, 2023
Merged
@github-actions
Copy link
Contributor

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 11, 2023
@luolanzone luolanzone removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 14, 2023
@rajnkamr rajnkamr mentioned this issue Sep 7, 2023
Open
@VaibhavMalik4187
Copy link
Contributor

@luolanzone Can I work on improving the code coverage of the cmd package?

@github-actions GitHub Actions
Copy link
Contributor

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 24, 2024
@hongliangl hongliangl removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 24, 2024
@luolanzone luolanzone removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 24, 2024
@luolanzone
Copy link
Contributor

Hi @VaibhavMalik4187 , sorry for a quite late response, you can work on coverage improvement if you are still interested in contribution. You can check this link https://app.codecov.io/gh/antrea-io/antrea/tree/codecov-ut-only to check overall Antrea coverage and take any package which coverage is low. Thanks a lot!

@github-actions GitHub Actions
Copy link
Contributor

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/test Issues or PRs related to unit and integration tests. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@antoninbas @tnqn @rajnkamr @VaibhavMalik4187 @hongliangl @luolanzone