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

Make Peer validation more comprehensive #3104

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

GraysonWu
Copy link
Contributor

Fixes #3084

Signed-off-by: wgrayson [email protected]

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.07%. Comparing base (b87b76c) to head (ba1ed4c).
Report is 1956 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3104      +/-   ##
==========================================
- Coverage   60.83%   52.07%   -8.77%     
==========================================
  Files         295      457     +162     
  Lines       24892    51603   +26711     
==========================================
+ Hits        15142    26870   +11728     
- Misses       8095    22485   +14390     
- Partials     1655     2248     +593     
Flag Coverage Δ
e2e-tests 54.00% <100.00%> (?)
integration-tests 31.37% <ø> (?)
kind-e2e-tests 47.83% <0.00%> (+0.08%) ⬆️
unit-tests 40.37% <0.00%> (+0.02%) ⬆️

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

Files with missing lines Coverage Δ
pkg/controller/networkpolicy/validate.go 71.14% <100.00%> (+49.88%) ⬆️

... and 450 files with indirect coverage changes

Dyanngg
Dyanngg previously approved these changes Dec 8, 2021
Copy link
Contributor

@Dyanngg Dyanngg left a comment

Choose a reason for hiding this comment

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

LGTM, let's also add E2E test to verify this as well?

pkg/controller/networkpolicy/validate.go Outdated Show resolved Hide resolved
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM.
Not specific to this PR, but I feel it's a bit overkill to have so many testInvalidACNPXXXX e2e tests, which could have been tested by unit tests.
@GraysonWu @Dyanngg @antoninbas what you think?

@@ -427,6 +427,46 @@ func testInvalidACNPAppliedToNotSetInAllRules(t *testing.T) {
}
}

func testInvalidACNPAppliedToGroupSetWithPodSelector(t *testing.T) {
invalidNpErr := fmt.Errorf("invalid Antrea ClusterNetworkPolicy with appliedTo set group with podSelector")
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should consider moving some e2e tests to unit tests if they don't require a whole system to validate the code. For example, the illegal inputs that can be validated by calling the validation function directly. This can enable greater converage with a table driven tests and reduce resources spent on e2e tests.

@antoninbas
Copy link
Contributor

I agree with Quan, we should move these tests to unit tests

@GraysonWu
Copy link
Contributor Author

Make sense to me too. I could open another PR to move those tests to unit tests.

@GraysonWu
Copy link
Contributor Author

/test-all

@tnqn
Copy link
Member

tnqn commented Dec 10, 2021

/test-integration

@tnqn tnqn merged commit 2fcec4a into antrea-io:main Dec 10, 2021
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.

Validation of group as a stand-alone selector is not comprehensive
5 participants