Skip to content

Conversation

@ChayanDass
Copy link
Contributor

@ChayanDass ChayanDass commented Feb 1, 2026

Summary

This PR expands backend test coverage for the experimental k8cache by validating cache invalidation on Add, Update, and Delete informer events. Previously, tests exercised only Add events; this change ensures Update/Delete handlers are also covered and behave correctly.

Related Issue

Fixes the review feedback on backend cache invalidation tests from PR

Fixes #4506

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 1, 2026
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 1, 2026
@illume illume requested a review from Copilot February 1, 2026 10:42
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances the test coverage for the experimental k8cache package by adding tests for Update and Delete informer events. Previously, the TestRunInformerToWatch test only covered Add events; this change ensures that cache invalidation works correctly for all three event types (Add, Update, and Delete).

Changes:

  • Added eventType field to test case structure to distinguish between add, update, and delete events
  • Added two new test cases: one for update events and one for delete events
  • Implemented event-specific logic in a switch statement to trigger the appropriate Kubernetes client operations for each event type
  • Fixed variable naming (schema → scheme) for consistency with Go conventions
  • Added necessary imports (context and metav1)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

Thanks!

The GitHub check is failing on a lint issue. You can run lint locally...

npm run backend:lint
npm run backend:format
npm run backend:test

Please consider the open review comment?

@ChayanDass ChayanDass force-pushed the test/cacheinvalidation branch from fccf03f to fe0443a Compare February 1, 2026 11:20
@ChayanDass
Copy link
Contributor Author

ChayanDass commented Feb 1, 2026

@illume The lint error was from dupl due to structurally duplicated test data. Changing values didn’t help, so I refactored the shared data into a common fixture/struct.
Please take a look and let me know if it looks good to you , i will squash all my commits .

@illume
Copy link
Contributor

illume commented Feb 1, 2026

@ChayanDass

Thanks for that.

Did you look at the coverage report?

npm run backend:coverage:html
# To just print a simpler coverage report to the console.
npm run backend:coverage

Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

Can you please squash your commits?

@ChayanDass ChayanDass force-pushed the test/cacheinvalidation branch from 1ac1f6b to ebb1adc Compare February 1, 2026 12:31
@ChayanDass
Copy link
Contributor Author

Done!

@illume
Copy link
Contributor

illume commented Feb 1, 2026

Thanks for that.

Just wondering if you could confirm that this is covering the code paths as you expected by looking at the test coverage report?


Some nits on the git commit message for the future (feel free to ignore for this PR)

  • We don't need "Signed-off-by" in this repo, because it uses the CLA.
  • We capitalise the imperative mood verb at the start, because it's like a normal subject line

@ChayanDass
Copy link
Contributor Author

ChayanDass commented Feb 1, 2026

image

before -> 50%
After -> 75%

@ChayanDass
Copy link
Contributor Author

ChayanDass commented Feb 1, 2026

Is the current test coverage okay, or should I add more tests to increase coverage further?

@illume
Copy link
Contributor

illume commented Feb 1, 2026

Seems pretty good. If you look at the html view you can see which code paths are not covered by tests.

@ChayanDass ChayanDass force-pushed the test/cacheinvalidation branch from ebb1adc to 0879f1b Compare February 2, 2026 12:47
@ChayanDass ChayanDass force-pushed the test/cacheinvalidation branch from 0879f1b to 9d5899c Compare February 2, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

backend: cacheInvalidation_test: TestRunInformerToWatch: Add tests for Update and Delete as well

3 participants