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

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!

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.45%. Comparing base (2c82055) to head (1575e6a).
Report is 43 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.45%   +0.13%     
==========================================
  Files         661      663       +2     
  Lines       54326    54585     +259     
==========================================
+ Hits        25163    25358     +195     
- Misses      27537    27596      +59     
- Partials     1626     1631       +5

Flag	Coverage Δ
unittests	`46.45% <ø> (+0.13%)`	⬆️

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.

XiShanYongYe-Chang · 2024-11-25T09:19:51Z

cc @zhzhuang-zju

XiShanYongYe-Chang · 2024-11-26T09:13:11Z

Hi @zhzhuang-zju Can you help check whether the changes have met your expectations?

/assign

zhzhuang-zju · 2024-11-26T11:18:11Z

/lgtm

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

Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com> Added unit tests for interpretation context handling Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com> Added unit tests for interpretation context handling Signed-off-by: Anuj Agrawal <anujagrawal380@gmail.com>

karmada-bot · 2024-11-26T12:55:41Z

New changes are detected. LGTM label has been removed.

karmada-bot · 2024-11-26T12:55:48Z

[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 ask for approval from xishanyongye-chang. 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

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

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

karmada-bot assigned XiShanYongYe-Chang Nov 26, 2024

karmada-bot assigned zhzhuang-zju Nov 26, 2024

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024

XiShanYongYe-Chang reviewed Nov 26, 2024

View reviewed changes

pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext_test.go Outdated Show resolved Hide resolved

anujagrawal699 force-pushed the addedTests-pkg/resourceinterpreter/customized/webhook/request/resourceinterpretercontext.go branch from f8f3274 to 1575e6a Compare November 26, 2024 12:55

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2024

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

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 •

edited

Loading

XiShanYongYe-Chang commented Nov 25, 2024

XiShanYongYe-Chang commented Nov 26, 2024

zhzhuang-zju commented Nov 26, 2024

karmada-bot commented Nov 26, 2024

karmada-bot commented Nov 26, 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

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 • edited Loading

Codecov Report

XiShanYongYe-Chang commented Nov 25, 2024

XiShanYongYe-Chang commented Nov 26, 2024

zhzhuang-zju commented Nov 26, 2024

karmada-bot commented Nov 26, 2024

karmada-bot commented Nov 26, 2024

chaosi-zju Nov 22, 2024 •

edited

Loading

codecov-commenter commented Nov 22, 2024 •

edited

Loading