Added unit tests for interpretation context handling #5850

anujagrawal699 · 2024-11-20T14:54:44Z

Description:
This PR adds comprehensive unit tests for the interpretation context handling. These tests ensure robust functionality of webhook response processing and validation for various interpreter operations.

Additions:

pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext_test.go

Test Coverage:

pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext.go: 0% to 90.80%

What type of PR is this?
/kind feature

Which issue(s) this PR fixes:
Fixes a part of #5470

Does this PR introduce a user-facing change?:

NONE

karmada-bot · 2024-11-20T14:54:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ikaven1024 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/resourceinterpreter/OWNERS

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

zhzhuang-zju · 2024-11-22T02:52:49Z

pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext_test.go

+	const (
+		testAPIVersion = "apps/v1"
+		testKind       = "Deployment"
+		testName       = "test-deployment"
+		testNamespace  = "default"
+	)


Suggested change

const (

testAPIVersion = "apps/v1"

testKind = "Deployment"

testName = "test-deployment"

testNamespace = "default"

)

var (

testAPIVersion = "apps/v1"

testKind = "Deployment"

testName = "test-deployment"

testNamespace = "default"

)

why change const to var? I think you need to give a reason for the change.

Thank you for pointing out this issue, my bad. I have searched some specification documents and did not find restrictions on defining const inside functions. It's just relatively rare for me. Therefore, this comment can be disregarded unless there is a corresponding specification found.

In my humble opinion, whether const or var has nothing to do with location, but depend on whether you want the value to be modified.

Thanks for your confirm, agree

I guess @zhzhuang-zju is concerned that different test cases have the same constants but they can't be shared. Constants we usually use are global, and variables don't have to worry about this.

You can see that we have the same constants such as testName and testNamespace in the test file.

zhzhuang-zju · 2024-11-22T03:09:11Z

pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext_test.go

+			assert.NotNil(t, ctx)
+			requestJSON, err := json.Marshal(request)
+			assert.NoError(t, err)
+			assert.Contains(t, string(requestJSON), testName)
+			assert.Contains(t, string(requestJSON), testNamespace)
+			assert.Contains(t, string(requestJSON), testAPIVersion)
+			assert.Contains(t, string(requestJSON), testKind)


Suggested change

assert.NotNil(t, ctx)

requestJSON, err := json.Marshal(request)

assert.NoError(t, err)

assert.Contains(t, string(requestJSON), testName)

assert.Contains(t, string(requestJSON), testNamespace)

assert.Contains(t, string(requestJSON), testAPIVersion)

assert.Contains(t, string(requestJSON), testKind)

// Verify the object is not nil

assert.NotNil(t, ctx)

assert.Equal(t, testName, ctx.Request.Name)

assert.Equal(t, testNamespace, ctx.Request.Namespace)

assert.Equal(t, testAPIVersion, ctx.Request.Kind.Group+ "/" +ctx.Request.Kind.Version)

assert.Equal(t, testKind, ctx.Request.Kind.Kind)

Good Suggestion!

zhzhuang-zju · 2024-11-22T03:09:57Z

pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext_test.go

+}
+
+func TestCreateV1alpha1ResourceInterpreterContext(t *testing.T) {
+	const (


Suggested change

const (

var (

zhzhuang-zju · 2024-11-22T03:15:57Z

pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext_test.go

+func TestVerifyResourceInterpreterContextWithPatch(t *testing.T) {
+	const (
+		validPatch     = `[{"op": "add", "path": "/spec/replicas", "value": 3}]`
+		malformedPatch = `[{"op": "add", "path": "/spec/replicas", "value": 3}` // missing closing bracket


malformedPatch seems haven‘t been used by the following test case

I'll remove it.

zhzhuang-zju · 2024-11-22T03:30:48Z

pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext_test.go

+
+			// Additional validation for successful cases
+			if err == nil && tt.response.Patch != nil {
+				// Verify patch is valid JSON if present


cc @XiShanYongYe-Chang Do you think it's necessary to add this check in the func verifyResourceInterpreterContextWithPatch as well?

I think we should remove it as verifyResourceInterpreterContextWithPatch function doesn't actually validate JSON.

But if this is a valid check, we could also consider adding it to verifyResourceInterpreterContextWithPatch.

Let's wait for @XiShanYongYe-Chang 's suggestion.

That's a good question, and I understand it's actually asking if we should advance the validation of patches to method verifyResourceInterpreterContextWithPatch, because we only verify them when we're using them.

I think if we want to modify this logic, we can do it in a separate pr to allow the error to leak out earlier, which is two things from the current new tests and can be handled separately.

In addition, I think this extra validation could be removed, and it's superfluous for the current method.

Okay @XiShanYongYe-Chang , Thanks!

Signed-off-by: Anuj Agrawal <[email protected]> Added unit tests for interpretation context handling Signed-off-by: Anuj Agrawal <[email protected]>

codecov-commenter · 2024-11-22T09:18:30Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.52%. Comparing base (2c82055) to head (f8f3274).
Report is 23 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5850      +/-   ##
==========================================
+ Coverage   46.31%   46.52%   +0.20%     
==========================================
  Files         661      661              
  Lines       54326    54400      +74     
==========================================
+ Hits        25163    25311     +148     
+ Misses      27537    27463      -74     
  Partials     1626     1626

Flag	Coverage Δ
unittests	`46.52% <ø> (+0.20%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 20, 2024

karmada-bot requested review from ikaven1024 and XiShanYongYe-Chang November 20, 2024 14:54

karmada-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 20, 2024

zhzhuang-zju reviewed Nov 22, 2024

View reviewed changes

Added unit tests for interpretation context handling

f8f3274

Signed-off-by: Anuj Agrawal <[email protected]> Added unit tests for interpretation context handling Signed-off-by: Anuj Agrawal <[email protected]>

anujagrawal699 force-pushed the addedTests-pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext.go branch from 6e61b1f to f8f3274 Compare November 22, 2024 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added unit tests for interpretation context handling #5850

Added unit tests for interpretation context handling #5850

anujagrawal699 commented Nov 20, 2024

karmada-bot commented Nov 20, 2024

zhzhuang-zju Nov 22, 2024

chaosi-zju Nov 22, 2024 •

edited

Loading

zhzhuang-zju Nov 22, 2024

chaosi-zju Nov 22, 2024

zhzhuang-zju Nov 22, 2024

XiShanYongYe-Chang Nov 22, 2024

zhzhuang-zju Nov 22, 2024

anujagrawal699 Nov 22, 2024

zhzhuang-zju Nov 22, 2024

zhzhuang-zju Nov 22, 2024

anujagrawal699 Nov 22, 2024

zhzhuang-zju Nov 22, 2024

anujagrawal699 Nov 22, 2024

zhzhuang-zju Nov 22, 2024

anujagrawal699 Nov 22, 2024

XiShanYongYe-Chang Nov 22, 2024

anujagrawal699 Nov 22, 2024

codecov-commenter commented Nov 22, 2024

Added unit tests for interpretation context handling #5850

Are you sure you want to change the base?

Added unit tests for interpretation context handling #5850

Conversation

anujagrawal699 commented Nov 20, 2024

karmada-bot commented Nov 20, 2024

Choose a reason for hiding this comment

chaosi-zju Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 22, 2024

Codecov Report

chaosi-zju Nov 22, 2024 •

edited

Loading