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

test: Add error check during cleanup #256

Merged
merged 1 commit into from
Oct 2, 2023
Merged

test: Add error check during cleanup #256

merged 1 commit into from
Oct 2, 2023

Conversation

vprashar2929
Copy link
Collaborator

This PR adds the check for any error in the case when cleanup happens.

@@ -88,7 +93,8 @@ func TestNodeSelector(t *testing.T) {
err = f.AddResourceLabels("node", node, labels)
assert.NoError(t, err, "could not label node")
t.Cleanup(func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vprashar2929 thanks for the patch. Since you are at it, could you also please move t.Cleanup to f.AddResourceLabels itself. There are other functions in the framework that automatically cleans up after itself so that the caller need not worry about cleaning up.

Comment on lines 134 to 138
t.Cleanup(func() {
// remove taint
f.TaintNode(node, fmt.Sprintf("%s-", e2eTestTaint.Key))
err := f.TaintNode(node, fmt.Sprintf("%s-", e2eTestTaint.Key))
assert.NoError(t, err, "could not remove taint from node")
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for the fix above.. can we do the same here 🙏 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vprashar2929 may be in a followup PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Completely forgot this😅 I will fix this as well in the PR

sthaha
sthaha previously approved these changes Sep 28, 2023
Comment on lines 134 to 138
t.Cleanup(func() {
// remove taint
f.TaintNode(node, fmt.Sprintf("%s-", e2eTestTaint.Key))
err := f.TaintNode(node, fmt.Sprintf("%s-", e2eTestTaint.Key))
assert.NoError(t, err, "could not remove taint from node")
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vprashar2929 may be in a followup PR?

@sthaha sthaha merged commit 884e514 into v1alpha1 Oct 2, 2023
9 checks passed
@vprashar2929 vprashar2929 deleted the update-e2e branch October 2, 2023 14:44
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.

2 participants