Skip to content

test: add missing unit tests for adk and core packages - #2520

Open
yashrajshuklaaa wants to merge 6 commits into
kagent-dev:mainfrom
yashrajshuklaaa:chore/add-missing-tests
Open

yashrajshuklaaa wants to merge 6 commits into
kagent-dev:mainfrom
yashrajshuklaaa:chore/add-missing-tests

Conversation

@yashrajshuklaaa

@yashrajshuklaaa yashrajshuklaaa commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor

What

Unit tests for go/adk/pkg/auth and go/adk/pkg/runner. No production code changes.

Changes

  • auth/token_test.go: token lifecycle, header injection, TokenRoundTripper.
  • runner/adapter_test.go: agentNameFromAppName, splitCSV, buildTokenPropagationPlugin env branching, and CreateRunnerConfig wiring (app-name default, session fallback, missing-model error).

Revision note

Per @EItanya's review: removed tests for core/internal/controller/reconciler/{status,utils}, translator, and service/agent. Their implementation moved or was removed on main, so they no longer compiled. Removed two ShareTools test cases that didn't actually test ShareTools behavior. Removed the mock HTTP server and fixtures since no test called the model; CreateRunnerConfig tests now build the config inline.

Validation

go build, go vet, go test - All pass.

… reconciler, reconciler utils

Signed-off-by: Yashraj Shukla <shuklayashraj68@gmail.com>
@yashrajshuklaaa
yashrajshuklaaa requested review from a team and supreme-gg-gg as code owners August 21, 2026 23:27
@github-actions github-actions Bot added the testing Additional testing required label Aug 21, 2026
@yashrajshuklaaa
yashrajshuklaaa force-pushed the chore/add-missing-tests branch 2 times, most recently from bff00a2 to c06a3f0 Compare August 22, 2026 00:10
assert.True(t, serviceerrors.IsCode(err, serviceerrors.CodePermissionDenied))
}

func assertAnError() error {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

name says assert, but this just builds an error. confusing name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed :) @mesutoezdil
PTAL

Spec: v1alpha3.SandboxAgentSpec{
Type: v1alpha3.AgentType_Declarative,
Declarative: &v1alpha3.DeclarativeAgentSpec{
// ModelConfig deliberately omitted / points at a ModelConfig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

comment says omitted, but code sets it to a missing name. pick one wording.

@yashrajshuklaaa yashrajshuklaaa Aug 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

resolved! thanks for the suggestion.

Signed-off-by: Yashraj Shukla <shuklayashraj68@gmail.com>
…CreateRunnerConfig via mockllm)

Signed-off-by: Yashraj Shukla <shuklayashraj68@gmail.com>
Signed-off-by: Yashraj Shukla <shuklayashraj68@gmail.com>
@yashrajshuklaaa
yashrajshuklaaa force-pushed the chore/add-missing-tests branch from c06a3f0 to baa973f Compare August 25, 2026 03:33
@yashrajshuklaaa

Copy link
Copy Markdown
Contributor Author

@EItanya @supreme-gg-gg the test-e2e failure (TestAgentInstanceActiveTask) looks unrelated to this PR. it's a stream/actor-suspend race in an e2e test and this PR only adds unit tests with no production code changes. Could one of you re-run the job or take a look when you get a chance?

@yashrajshuklaaa

Copy link
Copy Markdown
Contributor Author

@EItanya Could you please approve the workflow run so CI can complete?

@EItanya

EItanya commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-assisted review.

Thanks for putting time into improving test coverage! I think this PR would be more useful if we narrowed it to tests that protect current behavior against regressions.

  • Several of the new core tests reference packages or functions that have since been removed. At the current head (05d13d19), the focused test run fails to compile four core packages. Those obsolete cases should be removed.
  • The shared-tools tests only assert that an agent is created, but CreateRunnerConfig no longer uses controllerClient or ShareTools. They pass without exercising the behavior their names describe, so those cases can also be removed.
  • The runner configuration tests never call the model, so the mock HTTP server and response fixture seem unnecessary.

The auth header assertions and parsing edge cases look worth keeping. Could you trim the PR to focused tests for behavior that still exists, with assertions that would fail if that behavior broke? A smaller suite with clear regression coverage would be a stronger addition.

…ests

Signed-off-by: Yashraj Shukla <shuklayashraj68@gmail.com>
Signed-off-by: Yashraj Shukla <shuklayashraj68@gmail.com>
@github-actions github-actions Bot added testing Additional testing required and removed testing Additional testing required labels Sep 25, 2026
@yashrajshuklaaa

Copy link
Copy Markdown
Contributor Author

@EItanya all fixed. Removed the obsolete core tests, the hollow ShareTools tests, and the unused mock server. PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Additional testing required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants